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 resolving from a ModuleReference #730

Merged
merged 2 commits into from
Apr 23, 2021
Merged

Fix resolving from a ModuleReference #730

merged 2 commits into from
Apr 23, 2021

Conversation

AnakinRaW
Copy link
Contributor

As requested here is the failing PR for #725 .
I managed to use the existing test resources mma.exe to reproduce the error.

The test simple asserts that no exception shall get thrown, but since I'm not sure how you want to handle this situation, this is just a placeholder and should be replaced once a decision on actual behavior is made.

@AnakinRaW AnakinRaW changed the title add failing test Failing Test for NRE when trying to resolve Members from a ModuleReference Mar 15, 2021
@jbevain
Copy link
Owner

jbevain commented Mar 15, 2021

That's related to #652. Funny that the code assumes this works.

@jbevain jbevain changed the title Failing Test for NRE when trying to resolve Members from a ModuleReference Fix resolving Members from a ModuleReference Mar 16, 2021
@jbevain jbevain changed the title Fix resolving Members from a ModuleReference Fix resolving from a ModuleReference Mar 16, 2021
@jbevain
Copy link
Owner

jbevain commented Mar 16, 2021

Also fix #652

@AnakinRaW
Copy link
Contributor Author

changes look good for me, thanks.
Maybe a few other thoughts:

  1. I guess it should be NOT part of Cecil to find a weak "ModuleReference match". But since MetadataResolver::Resolve is virtual i guess there should be possibility for anyone to have custom logic. I like that!
  2. At this point I think it would be cool to have a nullable syntax/doc for properties. Are there any concerns about this? Is this a new issue? Enabling C# nullable syntax might reveal more such issues.

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.

2 participants