Prepare GenAPI so it can be used for a new assembly diff generator #45389

carlossanlop · 2024-12-10T05:01:53Z

We intend to create a new tool to generate the monthly API diffs that will replace arcade's AsmDiff, which depends on Cci, an obsolete and unsupported library.

This PR makes the preliminary changes needed to be able to call GenAPI from yet another project besides GenAPI.Tool, GenAPI.Task and ApiCompat.

I made the following changes:

Added a new class, GenApiAppConfiguration, that can be used to store the settings for GenAPI.
- Make it an option to load the assemblies either from the filesystem or from streams representing the assembly code as string. Same for the assembly references.
- Make it an option to set the header either from a file or from a string.
- Move into this class the job of collecting the DocId files (either for API or attribute exclusion) from the filesystem. This way, the user can create a configuration instance passing a collection of DocID strings directly, not from disk.
- Move into this class the code that configures the assemblies and the loader. It was repeated in GenApiApp and in the tests.
- Use this class to allow making the output either the filesystem or streams for later usage.

The diffing tool will mainly need the last point: being able to have the strings stored in streams to be able to do the comparisons afterwards.

src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Filtering/DocIdSymbolFilter.cs

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/CSharpFileBuilder.cs

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/GenAPIConfiguration.cs

src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/AssemblySymbolLoader.cs

src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Filtering/DocIdSymbolFilter.cs

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/CSharpFileBuilder.cs

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/GenAPIConfiguration.cs

ViktorHofer · 2024-12-17T22:07:09Z

test/Microsoft.DotNet.ApiCompatibility.Tests/Microsoft.DotNet.ApiCompatibility.Tests.csproj

@@ -16,8 +16,9 @@
  </ItemGroup>

  <ItemGroup>
-    <ProjectReference Include="$(RepoRoot)src\Compatibility\ApiCompat\Microsoft.DotNet.ApiCompatibility\Microsoft.DotNet.ApiCompatibility.csproj" />
+    <ProjectReference Include="$(RepoRoot)src\Compatibility\GenAPI\Microsoft.DotNet.GenAPI\Microsoft.DotNet.GenAPI.csproj" />


revert:

Suggested change

<ProjectReference Include="$(RepoRoot)src\Compatibility\GenAPI\Microsoft.DotNet.GenAPI\Microsoft.DotNet.GenAPI.csproj" />

You found it! This is why I couldn't figure out where all the ApiCompat failures were coming from in the tests. It got lost in the weeds.

Looking at the current source, I still see these two P2Ps which are wrong.

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI.Task/GenAPITask.cs

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/CSharpFileBuilder.cs

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/GenAPIApp.cs

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/AssemblyLoaderFactory.cs

ericstj

A couple small comments, would like for @ViktorHofer to weigh in on style / architecture before going deeper.

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/AssemblyLoaderFactory.cs

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/CSharpAssemblyVisitor.cs

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/SymbolFilterFactory.cs

ViktorHofer · 2025-01-07T20:17:32Z

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI.Task/GenAPITask.cs

+            (IAssemblySymbolLoader loader, Dictionary<string, IAssemblySymbol> assemblySymbols) = AssemblyLoaderFactory.CreateFromFiles(
+                assembliesPaths: Assemblies ?? throw new NullReferenceException("Assemblies cannot be null."),
+                assemblyReferencesPaths: AssemblyReferences,
+                RespectInternals);


One of the main design principles in the Compatibility tools is to keep the UI layer as thin as possible and let the business logic layer handle with the start-up and shutdown. The existing code base does this well by parsing arguments and calling into GenAPIApp. The UI stack explicitly doesn't use lower level types like from Microsoft.CodeAnalysis or ApiSymbolExtensions.

...ibility/GenAPI/Microsoft.DotNet.GenAPI/SyntaxRewriter/TypeDeclarationCSharpSyntaxRewriter.cs

src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/AssemblySymbolLoader.cs

…stead of the whole config. Rename the config instance variables to something more meaningful instead of c or _c. Fix the build failure happening under #if NET.

…ssembly name.

…, split it into two separate classes.

…harpSyntaxRewriter

ericstj · 2025-01-07T22:56:06Z

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/CSharpAssemblyVisitor.cs

+    }
+
+    /// <inheritdoc />
+    public SyntaxNode GetFormattedRootNodeForDocument(Document document) => document.GetSyntaxRootAsync().Result!.Rewrite(new SingleLineStatementCSharpSyntaxRewriter());


