Fix a StackOverflowException. by matthid · Pull Request #250 · pythonnet/pythonnet

@matthid

Currently I added only the test, and I have no ETA when I can provide the fix. If anyone wants to step in, feel free. I noticed this with F# code (as the compiler will generate a lot of nested types when using DUs).

@matthid

@den-run-ai

@matthid is this associated with any particular known issue?

@matthid

@denfromufa I did not find anything

@matthid

Note for someone hit by this: In my situation a workaround was to mark the class as internal, as there was no need for it to be exposed...

@matthid

@matthid

Not sure if its the best fix as there now we potentially have uninitialized instances floating around at the place I marked with a comment.

However a "better" fix would need to consider this (edge-)case from the start and would need drastic changes...

@matthid matthid changed the title WIP try to fix a stackoverflowexception. Fix a StackOverflowException.

Aug 26, 2016

@den-run-ai

@matthid can you provide this edge case as a test?

Also I think the test should be formatted something like a node class and its child as subclass, otherwise it is confusing why would people ever need recursive types like this. So the use case is for trees and graphs?

@matthid

@denfromufa not sure I understand. I already added a testcase.
I'm wondering why the use case matters.

Anyway I tried to explain that the F# compiler generates classes like these for discriminated unions. And I have from time to time written such constructs myself (for exactly the same concept in C#).

Problem is that I had the stack overflow in a class I was not even using in interop (this is due the fact how the F# compiler organizes code)

Hope that helps.

@matthid

To clarify: The "recursive" class was not even used in the interop and I still got this error because it was part of a F# module (which will be translated to a static class)...

@matthid

Just let me know if there is still something missing in this PR.
@denfromufa If I should refactor the test classes can you please suggest the names I should use? Thank you

@den-run-ai

@matthid are there any known side-effects of "...we potentially have uninitialized instances floating around..."?

@matthid

@denfromufa Not that I'm aware of. On the other hand this case is tested with the unit test. It's just that it "feels" a little bit unclean.

@den-run-ai

@filmor

From my side this looks fine.