Skip to content

Pythonnet 2.1 not identifying System.Object type #203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vmuriart opened this issue Apr 17, 2016 · 33 comments · Fixed by #377
Closed

Pythonnet 2.1 not identifying System.Object type #203

vmuriart opened this issue Apr 17, 2016 · 33 comments · Fixed by #377

Comments

@vmuriart
Copy link
Contributor

I'm not sure how the tests wouldn't have caught this bug.
I have a .net class that has the function AddQuery with the following method signatures.

AddQuery
    Void AddQuery(api.QueryType)...
    Void AddQuery(System.Object)...

where QueryType is another custom object type.

On 2.0 and 2.1dev1 the function works as expected.

  • I can pass a QueryType object to AddQuery and it would call the first method.
  • Calling AddQuery with a string or int would use the second method and process them.

On 2.1.0 and the earliest archived copy in appveyor this behavior changed.

  • Passing QueryType object to AddQuery still works.
  • Passing string type to AddQuery errors with No method matches given arguments

I've tested on Python 2.7, Python 3.5 both x64.
Looks like Pythonnet is no longer identifying string int and other types to be of type System.Object as well.

@vmuriart
Copy link
Contributor Author

Possibly related to #187
Tested on windows 7 and 8.1

@vmuriart
Copy link
Contributor Author

Tried build before and after #176, both still have this bug present.

@den-run-ai
Copy link
Contributor

@vmuriart
try passing System.Object("your_python_string") or System.Object(123) for string or int types respectively.

this breaking change is because implicit casting can be dangerous:

#131

@vmuriart
Copy link
Contributor Author

@denfromufa just tried your suggestion and it semi-works. It passes through pythonnet and passes the Object-type string to the library I'm using, and breaks there. The underlying function is expecting a String-type string and fails to identify the Object-type string as such.

@den-run-ai
Copy link
Contributor

did you try System.String() and System.Int32()? or maybe even System.Object(System.String())...

@vmuriart
Copy link
Contributor Author

I did. those to work, but have to explicitly cast each type before sending it.
I'm taking a look at the src; I think there might be a better solution for the original bug #131 that would both fix the bug and not break legacy code. Might even try to fix #90 finally 😄

@den-run-ai
Copy link
Contributor

den-run-ai commented Apr 17, 2016

@vmuriart I'm thinking of context manager that would allow you to specify "unsafe" casting operations like before for overloaded methods, see discussion here (my 4th comment):

#174

@den-run-ai
Copy link
Contributor

@vmuriart I think for your case, the overloading resolution in pythonnet could be improved by checking isinstance("string",System.Object) which corresponds to is operator in c#, but this is still dangerous - what if both types are present in 2 different overloaded methods?

@vmuriart
Copy link
Contributor Author

I see your point. I'm not familiar enough with the source code in relation to how pythonnet decides which method to call.
The way I was thinking of it is that each Python type has an underlying c-type associated to it which should give an indication how to properly cast it into its equivalent .net counterpart. That may or may not work as a solution here though

@den-run-ai
Copy link
Contributor

@vmuriart python and .net types cannot be directly mapped to c-types due to much more complicated type system. all conversions are manually taken care for basic types in pythonnet:

https://github.com/pythonnet/pythonnet/blob/master/src/runtime/converter.cs

@vmuriart
Copy link
Contributor Author

The problem that I am having with it at the moment is that quite a few of the libraries have broken with 2.1 release. Not sure if in this case the benefit outweighs the loss of backward compatibility.
@tonyroberts it seems pr #151 has introduced a few incompatibilities in the attempt to solve #131. Can #151 be reverted until a better fix is in place?

@den-run-ai
Copy link
Contributor

Can we just look at all types of incompatibilities that you experience with
new method overloading?

Then we should really think whether the wrong results from
method overloading are less important than possible exceptions
with incompatible types? It would be very hard to spot the former and very
easy to fix or workaround the latter.

On Sunday, April 17, 2016, Vik notifications@github.com wrote:

The problem that I am having with it at the moment is that quite a few of
the libraries have broken with 2.1 release. Not sure if in this case the
benefit outweighs the loss of backward compatibility.
@tonyroberts https://github.com/tonyroberts it seems pr #151
#151 has introduced a few
incompatibilities in the attempt to solve #131
#131. Can #151
#151 be reverted until a b
etter fix is in place?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#203 (comment)

@tonyroberts
Copy link
Contributor

@vmuriart @denfromufa my preference would be to attempt to fix 2.1 to fix the backward compatibilities, rather than just roll back. If it's just adding a case so that System.Object matches everything then hopefully that wouldn't be a big change, would it? Or are there more problems than that?

@tonyroberts
Copy link
Contributor

@vmuriart would it be easy for you to add a few unit tests to check what's currently breaking? I think it's better to show the build currently as broken, if the only reason it's not showing as broken now is just lack of test coverage! :)

@den-run-ai
Copy link
Contributor

@tonyroberts so match even python object on System.Object?

@tonyroberts
Copy link
Contributor

tonyroberts commented Apr 18, 2016

@denfromufa it may be more robust to check for base classes and interfaces of the managed class mapped to the Python type

@den-run-ai
Copy link
Contributor

@tonyroberts @vmuriart can you provide me with few examples which need to be fixed?

@vmuriart
Copy link
Contributor Author

I'll write up a couple tests for the issue

On Monday, April 18, 2016, denfromufa notifications@github.com wrote:

@tonyroberts https://github.com/tonyroberts @vmuriart
https://github.com/vmuriart can you provide me with few examples which
need to be fixed?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#203 (comment)

@vmuriart
Copy link
Contributor Author