Does this actually use any instance state, or is it just a transformation on Document? Why does this live on the assembly visitor?

I renamed the class as suggested so the purpose of this method made more sense.

Just tossing the idea out there - now that you have both CSharpAssemblyDocumentGenerator and CSharpFileBuilder do you think it makes sense to have these two different types? Could we combine them back together and just have one that is used slightly differently depending on the caller? It might reduce the total number of names we have to come up with while still being meaningful and keeping the diff smaller.

ericstj · 2025-01-07T22:58:26Z

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/IAssemblyVisitor.cs

+
+namespace Microsoft.DotNet.GenAPI;
+
+public interface IAssemblyVisitor


Can you document this, and mention its purpose? Is it for testability - and if so, do you use that anywhere? For that matter I also see that IAssemblySymbolWriter is an interface only ever used with a single type. @ViktorHofer do you think there is value in keeping these interfaces?

Correct me if I'm wrong, but we use interfaces in case we one day decide to provide similar support to Visual Basic code, no?

I'm not certain, but if we did add VB support we could add the interface at that time? I tried to look back at the history and it was there from the start. I couldn't see anywhere we actually made use of it.

I 10000 also see that IAssemblySymbolWriter is an interface only ever used with a single type. @ViktorHofer do you think there is value in keeping these interfaces?

Yes, the general idea for IAssemblySymbolWriter was to not depend on the specific implementation in the unit of code that depends on it (basically follow the Liskov Substitution Principle). Looks like GenAPIApp depends on the concrete type though so the interface doesn't serve much purpose today. There's probably room for improvement here and I would keep the interface.

…mblyVisitor class, remove its interface. Move the static methods from SymbolFilterFactory to CompositeSymbolFilter, and delete SymbolFilterFactory. Move TryGetRecordConstructor static extension method to a higher assembly. Move the NullableAttributes.cs dependency to the same higher assembly. Move ImplicitSymbolFilter to a higher assembly. Adjust the tests accordingly.

ViktorHofer · 2025-01-08T14:59:59Z

src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Filtering/CompositeSymbolFilter.cs

