-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Don't emit analyzer warnings for MakeGeneric understood by ILC #100858
Conversation
Follow up to dotnet#99037. Resolves dotnet#81204. When `MakeGenericXXX` API is used in the "bridging constraints" pattern and all types/members involved are statically known, don't emit warning from the Roslyn analyzer since ILC will do the right thing and ensure this works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you think about sharing some of these tests between ILC and the analyzer?
if (!instance.IsEmpty ()) { | ||
foreach (var value in instance.AsEnumerable ()) { | ||
if (value is SystemTypeValue) { | ||
if (!IsKnownInstantiation (arguments[0])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also check that the instantiation length matches the method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the instantiation length doesn't match, we'd get an exception at runtime both before and after AOT compilation, so maybe we don't need an AOT warning. The code is wrong, but we don't concern ourselves with bugs that exist before and after publish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should this return true
if it's not a known instantiation, but the length doesn't match? I think that's what ILC does if I read #99037 correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh never mind, it triggers the warning if TryGetMakeGenericInstantiation returns false. Probably not an important edge case anyway.
Do we have a good spot for that? For the AOT testing, there's more concern about whether it actually works at runtime, so I wrote tests that actually execute. We don't typically execute the code in illink/analyzer testing. |
https://github.com/dotnet/runtime/blob/main/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/MakeGenericDataFlow.cs might be a good place, except this suppresses IL3050 currently. You could fix up the ExpectedWarnings for AOT or add a new file next to that one. Just a suggestion, I think the current approach is OK too. |
…t#100858) Follow up to dotnet#99037. Resolves dotnet#81204. When `MakeGenericXXX` API is used in the "bridging constraints" pattern and all types/members involved are statically known, don't emit warning from the Roslyn analyzer since ILC will do the right thing and ensure this works.
Follow up to #99037.
Resolves #81204.
When
MakeGenericXXX
API is used in the "bridging constraints" pattern and all types/members involved are statically known, don't emit warning from the Roslyn analyzer since ILC will do the right thing and ensure this works.Cc @dotnet/ilc-contrib