Implement type name resolution for ILLink analyzer #106209

sbomer · 2024-08-09T21:32:44Z

This reverts commit 0e0f51d.

jtschuster

A couple style/organization nits, but LGTM otherwise, thank you!

src/tools/illink/src/ILLink.Shared/TrimAnalysis/RequireDynamicallyAccessedMembersAction.cs

jtschuster · 2024-08-12T18:13:55Z

...ols/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/RequireDynamicallyAccessedMembersAction.cs

+		readonly Location _location;
+		readonly Action<Diagnostic>? _reportDiagnostic;


Do we need these in addition to the _diagnosticContext? I don't see anywhere where they would have different values

Yes, _location needs to be separate so that we can pass it through to ReflectionAccessAnalyzer, see #105956 for more context.

src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TypeNameResolver.cs

jtschuster · 2024-08-12T18:26:39Z

...ols/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/RequireDynamicallyAccessedMembersAction.cs


 namespace ILLink.Shared.TrimAnalysis
 {
 	internal partial struct RequireDynamicallyAccessedMembersAction
 	{
+		readonly Compilation _compilation;


nit: It looks like this Compilation is only ever passed down to ReflectionAccessAnalyzer.TryResolveTypeNameAndMark() to create the same TypeNameResolver. Maybe we could just have a TypeNameResolver field instead? And we could then call TryResolveTypeNameAndMark directly on that field and remove ReflectionAccessAnalyzer.TryResolveTypeNameAndMark().

Changed this to pass around TypeNameResolver instead of Compilation, and made it a field of ReflectionAccessAnalyzer. I didn't get rid of TryResolveTypeNameAndMark from ReflectionAccessAnalyzer, to keep the code structure similar to ReflectionMarker.

src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/AssemblyQualifiedNameDataflow.cs

- Remove unused using - Simplify string concatenation - Remove unnecessary partial - Pass around TypeNameResolver instead of Compilation

jakobbotsch · 2024-08-13T08:37:30Z

The repo no longer builds locally for me after this PR. I get

...
  LibraryImportGenerator -> C:\dev\dotnet\runtime4\artifacts\bin\LibraryImportGenerator\Release\netstandard2.0\Microsoft.Interop.LibraryImportGenerator.dll
  System.Private.CoreLib -> C:\dev\dotnet\runtime4\artifacts\bin\System.Private.CoreLib\ref\Checked\net9.0\System.Private.CoreLib.dll
CSC : error CS8032: An instance of analyzer ILLink.RoslynAnalyzer.RequiresAssemblyFilesAnalyzer cannot be created from C:\dev\dotnet\runtime4\artifacts\bin\ILLink.RoslynAnalyzer\Release\netstandard2.0\ILLink.RoslynAnal
yzer.dll : Could not load file or assembly 'System.Collections.Immutable, Version=9.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified.. [C:\dev\dotnet\runtime4\src\core
clr\System.Private.CoreLib\System.Private.CoreLib.csproj]
CSC : error CS8032: An instance of analyzer ILLink.RoslynAnalyzer.RequiresDynamicCodeAnalyzer cannot be created from C:\dev\dotnet\runtime4\artifacts\bin\ILLink.RoslynAnalyzer\Release\netstandard2.0\ILLink.RoslynAnalyz
er.dll : Could not load file or assembly 'System.Collections.Immutable, Version=9.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified.. [C:\dev\dotnet\runtime4\src\corecl
r\System.Private.CoreLib\System.Private.CoreLib.csproj]
CSC : error CS8032: An instance of analyzer ILLink.RoslynAnalyzer.COMAnalyzer cannot be created from C:\dev\dotnet\runtime4\artifacts\bin\ILLink.RoslynAnalyzer\Release\netstandard2.0\ILLink.RoslynAnalyzer.dll : Could n
ot load file or assembly 'System.Collections.Immutable, Version=9.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified.. [C:\dev\dotnet\runtime4\src\coreclr\System.Private
.CoreLib\System.Private.CoreLib.csproj]
CSC : error CS8032: An instance of analyzer ILLink.RoslynAnalyzer.DynamicallyAccessedMembersAnalyzer cannot be created from C:\dev\dotnet\runtime4\artifacts\bin\ILLink.RoslynAnalyzer\Release\netstandard2.0\ILLink.Rosly
nAnalyzer.dll : Could not load file or assembly 'System.Collections.Immutable, Version=9.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified.. [C:\dev\dotnet\runtime4\src
\coreclr\System.Private.CoreLib\System.Private.CoreLib.csproj]
CSC : error CS8032: An instance of analyzer ILLink.RoslynAnalyzer.RequiresUnreferencedCodeAnalyzer cannot be created from C:\dev\dotnet\runtime4\artifacts\bin\ILLink.RoslynAnalyzer\Release\netstandard2.0\ILLink.RoslynA
nalyzer.dll : Could not load file or assembly 'System.Collections.Immutable, Version=9.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified.. [C:\dev\dotnet\runtime4\src\c
oreclr\System.Private.CoreLib\System.Private.CoreLib.csproj]

Any idea why?

AlexRadch · 2024-08-13T13:32:21Z

