Define convention to consume and/or package analyzers #69069

ViktorHofer · 2022-05-09T19:19:43Z

Until now we required source libraries to define ProjectReferences when
an analyzer should be part of the shared framework. That strategy causes
analyzer projects to leak into the ProjectReference closure and by that
into a solution file.

As an example:
When another library references the source library that references the
analyzer, the analyzer is part of the dependency closure even though it
might not be required.

This change makes it possible to define the shared framework analyzer
projects in the NetCoreAppLibrary.props file for both the .NETCoreApp,
and the AspNetCoreApp shared framework.

Out-of-band projects which ship analyzers inside their produced package,
continue to reference the analyzers via the AnalyzerProject item.

ghost · 2022-05-09T19:19:53Z

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #61321

Until now we required source libraries to define ProjectReferences when
an analyzer should be part of the shared framework. That strategy causes
analyzer projects to leak into the ProjectReference closure and by that
into a solution file.

As an example:
When another library references the source library that references the
analyzer, the analyzer is part of the dependency closure even though it
might not be required.

This change makes it possible to define the shared framework analyzer
projects in the NetCoreAppLibrary.props file for both the .NETCoreApp,
and the AspNetCoreApp shared framework.

Out-of-band projects which ship analyzers inside their produced package,
continue to reference the analyzers via the AnalyzerProject item.

Author:	ViktorHofer
Assignees:	ViktorHofer
Labels:	`area-Infrastructure-libraries`
Milestone:	7.0.0

Fixes dotnet#61321 Until now we required source libraries to define ProjectReferences when an analyzer should be part of the shared framework. That strategy causes analyzer projects to leak into the ProjectReference closure and by that into a solution file. As an example: When another library references the source library that references the analyzer, the analyzer is part of the dependency closure even though it might not be required. This change makes it possible to define the shared framework analyzer projects in the NetCoreAppLibrary.props file for both the .NETCoreApp, and the AspNetCoreApp shared framework. Out-of-band projects which ship analyzers inside their produced package, continue to reference the analyzers via the `AnalyzerProject` item.

...crosoft.Extensions.Logging.Abstractions/src/Microsoft.Extensions.Logging.Abstractions.csproj

ericstj · 2022-05-11T21:31:06Z

src/libraries/Directory.Build.targets

-         so we don't apply TargetFramework filters nor do we pass in TargetFramework.
-         When BuildProjectReferences=false we make sure to set BuildReference=false to make
-         sure not to try to call GetTargetPath in the outerbuild of the analyzer project. -->
-    <ProjectReference Include="@(AnalyzerReference)"


Why isn't this needed anymore?

When you originally added this code, source generators unnecessarily multi-targeted (by using TargetFramework instead of TargetFrameworks). Since then, source generators were updated to not multi-target and we do have general protection that makes sure that they only target netstandard2.0. The TargetFramework.SDK was updated to support single tfms and same as in the SDK, drops the TargetFramework metadata from ProjectReference items (to not cause unnecessary evaluations).

Ok, I couldn't remember exactly why we added this and I tried to trace it back in history and couldn't see it. I trust your judgement. Maybe a few things to try for regression purposes:

Open the S.T.J sln and build it.

dotnet build --no-dependencies System.Text.Json.csproj

dotnet pack System.Text.Json.csproj

dotnet pack ---no-build System.Text.Json.csproj

ericstj · 2022-05-11T21:31:11Z

eng/generators.targets

-                                $(LibrariesProjectRoot)System.Runtime.InteropServices\gen\Microsoft.Interop.SourceGeneration\Microsoft.Interop.SourceGeneration.csproj"
-                      OutputItemType="Analyzer"
-                      ReferenceOutputAssembly="false" />
+    <AnalyzerReference Include="$(LibrariesProjectRoot)System.Runtime.InteropServices\gen\LibraryImportGenerator\LibraryImportGenerator.csproj;


Do we still need to do this in targets? Could we make libraries in the shared framework use AnalyzerReference just like they have to declare their reference to Interop and then have libraries outside the shared framework just get it (like customer projects will, now that its in the refpack)?

That's eventually the goal, yes. I just yesterday discussed this with @jkoritzinsky in the infrastructure stand-up. Anything that's OOB should get the analyzers via the targeting pack and anything inside the shared framework should explicitly reference the analyzers. Devil in the details: You still need an explicit analyzer reference when you build pre net7.0 (net6.0, netstandard2.0, net462).
Therefore, we will still need to reference the analyzer for anything that is not NetCoreAppCurrent. In addition to that, Visual Studio's CPS currently has a bug that causes ProjectReferences that aren't referenced by the "main" active configuration to not be built. Because of that, I would prefer to keep the explicit analyzers reference on NetCoreAppCurrent (OOB) for now.

