-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Reapply "Implement type name resolution for ILLink analyzer (#106209)" (#106343) #116505
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
…06209)" (dotnet#106343) This reverts commit 5ee1b86.
The referenced issue has since been fixed
The analyzer shouldn't use TypeName.Unescape which was added in .NET 10 (since Roslyn references Sytem.Reflection.Metadata 9.0.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.
Pull Request Overview
This PR re-applies and updates changes from a previous fix for type name resolution in the ILLink analyzer. Key changes include the removal of tool-specific parameters from ExpectedWarning attributes in tests, propagation of a new TypeNameResolver parameter throughout the analyzer code, and updates to project configuration for new dependencies.
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/AttributePropertyDataflow.cs | Removed tool parameters from the ExpectedWarning attribute to simplify warning checks. |
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/AttributeFieldDataflow.cs | Similar removal of extra parameters from ExpectedWarning attributes. |
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/AttributeConstructorDataflow.cs | Removed extra warning parameters from ExpectedWarning, updating test expectations. |
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/AssemblyQualifiedNameDataflow.cs | Updated ExpectedWarning attribute message by removing extra parameters. |
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/TestCaseCompilation.cs | Changed the assemblyName from a generated GUID to a static "test" string. |
(Multiple analyzer files) | Introduced a new TypeNameResolver parameter and propagated it for improved type resolution logic. |
src/tools/illink/src/ILLink.RoslynAnalyzer/ILLink.RoslynAnalyzer.csproj | Added AllowUnsafeBlocks and a new package reference for System.Reflection.Metadata. |
Comments suppressed due to low confidence (7)
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/AttributePropertyDataflow.cs:58
- Removed tool-specific parameters from the ExpectedWarning attribute. Ensure that the revised warning expectation matches the analyzer's output in all scenarios.
[ExpectedWarning ("IL2026", "--ClassWithKeptPublicMethodsKeptByName--")]
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/AttributeFieldDataflow.cs:43
- Removed extra parameters from the ExpectedWarning attribute to simplify warning checks. Confirm that this change aligns with the intended analyzer behavior.
[ExpectedWarning ("IL2026", "--ClassWithKeptPublicMethods--")]
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/AttributeConstructorDataflow.cs:28
- Simplified the ExpectedWarning attribute by removing redundant tool information. Verify that the analyzer still triggers the expected warning.
[ExpectedWarning ("IL2026", "--ClassWithKeptPublicMethods--")]
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/AssemblyQualifiedNameDataflow.cs:61
- By removing extra diagnostic parameters, ensure that the test still properly verifies the need for assembly-qualified type names.
[ExpectedWarning ("IL2122", "Type 'System.Invalid.TypeName' is not assembly qualified. Type name strings used for dynamically accessing a type should be assembly qualified.")]
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/TestCaseCompilation.cs:47
- Changed assemblyName from a generated GUID to a static value. Confirm that this change does not disrupt tests that may rely on unique assembly identifiers.
assemblyName: "test",
src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/RequireDynamicallyAccessedMembersAction.cs:40
- Ensure that the type resolution logic integrates correctly with the new TypeNameResolver parameter and that generic argument processing remains robust.
if (_reflectionAccessAnalyzer.TryResolveTypeNameAndMark (typeName, diagnosticContext, needsAssemblyName, out ITypeSymbol? foundType)) {
src/tools/illink/src/ILLink.RoslynAnalyzer/ILLink.RoslynAnalyzer.csproj:33
- New project settings include AllowUnsafeBlocks and a package reference for System.Reflection.Metadata. Verify that these changes are documented appropriately and that all required dependencies are satisfied.
<PackageReference Include="System.Reflection.Metadata" Version="$(SystemReflectionMetadataVersion)" />
SystemReflectionMetadataVersion is 10.0.0-preview.6 now, does that affect whether this will work? The repo builds for me with a global install, but not sure if this would still cause issues in VS. |
@@ -28,6 +30,7 @@ | |||
<PrivateAssets>all</PrivateAssets> | |||
<ExcludeAssets>contentfiles</ExcludeAssets> <!-- We include our own copy of the ClosedAttribute to work in source build --> | |||
</PackageReference> | |||
<PackageReference Include="System.Reflection.Metadata" Version="$(SystemReflectionMetadataVersion)" /> |
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.
Do we want SystemReflectionMetadataToolsetVersion
here? Basing off of this comment
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
Any more testing we need for this? |
@sbomer do you want to resolve the merge conflicts? |
Will do - I need to revisit the questions about the SRM version and ideally check that this doesn't hit an issue similar to #106321. I had some trouble reproing that last time and want to make sure the issue is actually fixed. |
Fixes #95118
Reapplies #106209 which was reverted due to #106321. Now that roslyn has updated to reference System.Reflection.Metadata 9.0, we can use
TypeName
. However, the code was usingTypeNameHelpers.Unescape
which was moved toTypeName.Unescape
in .NET 10, sowe need to preserve a copy of the
Unescape
logic for use by the analyzer.