@sbomer I can not build runtime after this PR #106321

ViktorHofer · 2024-08-13T13:49:18Z

src/tools/illink/src/ILLink.RoslynAnalyzer/ILLink.RoslynAnalyzer.csproj

@@ -28,6 +31,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)" />


This change broke the repo build. See #106321. I think this version of SRM is too new. cc @ericstj

Yeah, an analyzer should only be using the version of SRM that roslyn provides for it.

It could be possible for an analyzer to carry a newer version of SRM, but it would need to be copied/deployed with the analyzer (and have a different version than the one in roslyn) - and we'd need to make sure to never try to exchange SRM types with the compiler (I don't think any SRM types are part of roslyn's public API but I can't be certain).

It might be possible to have this work once we upgrade to preview7 SDK, but I think it might still be broken in the editor/design-time build if VS itself doesn't have the new SRM.

This reverts commit e5b1d02.

…#106343) This reverts commit e5b1d02.

…6209)" This reverts commit e5b1d02.

Adds tests from #106209. This includes test coverage for #95118 and #106214.

Implement type name resolution for ILLink analyzer

2fda35d

dotnet-issue-labeler bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Aug 9, 2024

dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Aug 9, 2024

dotnet-policy-service bot assigned sbomer Aug 9, 2024

sbomer added 5 commits August 9, 2024 15:43

Don't change partial method signature

4cb6f78

Don't keep members of pointer or byref element types

0e0f51d

Cleanup

2fd6de1

Revert "Don't keep members of pointer or byref element types"

1011166

This reverts commit 0e0f51d.

Update expected behavior, add issue link

9903960

sbomer requested a review from a team August 9, 2024 23:42

sbomer marked this pull request as ready for review August 9, 2024 23:42

sbomer requested a review from marek-safar as a code owner August 9, 2024 23:42

This was referenced Aug 10, 2024

Nuget: Central Directory corrupt dotnet/dnceng#3099

Open

WasmTestOnChrome timeouts in CI #105363

Closed

jtschuster approved these changes Aug 12, 2024

View reviewed changes

sbomer added 2 commits August 12, 2024 13:26

PR feedback

d987ba6

- Remove unused using - Simplify string concatenation - Remove unnecessary partial - Pass around TypeNameResolver instead of Compilation

Make it a field

a41635c

sbomer merged commit e5b1d02 into dotnet:main Aug 12, 2024
75 of 77 checks passed

AlexRadch mentioned this pull request Aug 13, 2024

Build FAILED after #106209 PR (Implement type name resolution for ILLink analyzer) #106321

Closed

ViktorHofer reviewed Aug 13, 2024

View reviewed changes

sbomer added a commit that referenced this pull request Aug 13, 2024

Revert "Implement type name resolution for ILLink analyzer (#106209)"

d97142b

This reverts commit e5b1d02.

sbomer mentioned this pull request Aug 13, 2024

Revert "Implement type name resolution for ILLink analyzer" #106343

Merged

sbomer added a commit that referenced this pull request Aug 13, 2024

Revert "Implement type name resolution for ILLink analyzer (#106209)" (…

5ee1b86

…#106343) This reverts commit e5b1d02.

jtschuster added a commit to jtschuster/runtime that referenced this pull request Aug 16, 2024

Revert "Implement type name resolution for ILLink analyzer (dotnet#10…

df04484

…6209)" This reverts commit e5b1d02.

This was referenced Aug 19, 2024

Trim analyzer: Implement type name parsing #95118

Open

Add tests for trim warnings for marking types by name #106651

Merged

sbomer added a commit that referenced this pull request Aug 20, 2024

Test warning behavior for marking types by name (#106651)

80f5d44

Adds tests from #106209. This includes test coverage for #95118 and #106214.

github-actions bot locked and limited conversation to collaborators Sep 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement type name resolution for ILLink analyzer #106209

Implement type name resolution for ILLink analyzer #106209

sbomer commented Aug 9, 2024

jtschuster left a comment

jtschuster Aug 12, 2024

sbomer Aug 12, 2024

jtschuster Aug 12, 2024

sbomer Aug 12, 2024

jakobbotsch commented Aug 13, 2024

AlexRadch commented Aug 13, 2024

ViktorHofer Aug 13, 2024 •

edited

Loading

ericstj Aug 13, 2024 •

edited

Loading

		readonly Location _location;
		readonly Action<Diagnostic>? _reportDiagnostic;

Implement type name resolution for ILLink analyzer #106209

Implement type name resolution for ILLink analyzer #106209

Conversation

sbomer commented Aug 9, 2024

jtschuster left a comment

Choose a reason for hiding this comment

jtschuster Aug 12, 2024

Choose a reason for hiding this comment

sbomer Aug 12, 2024

Choose a reason for hiding this comment

jtschuster Aug 12, 2024

Choose a reason for hiding this comment

sbomer Aug 12, 2024

Choose a reason for hiding this comment

jakobbotsch commented Aug 13, 2024

AlexRadch commented Aug 13, 2024

ViktorHofer Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

ericstj Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

ViktorHofer Aug 13, 2024 •

edited

Loading

ericstj Aug 13, 2024 •

edited

Loading