@denfromufa try this test.
It's failing with the same mechanism I saw earlier.
vmuriart@7acf93f

@den-run-ai
Copy link
Contributor

what would you expect for this test on python side when calling this CLR
method?

On Mon, Apr 18, 2016 at 2:47 PM, Vik notifications@github.com wrote:

@denfromufa https://github.com/denfromufa try this test.
It's failing with the same mechanism I saw earlier.
vmuriart/pythonnet@7acf93f
vmuriart@7acf93f


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#203 (comment)

@vmuriart
Copy link
Contributor Author

Just verified that this used to work on the 2.0 version with this commit vmuriart/pythonnet@e5c4e74...f77695f

The test is pretty simple, I don't particularly care about the output just that the behavior still works.

@den-run-ai
Copy link
Contributor

i just noticed the python code, sorry for confusion.

What if the overloaded method with System.String is added? Then your tests
become ambiguous, so how should pythonnet handle this?

On Mon, Apr 18, 2016 at 3:11 PM, Vik notifications@github.com wrote:

Just verified that this used to work on the 2.0 version
https://travis-ci.org/vmuriart/pythonnet/jobs/124006627 with this
commit vmuriart/pythonnet@e5c4e74...f77695f
vmuriart/pythonnet@e5c4e74...f77695f

The test is pretty simple, I don't particularly care about the output just
that the behavior still works.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#203 (comment)

@den-run-ai
Copy link
Contributor

@vmuriart your tests are not complete - you do not check that correct overloaded method is selected. Some sort of asserting is required.

@vmuriart
Copy link
Contributor Author

Correct. I meant the test as a proof of concept to demonstrate how to repeat the bug and that the behavior worked on the old version.

@den-run-ai
Copy link
Contributor

@vmuriart sorry - I'm still not clear, what do you expect, if overloaded method with System.String argument is added?

this is definitely not bug, since no agreed upon test is available for this scenario as of yet and implicit overloading is not supported anymore.

@tonyroberts
Copy link
Contributor

@denfromufa if an overload with argument type System.String was added then that would be used instead when a string was passed. If a non string type (but one still deriving from System.Object, e.g. System.Int32) was used then the object method would be used.

There's no implicit conversion from one type to another going on, a string is an object so the overload is valid.

Here's a link that might be useful
https://msdn.microsoft.com/en-us/library/aa691356(v=vs.71).aspx

@den-run-ai
Copy link
Contributor

@tonyroberts thank you for this explanation. Let me now write this code for base classes.

@vmuriart
Copy link
Contributor Author

@tonyroberts thanks for explaining it! I was struggling on getting the right words together.

vmuriart added a commit to vmuriart/pythonnet that referenced this issue May 22, 2016
patch for pythonnet#203 - not best solution though.
den-run-ai pushed a commit to den-run-ai/pythonnet that referenced this issue May 26, 2016
@matthid
Copy link
Contributor

matthid commented Dec 4, 2016

Just an idea: Maybe Roslyn can help with issues like these. If we can re-use their (or parts of their) overload resolution logic we don't need to reimplement the spec.

On the other hand: Going a different route is valid as well. In F# you actually need to mark implicit casts with an operator (while this case would still work as everything is an object)

@den-run-ai
Copy link
Contributor

@matthid i was thinking about re-using roslyn as well, but then we have a lot of python-specific logic.

what do you mean by "On the other hand: Going a different route is valid as well"?

@matthid
Copy link
Contributor

matthid commented Dec 4, 2016

What I meant was that pythonnet doesn't need to mirror C# overload resolution logic. Instead it is completely fine to define its own (possibly simpler) logic. For this issue I'd say that most people expect this to work. But IMHO it could be closed as "wontfix" as well (and say that "by-design" types need to match exactly).

It depends on what spec/design guideline this project follows. As you are trying to mirror ironpython(?) this is most likely a bug :)

vmuriart added a commit that referenced this issue Jan 6, 2017
patch for #203 - not best solution though.
vmuriart added a commit to vmuriart/pythonnet that referenced this issue Jan 29, 2017
patch for pythonnet#203 - not best solution though.
vmuriart added a commit to vmuriart/pythonnet that referenced this issue Jan 29, 2017
patch for pythonnet#203 - not best solution though.
vmuriart added a commit to vmuriart/pythonnet that referenced this issue Jan 29, 2017
patch for pythonnet#203 - not best solution though.
vmuriart added a commit to vmuriart/pythonnet that referenced this issue Jan 29, 2017
patch for pythonnet#203 - not best solution though.
@vmuriart vmuriart mentioned this issue Feb 3, 2017
vmuriart added a commit to vmuriart/pythonnet that referenced this issue Feb 5, 2017
patch for pythonnet#203 - not best solution though.
vmuriart added a commit to vmuriart/pythonnet that referenced this issue Feb 9, 2017
patch for pythonnet#203 - not best solution though.
vmuriart added a commit to vmuriart/pythonnet that referenced this issue Feb 14, 2017
patch for pythonnet#203 - not best solution though.
@den-run-ai
Copy link
Contributor

it appears that precedence of types is simplified version of Jython, found by @vmuriart 👍

https://github.com/pythonnet/pythonnet/blob/master/src/runtime/methodbinder.cs#L170

I'm not sure how this compares to IronPython, but likely very similar due to the same author:
https://en.wikipedia.org/wiki/Jim_Hugunin

@vmuriart
Copy link
Contributor Author

Fun fact on the history of Jim and IronPython.

IronPython was created in 2005 by Jim Hugunin to prove that the .NET Framework was a poor platform for dynamic languages. He failed to do so, and IronPython was born.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants