-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Add issue links / reasons for more ExpectedWarningAttributes with tool exceptions #116799
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
base: main
Are you sure you want to change the base?
Conversation
Tagging subscribers to this area: @dotnet/illink |
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.
Pull Request Overview
This PR adds explicit reason strings (typically issue links or “NativeAOT-specific warning”) to ExpectedWarning
/UnexpectedWarning
attributes across many linker and analyzer test cases so that tools that don’t emit certain warnings can be better tracked and documented.
- Added a GitHub issue URL for redundant suppression warnings (
IL2121
/IL2071
) in trimmer tests. - Provided a “NativeAOT-specific warning” message for analyzer and Native AOT warnings (
IL3002
/IL3050
). - Updated behavioral tests (
UnexpectedWarning
) in dataflow and exponential scenarios with issue links.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/tools/illink/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/DetectRedundantSuppressionsInMembersAndTypesUsingTarget.cs | Added issue link reason to trimmer warnings |
src/tools/illink/test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresWithCopyAssembly.cs | Added “NativeAOT-specific warning” for method warnings |
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ExponentialDataFlow.cs | Marked analyzer warning as unexpected with issue link |
Comments suppressed due to low confidence (3)
src/tools/illink/test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresWithCopyAssembly.cs:45
- [nitpick] The literal "NativeAOT-specific warning" is duplicated across many tests. Consider defining it as a shared const (e.g.
const string NativeAotWarningReason
) and reusing it to reduce duplication and typos.
[ExpectedWarning ("IL3002", "--Method--", Tool.Analyzer | Tool.NativeAot, "NativeAOT-specific warning")]
src/tools/illink/test/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/DetectRedundantSuppressionsInMembersAndTypesUsingTarget.cs:40
- [nitpick] The issue link is repeated in multiple attributes throughout these test files. Extract
"https://github.com/dotnet/linker/issues/1971"
into a shared constant (e.g.const string LinkerIssue1971
) to improve maintainability.
[ExpectedWarning ("IL2121", "IL2071", Tool.Trimmer, "https://github.com/dotnet/linker/issues/1971")]
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ExponentialDataFlow.cs:103
- [nitpick] The runtime issue URL is repeated in many test attributes here. Consider extracting
"https://github.com/dotnet/runtime/issues/87596"
into a constant or helper so future updates are easier.
[ExpectedWarning ("IL2091", "'T'", Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/87596")]
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/CompilerGeneratedCodeDataflow.cs
Outdated
Show resolved
Hide resolved
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ExceptionalDataFlow.cs
Outdated
Show resolved
Hide resolved
...st/Mono.Linker.Tests.Cases/Warnings/WarningSuppression/DetectRedundantSuppressionsFromXML.cs
Outdated
Show resolved
Hide resolved
|
||
[ExpectedWarning ("IL2026", nameof (DerivedTypeWithRequires_BaseMethodWithRequires))] | ||
[ExpectedWarning ("IL2026", nameof (DerivedTypeWithRequires_BaseMethodWithRequires.MethodWithRequires))] | ||
[ExpectedWarning ("IL3050", nameof (DerivedTypeWithRequires_BaseMethodWithRequires), Tool.Analyzer | Tool.NativeAot, "NativeAOT-specific warning")] |
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.
Not necessarily for this PR, but it might reduce some noise if we had an ExpectedAotWarning
so we don't have to pass Tool.Analyzer | Tool.NativeAot, "NativeAOT-specific warning"
to all of these.
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.
LGTM, thank you!
Adds links for a number of expected warnings in test cases that aren't produced by all 3 tools.
Adds a reason string for IL3XXX warnings that are produced by both the analyzer and Native AOT.