Skip to content

Define convention to consume and/or package analyzers #69069

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions docs/coding-guidelines/libraries-packaging.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ In some occasions we may want to include a library in the shared framework, but

Libraries included in the shared framework should ensure all direct and transitive assembly references are also included in the shared framework. This will be validated as part of the build and errors raised if any dependencies are unsatisfied.

Source generators and analyzers can be included in the shared framework by specifying `IsNetCoreAppAnalyzer`. These projects should specify `AnalyzerLanguage` as mentioned [below](#analyzers--source-generators).
Source generators and analyzers can be included in the shared framework by adding their project name into the NetCoreAppLibrary.props file under the `NetCoreAppLibraryGenerator` section. These projects should specify `AnalyzerLanguage` as mentioned [below](#analyzers--source-generators).

Removing a library from the shared framework is a breaking change and should be avoided.

Expand Down Expand Up @@ -70,10 +70,10 @@ Build props and targets may be needed in NuGet packages. To define these, author

Some packages may wish to include a companion analyzer or source-generator with their library. Analyzers are much different from normal library contributors: their dependencies shouldn't be treated as nuget package dependencies, their TargetFramework isn't applicable to the project they are consumed in (since they run in the compiler). To facilitate this, we've defined some common infrastructure for packaging Analyzers.

To include an analyzer in a package, simply add an `AnalyzerReference` item to the project that produces the package that should contain the analyzer
To include an analyzer in a package, simply add an `AnalyzerReference` item to the project that produces the package that should contain the analyzer and set the `Pack` metadata to true. If you just want to include the analyzer but not consume it, set the `ReferenceAnalyzer` metadata to false.
```xml
<ItemGroup>
<AnalyzerReference Include="..\gen\System.Banana.Generators.csproj" />
<AnalyzerReference Include="..\gen\System.Banana.Generators.csproj" Pack="true" ReferenceAnalyzer="false" />
</ItemGroup>
```

Expand Down
1 change: 1 addition & 0 deletions docs/coding-guidelines/project-guidelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ All test outputs should be under

## gen
In the gen directory any source generator related to the assembly should exist. This does not mean the source generator is only used for that assembly only that it is conceptually apart of that assembly. For example, the assembly may provide attributes or low-level types the source generator uses.
To consume a source generator, simply add an `<AnalyzerReference Include="..." />` item to the project, usually next to the `References` and `ProjectReferences` items.

## Facades
Facade are unique in that they don't have any code and instead are generated by finding a contract reference assembly with the matching identity and generating type forwards for all the types to where they live in the implementation assemblies (aka facade seeds). There are also partial facades which contain some type forwards as well as some code definitions. All the various build configurations should be contained in the one csproj file per library.
Expand Down
17 changes: 9 additions & 8 deletions eng/generators.targets
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,18 @@
That is required as the EnabledGenerators condition checks on the Reference and ProjectReference items and hence can't be a property condition. -->
<ItemGroup Condition="'@(EnabledGenerators)' != '' and
@(EnabledGenerators->AnyHaveMetadataValue('Identity', 'LibraryImportGenerator'))">
<ProjectReference Include="$(LibrariesProjectRoot)System.Runtime.InteropServices\gen\LibraryImportGenerator\LibraryImportGenerator.csproj;
$(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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member Author

@ViktorHofer ViktorHofer May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$(LibrariesProjectRoot)System.Runtime.InteropServices\gen\Microsoft.Interop.SourceGeneration\Microsoft.Interop.SourceGeneration.csproj" />
</ItemGroup>

<!-- Use a normal property condition as this source generator is opt-in and doesn't read from an item list. -->
<ItemGroup Condition="'$(EnableRegexGenerator)' == 'true'">
<ProjectReference Include="$(LibrariesProjectRoot)System.Text.RegularExpressions\gen\System.Text.RegularExpressions.Generator.csproj"
<!-- AnalyzerReference items are transformed to ProjectReferences with the required analyzer metadata. -->
<ItemGroup>
<ProjectReference Include="@(AnalyzerReference)"
ReferenceOutputAssembly="false"
OutputItemType="Analyzer"
ReferenceOutputAssembly="false" />
Pack="false" />
<ProjectReference Update="@(AnalyzerReference->WithMetadataValue('ReferenceAnalyzer', 'false'))"
OutputItemType="" />
</ItemGroup>

<Target Name="ConfigureGenerators"
Expand Down
15 changes: 9 additions & 6 deletions eng/packaging.targets
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,12 @@
</ItemGroup>
</Target>

<Target Name="IncludeAnalyzersInPackage" Condition="'@(AnalyzerReference)' != ''">
<!-- Call a target in the analyzer project to get all the files it would normally place in a package.
These will be returned as items with identity pointing to the built file, and PackagePath metadata
set to their location in the package. IsSymbol metadata will be set to distinguish symbols. -->
<MSBuild Projects="@(AnalyzerReference)"
<!-- Call a target in the analyzer project to get all the files it would normally place in a package.
These will be returned as items with identity pointing to the built file, and PackagePath metadata
set to their location in the package. IsSymbol metadata will be set to distinguish symbols. -->
<Target Name="IncludeAnalyzersInPackage"
Condition="'@(AnalyzerReference)' != '' and @(AnalyzerReference->AnyHaveMetadataValue('Pack', 'true'))">
<MSBuild Projects="@(AnalyzerReference->WithMetadataValue('Pack', 'true'))"
Targets="GetAnalyzerPackFiles">
<Output TaskParameter="TargetOutputs" ItemName="_AnalyzerFile" />
</MSBuild>
Expand All @@ -165,7 +166,9 @@
<!-- In packages that contain Analyzers, include a .targets file that will select the correct analyzer. -->
<Target Name="IncludeMultiTargetRoslynComponentTargetsInPackage"
AfterTargets="IncludeAnalyzersInPackage"
Condition="'@(AnalyzerReference)' != '' and '$(IncludeMultiTargetRoslynComponentTargets)' == 'true'"
Condition="'@(AnalyzerReference)' != '' and
@(AnalyzerReference->AnyHaveMetadataValue('Pack', 'true')) and
'$(IncludeMultiTargetRoslynComponentTargets)' == 'true'"
DependsOnTargets="GenerateMultiTargetRoslynComponentTargetsFile">
<ItemGroup>
<Content Include="$(MultiTargetRoslynComponentTargetsFileIntermediatePath)" PackagePath="buildTransitive\netstandard2.0\$(PackageId).targets" />
Expand Down
23 changes: 4 additions & 19 deletions src/libraries/Directory.Build.targets
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@
<IsNETCoreAppRef Condition="('$(IsReferenceAssemblyProject)' == 'true' or '$(IsRuntimeAndReferenceAssembly)' == 'true') and
$(NetCoreAppLibrary.Contains('$(AssemblyName);')) and
!$(NetCoreAppLibraryNoReference.Contains('$(AssemblyName);'))">true</IsNETCoreAppRef>
<IsNETCoreAppAnalyzer Condition="'$(IsGeneratorProject)' == 'true' and
$(NetCoreAppLibraryGenerator.Contains('$(MSBuildProjectName);'))">true</IsNETCoreAppAnalyzer>
<!-- By default, disable implicit framework references for NetCoreAppCurrent libraries. -->
<DisableImplicitFrameworkReferences Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and
$([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), '$(NETCoreAppCurrentVersion)')) and
Expand Down Expand Up @@ -201,25 +203,6 @@
</When>
</Choose>

<PropertyGroup>
<BuildAnalyzerReferences>$(BuildProjectReferences)</BuildAnalyzerReferences>
<BuildAnalyzerReferences Condition="'$(BuildingInsideVisualStudio)' == 'true'">false</BuildAnalyzerReferences>
</PropertyGroup>

<ItemGroup>
<!-- Ensure AnalyzerReference items are restored and built
The target framework of Analyzers has no relationship to that of the refrencing project,
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)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this needed anymore?

Copy link
Member Author

@ViktorHofer ViktorHofer May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Open the S.T.J sln and build it.
  2. dotnet build --no-dependencies System.Text.Json.csproj
  3. dotnet pack System.Text.Json.csproj
  4. dotnet pack ---no-build System.Text.Json.csproj

SkipGetTargetFrameworkProperties="true"
UndefineProperties="TargetFramework"
ReferenceOutputAssembly="false"
PrivateAssets="all"
BuildReference="$(BuildAnalyzerReferences)" />
</ItemGroup>

<Target Name="GetAnalyzerPackFiles"
DependsOnTargets="$(GenerateNuspecDependsOn)"
Returns="@(_AnalyzerPackFile)">
Expand All @@ -228,11 +211,13 @@
<_analyzerPath Condition="'$(AnalyzerRoslynVersion)' != ''">$(_analyzerPath)/roslyn$(AnalyzerRoslynVersion)</_analyzerPath>
<_analyzerPath Condition="'$(AnalyzerLanguage)' != ''">$(_analyzerPath)/$(AnalyzerLanguage)</_analyzerPath>
</PropertyGroup>

<ItemGroup>
<_AnalyzerPackFile Include="@(_BuildOutputInPackage)" IsSymbol="false" />
<_AnalyzerPackFile Include="@(_TargetPathsToSymbols)" IsSymbol="true" />
<_AnalyzerPackFile PackagePath="$(_analyzerPath)/%(TargetPath)" />
</ItemGroup>

<Error Condition="'%(_AnalyzerPackFile.TargetFramework)' != 'netstandard2.0'"
Text="Analyzers must only target netstandard2.0 since they run in the compiler which targets netstandard2.0. The following files were found to target '%(_AnalyzerPackFile.TargetFramework)': @(_AnalyzerPackFile)" />
</Target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,12 @@ Microsoft.Extensions.Logging.Abstractions.NullLogger</PackageDescription>

<ItemGroup>
<AnalyzerReference Include="..\gen\Microsoft.Extensions.Logging.Generators.Roslyn3.11.csproj"
Pack="true"
ReferenceAnalyzer="false"
Condition="'$(DotNetBuildFromSource)' != 'true'" />
<AnalyzerReference Include="..\gen\Microsoft.Extensions.Logging.Generators.Roslyn4.0.csproj" />
<AnalyzerReference Include="..\gen\Microsoft.Extensions.Logging.Generators.Roslyn4.0.csproj"
Pack="true"
ReferenceAnalyzer="false" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,14 @@
<ItemGroup>
<!-- Requires Private=true to calculate ReferenceCopyLocalPaths items. Also share System.Net.Quic which isn't part of aspnetcore's shared framework but which is needed by them. -->
<ProjectReference Include="@(AspNetCoreAppLibrary->'$(LibrariesProjectRoot)%(Identity)\src\%(Identity).csproj');
$(LibrariesProjectRoot)System.Net.Quic\src\System.Net.Quic.csproj" PrivateAssets="all" Pack="true" Private="true" IncludeReferenceAssemblyInPackage="true" />
<!-- TODO: Find a better way to include source generators without hardcoding them. -->
$(LibrariesProjectRoot)System.Net.Quic\src\System.Net.Quic.csproj"
Pack="true"
PrivateAssets="all"
Private="true"
IncludeReferenceAssemblyInPackage="true" />
<!-- Only include the 4.0 version in the ref pack, since targeting net6.0 requires Roslyn 4.0 -->
<AnalyzerReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Logging.Abstractions\gen\Microsoft.Extensions.Logging.Generators.Roslyn4.0.csproj" />
<AnalyzerReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Logging.Abstractions\gen\Microsoft.Extensions.Logging.Generators.Roslyn4.0.csproj"
Pack="true"
ReferenceAnalyzer="false" />
</ItemGroup>
</Project>
7 changes: 7 additions & 0 deletions src/libraries/NetCoreAppLibrary.props
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,12 @@
System.Private.Xml;
System.Private.Xml.Linq;
</NetCoreAppLibraryNoReference>
<!-- List .NETCoreApp shared framework generator project names below. -->
<NetCoreAppLibraryGenerator>
LibraryImportGenerator;
System.Text.Json.SourceGeneration.Roslyn4.0;
System.Text.RegularExpressions.Generator;
</NetCoreAppLibraryGenerator>
<AspNetCoreAppLibrary>
Microsoft.Extensions.Caching.Abstractions;
Microsoft.Extensions.Caching.Memory;
Expand Down Expand Up @@ -245,6 +251,7 @@
<NetFxReference Include="$(NetFxReference)" />
<NetCoreAppLibrary Include="$(NetCoreAppLibrary)" />
<NetCoreAppLibraryNoReference Include="$(NetCoreAppLibraryNoReference)" />
<NetCoreAppLibraryGenerator Include="$(NetCoreAppLibraryGenerator)" />
<AspNetCoreAppLibrary Include="$(AspNetCoreAppLibrary)" />
<WindowsDesktopCoreAppLibrary Include="$(WindowsDesktopCoreAppLibrary)" />
</ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>$(NetCoreAppCurrent)</TargetFramework>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<IsPartialFacadeAssembly>true</IsPartialFacadeAssembly>
<TargetFramework>$(NetCoreAppCurrent)</TargetFramework>
<EnableRegexGenerator>true</EnableRegexGenerator>
</PropertyGroup>

<ItemGroup>
<ILLinkSubstitutionsXmls Include="$(ILLinkDirectory)ILLink.Substitutions.xml" />
</ItemGroup>

<ItemGroup>
<Compile Include="MS\Internal\Xml\Linq\ComponentModel\XComponentModel.cs" />
<Compile Include="System\ComponentModel\ArrayConverter.cs" />
Expand Down Expand Up @@ -238,7 +239,9 @@
<Compile Include="System\ComponentModel\ComponentResourceManager.cs" />
<Compile Include="System\Security\Authentication\ExtendedProtection\ExtendedProtectionPolicyTypeConverter.cs" />
</ItemGroup>

<ItemGroup>
<AnalyzerReference Include="$(LibrariesProjectRoot)System.Text.RegularExpressions\gen\System.Text.RegularExpressions.Generator.csproj" />
<Reference Include="System.Collections" />
<Reference Include="System.Collections.NonGeneric" />
<Reference Include="System.Collections.Specialized" />
Expand Down
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>$(NetCoreAppCurrent)</TargetFramework>
<NoWarn>$(NoWarn);1634;1691;649</NoWarn>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<TargetFramework>$(NetCoreAppCurrent)</TargetFramework>
<!-- Too much private reflection. Do not bother with trimming -->
<ILLinkTrimAssembly>false</ILLinkTrimAssembly>
<EnableRegexGenerator>true</EnableRegexGenerator>
</PropertyGroup>

<PropertyGroup>
<RuntimeSerializationSources>System\Runtime\Serialization</RuntimeSerializationSources>
<JsonSources>System\Runtime\Serialization\Json</JsonSources>
<XmlSources>System\Xml</XmlSources>
<TextSources>System\Text</TextSources>
</PropertyGroup>

<ItemGroup>
<Compile Include="$(CommonPath)System\NotImplemented.cs"
Link="Common\System\NotImplemented.cs" />
Expand Down Expand Up @@ -149,7 +150,9 @@
<Compile Include="System\Xml\XmlCanonicalWriter.cs" />
<Compile Include="System\Xml\XmlSigningNodeWriter.cs" />
</ItemGroup>

<ItemGroup>
<AnalyzerReference Include="$(LibrariesProjectRoot)System.Text.RegularExpressions\gen\System.Text.RegularExpressions.Generator.csproj" />
<Reference Include="System.Collections" />
<Reference Include="System.Collections.Concurrent" />
<Reference Include="System.Collections.NonGeneric" />
Expand Down
Loading