Skip to content
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

Fix initialization of FakeProperty with setter #2792

Merged
merged 3 commits into from
Nov 3, 2022

Conversation

exyi
Copy link
Contributor

@exyi exyi commented Sep 29, 2022

ILSpy crashed with NullReferenceException when it couldn't resolve some dependency (Newtonsoft.Json in my case, the problem was around JObject.Item[string] indexer)

Seems like a typo when FakeProperty is initialized - the setter method was assigned to the Getter property

Solution

First commit contains the actual fix.

In the second commit I included some asserts and a ToString implementation I used to debug the problem. I'm guessing that more asserts would be welcome, but if you don't like it, just merge only the first commit.

  • At least one test covering the code changed
    • Sorry, but I couldn't make the tests run on Linux, is there any way?
    • I also don't know how to replicate this problem in unit tests - I'd have to compile a dll against some library which the decompiler won't see at runtime.

@siegfriedpammer
Copy link
Member

  • I also don't know how to replicate this problem in unit tests - I'd have to compile a dll against some library which the decompiler won't see at runtime.

You could create an IL-Pretty-Test like this one: https://github.com/icsharpcode/ILSpy/blob/master/ICSharpCode.Decompiler.Tests/TestCases/ILPretty/UnknownTypes.il

ILAsm will happily assemble a file that contains references to unknown types/members.

Seems like a typo - the setter method was assigned to the Getter property

The test GuessAccessors needed adjustments in the generated code.
ILSpy is more eager to merge property assignments
@exyi
Copy link
Contributor Author

exyi commented Oct 9, 2022

I added a test case which I believe covers the changed code.

I also needed to slightly change output of one the existing test. It does not change the meaning of the code, but it's now eager to merge assignments into var v = (x.Prop=c.Prop) expression and then it does not know how to name the variable v.

@siegfriedpammer siegfriedpammer merged commit 6dc55eb into icsharpcode:master Nov 3, 2022
@siegfriedpammer
Copy link
Member

Thank you for your contribution!

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 this pull request may close these issues.

None yet

2 participants