Do we still need to do this in targets? Could we make libraries in the shared framework use AnalyzerReference just like they have to declare their reference to Interop

Just to check, you did see that this logic doesn't run inside a target, just in a targets file, right?

What you are proposing could definitely be done BUT the current logic in generators.targets already does that automatically when you either reference System.Runtime.InteropServices or System.Private.CoreLib. IMO, I would treat the LibraryImportGenerator source generator as a transitive analyzer dependency of InteropServices and CoreLib. The current logic already does that.

Just to check, you did see that this logic doesn't run inside a target, just in a targets file, right?

Yes. I was just thinking that if we made it easy enough for projects to opt in it might be better to have them explicitly opt into referencing the analyzer, just like they explicitly reference every other reference. It could avoid that magic/complexity in the condition, and it might reduce the number of places that actually reference the analyzer.

ericstj · 2022-05-11T21:39:11Z

src/libraries/sfx.proj

+  <!-- Support building only the reference assemblies. -->
+  <ItemGroup Condition="'$(RefOnly)' != 'true'">
+    <ProjectReference Include="sfx-src.proj" OutputItemType="SharedFrameworkAssembly" />
+    <ProjectReference Include="sfx-gen.proj" />


Can you double check that we still include the analyzers in the ref-pack after this change? Also the ASP.NET transport package.

Just verified this locally by building from a clean state and the analyzers are correctly placed into the targeting packs' and the packages' analyzers/dotnet/(cs)/ folder.

I opened #69233 though as the LibraryImportGenerator might only work for C# projects and is currently not marked as lanague specific.

ViktorHofer added the area-Infrastructure-libraries label May 9, 2022

ViktorHofer added this to the 7.0.0 milestone May 9, 2022

ViktorHofer requested review from ericstj and a team May 9, 2022 19:19

ViktorHofer self-assigned this May 9, 2022

ViktorHofer mentioned this pull request May 9, 2022

Source Generator projects don't need to be included in other library's sln files #58879

Closed

ViktorHofer force-pushed the DefineGeneratorProjectsConvention branch 2 times, most recently from d14a0fe to 08ec129 Compare May 10, 2022 08:54

ViktorHofer force-pushed the DefineGeneratorProjectsConvention branch from 08ec129 to a826907 Compare May 10, 2022 09:09

ViktorHofer mentioned this pull request May 10, 2022

AnalyzerReference vs ProjectReference #69111

Closed

ViktorHofer changed the title ~~Define convention to include analyzers in ref pack~~ Define convention to consume and/or package analyzers May 10, 2022

eerhardt reviewed May 10, 2022

View reviewed changes

...crosoft.Extensions.Logging.Abstractions/src/Microsoft.Extensions.Logging.Abstractions.csproj Show resolved Hide resolved

ViktorHofer added 5 commits May 11, 2022 13:46

Merge branch 'main' into DefineGeneratorProjectsConvention

d507eab

Use AnalyzerReference consistently

8fad32b

Don't reference analyzer when its packaged

5fdbe12

Fix P2P reference

2b026dd

Fix multi target roslyn component target condition

1771c6d

ericstj requested review from joperezr and elinor-fung May 11, 2022 21:24

ericstj approved these changes May 11, 2022

View reviewed changes

ViktorHofer merged commit d3af492 into dotnet:main May 12, 2022

ViktorHofer deleted the DefineGeneratorProjectsConvention branch May 12, 2022 07:22

ghost locked as resolved and limited conversation to collaborators Jun 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Define convention to consume and/or package analyzers #69069

Define convention to consume and/or package analyzers #69069

Uh oh!

ViktorHofer commented May 9, 2022 •

edited

Loading

Uh oh!

ghost commented May 9, 2022

Uh oh!

Uh oh!

ericstj May 11, 2022

Uh oh!

ViktorHofer May 12, 2022 •

edited

Loading

Uh oh!

ericstj May 12, 2022

Uh oh!

ericstj May 11, 2022

Uh oh!

ViktorHofer May 12, 2022 •

edited

Loading

Uh oh!

ViktorHofer May 12, 2022

Uh oh!

ericstj May 12, 2022

Uh oh!

ericstj May 11, 2022

Uh oh!

ViktorHofer May 12, 2022

Uh oh!

ViktorHofer May 12, 2022

Uh oh!

Uh oh!

Define convention to consume and/or package analyzers #69069

Define convention to consume and/or package analyzers #69069

Uh oh!

Conversation

ViktorHofer commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 9, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ViktorHofer May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ViktorHofer commented May 9, 2022 •

edited

Loading

ViktorHofer May 12, 2022 •

edited

Loading

ViktorHofer May 12, 2022 •

edited

Loading