tunnelvisionlabs / ReferenceAssemblyAnnotator

IL weaver to add nullability annotations to .NET reference assemblies
MIT License
71 stars 8 forks source link

Add [MemberNotNull] and [MemberNotNullWhen] support #85

Closed kevinchalet closed 4 years ago

kevinchalet commented 4 years ago

Fixes https://github.com/tunnelvisionlabs/ReferenceAssemblyAnnotator/issues/84.

kevinchalet commented 4 years ago

@jnm2 @sharwell PTAL 😄

kevinchalet commented 4 years ago

Wooops, looks like I broke something. I'll take a look 😅

kevinchalet commented 4 years ago

I tried to add [ParamArray] to the members parameters to see if it could help, but it didn't.

I'm out of ideas. I'll wait for @sharwell to magically find the root cause in 3 seconds 🤣

jnm2 commented 4 years ago

Hmm, I can confirm that this repros locally for me on member_not_null but not on master:

msbuild /restore /verbosity:minimal
git clean -xdf tests
dotnet msbuild -restore tests/MultiTFM -warnaserror -nr:false -v:m
kevinchalet commented 4 years ago

What's super weird is that the error ("Mono.Cecil.ResolutionException: Failed to resolve System.AttributeTargets") is about a type reference I haven't changed in this PR 😕

I added the nullable/nullable context attributes as you suggested. Dunno if it will fix it tho 😄

image

image

jnm2 commented 4 years ago

Waiting for @sharwell seems like a good plan. The only idea I have is to do a binary search of deleting half your changes repeatedly until we find the line that seems to perturb the behavior into failing. Bit slow of a process.

kevinchalet commented 4 years ago

@jnm2 I'll wait for @sharwell's feedback.

Anyway, thanks a lot for your assistance and your review. Much appreciated 😃

kevinchalet commented 4 years ago

Alright, I'm now out of ideas and this is way a more painful process than I had anticipated.

@jnm2 @sharwell this is now failing in the WPF tests. If you guys have an idea, let me know. Otherwise, I'll close this PR as I don't think I'll be able to fix that.

kevinchalet commented 4 years ago

Latest commits improved things, but we're back to the Failed to resolve System.AttributeTargets error that makes no sense to me...

sharwell commented 4 years ago

@kevinchalet Been busy but hoping to get to this 😄

kevinchalet commented 4 years ago

Haha, no worries. I closed this PR as I wasn't able to fix it on my own, but I'd be happy to revive it if we can find the root cause of the issue 😄