+                                                                     bool includeEffectivelyPrivateSymbols = true,
+                                                                     bool includeExplicitInterfaceImplementationSymbols = true)
+        {
+            DocIdSymbolFilter? docIdSymbolFilter =


The CompositeSymbolFilter is one implementation of the ISymbolFilter interface and must not know about others, i.e. the DocIdSymbolFilter. Even if this these methods are static, they don't belong here. That would violate the separation of concerns.

I'll bring back the factory class. If there are methods that belong to a specific class, I'll put them there instead.

ViktorHofer · 2025-01-08T15:04:36Z

src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/AssemblySymbolLoaderFactory.cs

    /// <param name="includeInternalSymbols">True to include internal API when reading assemblies from the <see cref="AssemblySymbolLoader"/> created.</param>
-    public sealed class AssemblySymbolLoaderFactory(bool includeInternalSymbols = false) : IAssemblySymbolLoaderFactory
+    public sealed class AssemblySymbolLoaderFactory(ILog logger, bool includeInternalSymbols = false) : IAssemblySymbolLoaderFactory


I think we name the field log everywhere else, i.e.:

sdk/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompatibility/Rules/AssemblyIdentityMustMatch.cs

Line 21 in cc260aa

private readonly ISuppressibleLog _log;

ViktorHofer

This PR is getting quite big and complex. I think it would help if you would submit some of the non-controversial changes as one separate PR. Examples to include:

AssemblySymbolLoader receiving the log instance
The ctor change in DocIdSymbolFilter
and the whitespace removals and typo fixes

carlossanlop · 2025-01-10T19:16:41Z

I'll submit smaller PRs as this one is already full of merge conflicts.

carlossanlop added Team:Runtime Area-Tools labels Dec 10, 2024

carlossanlop requested review from ViktorHofer and ericstj December 10, 2024 05:01

carlossanlop self-assigned this Dec 10, 2024

carlossanlop requested a review from a team as a code owner December 10, 2024 05:01

ghost added the untriaged Request triage from a team member label Dec 10, 2024

ViktorHofer reviewed Dec 10, 2024

View reviewed changes

src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Filtering/DocIdSymbolFilter.cs Outdated Show resolved Hide resolved

ViktorHofer reviewed Dec 10, 2024

View reviewed changes

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/CSharpFileBuilder.cs Outdated Show resolved Hide resolved

ViktorHofer reviewed Dec 10, 2024

View reviewed changes

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/CSharpFileBuilder.cs Outdated Show resolved Hide resolved

ericstj reviewed Dec 10, 2024

View reviewed changes

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/GenAPIConfiguration.cs Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Dec 10, 2024

Failure on Helix tests - Tests timing out #40074

Open

carlossanlop commented Dec 11, 2024

View reviewed changes

src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/AssemblySymbolLoader.cs Outdated Show resolved Hide resolved

ViktorHofer reviewed Dec 12, 2024

View reviewed changes

src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Filtering/DocIdSymbolFilter.cs Outdated Show resolved Hide resolved

ViktorHofer reviewed Dec 12, 2024

View reviewed changes

src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Filtering/DocIdSymbolFilter.cs Outdated Show resolved Hide resolved

carlossanlop commented Dec 17, 2024

View reviewed changes

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/CSharpFileBuilder.cs Show resolved Hide resolved

build-analysis bot mentioned this pull request Dec 17, 2024

Template engine tests hitting known parallelism issue in GetVisualStudioInstances #44878

Open

carlossanlop commented Dec 17, 2024

View reviewed changes

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/GenAPIConfiguration.cs Outdated Show resolved Hide resolved

ViktorHofer reviewed Dec 17, 2024

View reviewed changes

carlossanlop mentioned this pull request Dec 18, 2024

[DRAFT] Implement ApiDiff on top of the refactored GenAPI code carlossanlop/sdk#1

Closed

carlossanlop commented Dec 18, 2024

View reviewed changes

ericstj reviewed Dec 18, 2024

View reviewed changes

src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/AssemblyLoaderFactory.cs Outdated Show resolved Hide resolved

ericstj reviewed Dec 18, 2024

View reviewed changes

carlossanlop mentioned this pull request Dec 18, 2024

Asm diff updates dotnet/arcade#13641

Closed

carlossanlop removed the untriaged Request triage from a team member label Jan 6, 2025

carlossanlop force-pushed the GenApiRefactor branch from 39ce146 to d8a3226 Compare January 6, 2025 23:20

This was referenced Jan 7, 2025

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

ViktorHofer reviewed Jan 7, 2025

View reviewed changes

...ibility/GenAPI/Microsoft.DotNet.GenAPI/SyntaxRewriter/TypeDeclarationCSharpSyntaxRewriter.cs Outdated Show resolved Hide resolved

ViktorHofer reviewed Jan 7, 2025

View reviewed changes

src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/AssemblySymbolLoader.cs Outdated Show resolved Hide resolved

carlossanlop added 10 commits January 7, 2025 14:10

Prepare GenAPI so it can be used for a new assembly diff generator.

8f1432e

Pass to CSharpFileBuilder constructor only the necessary arguments in…

bc754a6

…stead of the whole config. Rename the config instance variables to something more meaningful instead of c or _c. Fix the build failure happening under #if NET.

Rename DocIdSymbolFilter.GetFilterForDocIds to DocIdSymbolFilter.Create.

9c60ce5

Load assemblies as dictionary instead of list, where the key is the a…

d8190fa

…ssembly name.

Simplify GenAPIConfiguration

1286471

Use factory methods instead of fluent. Get rid of GenAPIConfiguration…

3666a3a

…, split it into two separate classes.

Address feedback and make some other improvements.

a3a2c70

Remove unnecessary factory class and pass logger where it was missing.

2006571

Merge CompositeSymbolFilter changes with main

a42e093

Do not set addPartialModifier to true by default in TypeDeclarationCS…

b385c9c

…harpSyntaxRewriter

ericstj reviewed Jan 7, 2025

View reviewed changes

carlossanlop force-pushed the GenApiRefactor branch from d8a3226 to a7358fc Compare January 8, 2025 01:00

ViktorHofer reviewed Jan 8, 2025

View reviewed changes

carlossanlop closed this Jan 10, 2025

carlossanlop mentioned this pull request Jan 13, 2025

ApiDiff: Add AssemblySymbolLoader method, a package dependency from CodeAnalysis, and minor fixes #45933

Closed

This was referenced Jan 31, 2025

[ApiDiff] Allow passing diagnostic options to all Compat places that use CSharpCompilationOptions #46424

Merged

[ApiDiff] Implementation of the new ApiDiff that reuses GenAPI features #46425

Merged


		namespace Microsoft.DotNet.GenAPI;

		public interface IAssemblyVisitor

Prepare GenAPI so it can be used for a new assembly diff generator #45389

Prepare GenAPI so it can be used for a new assembly diff generator #45389

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!