search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.92k stars 4.64k forks source link

Regex generator doesn't work when using statements only come from other generators #89923

Closed Emik03 closed 1 year ago

Emik03 commented 1 year ago

Description

System.Text.RegularExpressions.Generator fails to recognize all GeneratedRegexAttribute annotations when the import System.Text.RegularExpressions is done by source generation. (see reproduction steps)

This bug was discovered because the source generator had stopped working for me, and I couldn't find any resource about it not working. This was due to the fact that I was the only one to import Emik.SourceGenerators.Tattoo. This imports every namespace, which includes System.Text.RegularExpressions.

Reproduction Steps

Create files as specified by each header, and copy the contents of the code blocks into said files.

You may follow either the instructions for with or without. The one with the dependency is a more minimal example, but the other one proves that it's not my analyzer's fault.

With Emik.SourceGenerators.Tattoo (shorter)

./SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj

<Project Sdk="Microsoft.NET.Sdk">
    <PropertyGroup>
        <TargetFramework>net7.0</TargetFramework>
    </PropertyGroup>
    <ItemGroup>
        <PackageReference Include="Emik.SourceGenerators.Tattoo" Version="1.0.1.1"/>
    </ItemGroup>
</Project>

./SanityCheckSourceGenerators.Sample/A.cs

partial class A
{
    [GeneratedRegex("Test")]
    public static partial Regex Generated();
}

Execute dotnet build on SanityCheckSourceGenerators.Sample, or compile it in your IDE of choice.

Without Emik.SourceGenerators.Tattoo (much longer; without magic dependency)

./SanityCheckSourceGenerators/SanityCheckSourceGenerators.csproj

<Project Sdk="Microsoft.NET.Sdk">
    <PropertyGroup>
        <LangVersion>latest</LangVersion>
        <IsRoslynComponent>true</IsRoslynComponent>
        <TargetFramework>netstandard2.0</TargetFramework>
        <EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules>
    </PropertyGroup>
    <ItemGroup>
        <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.6.0"/>
        <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.6.0"/>
        <PackageReference Include="Microsoft.CodeAnalysis.Analyzers" Version="3.3.4" PrivateAssets="all"/>
    </ItemGroup>
</Project>

./SanityCheckSourceGenerators/Generator.cs

using Microsoft.CodeAnalysis;

namespace SanityCheckSourceGenerators;

[Generator]
public class Generator : ISourceGenerator
{
    public void Initialize(GeneratorInitializationContext context) { }

    public void Execute(GeneratorExecutionContext context) =>
        context.AddSource("Test", "global using System.Text.RegularExpressions;");
}

./SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj

<Project Sdk="Microsoft.NET.Sdk">
    <PropertyGroup>
        <TargetFramework>net7.0</TargetFramework>
    </PropertyGroup>
    <ItemGroup>
        <ProjectReference Include="../SanityCheckSourceGenerators/SanityCheckSourceGenerators.csproj" OutputItemType="Analyzer" ReferenceOutputAssembly="false"/>
    </ItemGroup>
</Project>

./SanityCheckSourceGenerators.Sample/A.cs

partial class A
{
    [GeneratedRegex("Test")]
    public static partial Regex Generated();
}

Execute dotnet build on SanityCheckSourceGenerators.Sample, or compile it in your IDE of choice.

Expected behavior

Source-generates the method, and therefore compiles, as this log demonstrates.

MSBuild version 17.6.8+c70978d4d for .NET
  Determining projects to restore...
  Restored /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj (in 82 ms).
  Restored /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators/SanityCheckSourceGenerators.csproj (in 169 ms).
  SanityCheckSourceGenerators -> /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators/bin/Debug/netstandard2.0/SanityCheckSourceGenerators.dll
  SanityCheckSourceGenerators.Sample -> /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/bin/Debug/net7.0/SanityCheckSourceGenerators.Sample.dll

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:02.91

Actual behavior

Doesn't source-generate the method, and therefore fails to compile since the method is no longer implemented.

SDK 7.0.304

emik@spicy-linux:~/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample$ dotnet build
MSBuild version 17.6.8+c70978d4d for .NET
  Determining projects to restore...
  All projects are up-to-date for restore.
  SanityCheckSourceGenerators -> /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators/bin/Debug/netstandard2.0/SanityCheckSourceGenerators.dll
/home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/Examples.cs(5,33): error CS8795: Partial method 'A.Generated()' must have an implementation part because it has accessibility modifiers. [/home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj]

Build FAILED.

/home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/Examples.cs(5,33): error CS8795: Partial method 'A.Generated()' must have an implementation part because it has accessibility modifiers. [/home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj]
    0 Warning(s)
    1 Error(s)

Time Elapsed 00:00:01.16

SDK 8.0.100-preview.6.23330.14

emik@spicy-linux:~/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample$ dotnet build
MSBuild version 17.7.0+5785ed5c2 for .NET
  Determining projects to restore...
  All projects are up-to-date for restore.
/home/emik/.dotnet/sdk/8.0.100-preview.6.23330.14/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.RuntimeIdentifierInference.targets(314,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy [/home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj]
  SanityCheckSourceGenerators -> /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators/bin/Debug/netstandard2.0/SanityCheckSourceGenerators.dll
/home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/Examples.cs(5,33): error CS8795: Partial method 'A.Generated()' must have an implementation part because it has accessibility modifiers. [/home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj]

Build FAILED.

/home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/Examples.cs(5,33): error CS8795: Partial method 'A.Generated()' must have an implementation part because it has accessibility modifiers. [/home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj]
    0 Warning(s)
    1 Error(s)

Regression?

Unlikely. The source generator was introduced in .NET 7, and I tested it both on 7.0.306 and 8.0.100-preview.6.23330.14. (the latest SDKs for both versions at the time of writing this)

Known Workarounds

Write your own global using explicitly:

./SanityCheckSourceGenerators.Sample/GlobalUsings.cs

global using System.Text.RegularExpressions;

Now it recognizes the method. When executing dotnet build, the build finally passes.

A real-world example of a commit that implements this is a project that I maintain, which can be seen here.

Configuration

I am pretty confident this affects all environments, however I have yet to actually confirm this.

Other information

No response

ghost commented 1 year ago

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

Issue Details
### Description [`System.Text.RegularExpressions.Generator`](https://github.com/dotnet/runtime/tree/main/src/libraries/System.Text.RegularExpressions/gen) fails to recognize all [`GeneratedRegexAttribute`](https://learn.microsoft.com/en-us/dotnet/api/system.text.regularexpressions.generatedregexattribute?view=net-7.0) annotations when the import `System.Text.RegularExpressions` is done by source generation. (see reproduction steps) This bug was discovered because the source generator had stopped working for me, and I couldn't find any resource about it not working. This was due to the fact that I was the only one to import [`Emik.SourceGenerators.Tattoo`](https://www.nuget.org/packages/Emik.SourceGenerators.Tattoo). This imports every namespace, which includes `System.Text.RegularExpressions`. ### Reproduction Steps **Create files as specified by each header, and copy the contents of the code blocks into said files.** You may follow either the instructions for with or without. The one with the dependency is a more minimal example, but the other one proves that it's not my analyzer's fault. --- ## With [`Emik.SourceGenerators.Tattoo`](https://www.nuget.org/packages/Emik.SourceGenerators.Tattoo) (shorter) #### `./SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj` ```xml net7.0 ``` #### `./SanityCheckSourceGenerators.Sample/A.cs` ```cs partial class A { [GeneratedRegex("Test")] public static partial Regex Generated(); } ``` Execute `dotnet build` on `SanityCheckSourceGenerators.Sample`, or compile it in your IDE of choice. ## Without [`Emik.SourceGenerators.Tattoo`](https://www.nuget.org/packages/Emik.SourceGenerators.Tattoo) (much longer; without magic dependency) #### `./SanityCheckSourceGenerators/SanityCheckSourceGenerators.csproj` ```xml latest true netstandard2.0 true ``` #### `./SanityCheckSourceGenerators/Generator.cs` ```cs using Microsoft.CodeAnalysis; namespace SanityCheckSourceGenerators; [Generator] public class Generator : ISourceGenerator { public void Initialize(GeneratorInitializationContext context) { } public void Execute(GeneratorExecutionContext context) => context.AddSource("Test", "global using System.Text.RegularExpressions;"); } ``` #### `./SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj` ```xml net7.0 ``` #### `./SanityCheckSourceGenerators.Sample/A.cs` ```cs partial class A { [GeneratedRegex("Test")] public static partial Regex Generated(); } ``` Execute `dotnet build` on `SanityCheckSourceGenerators.Sample`, or compile it in your IDE of choice. ### Expected behavior Source-generates the method, and therefore compiles, as this log demonstrates. ```bash MSBuild version 17.6.8+c70978d4d for .NET Determining projects to restore... Restored /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj (in 82 ms). Restored /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators/SanityCheckSourceGenerators.csproj (in 169 ms). SanityCheckSourceGenerators -> /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators/bin/Debug/netstandard2.0/SanityCheckSourceGenerators.dll SanityCheckSourceGenerators.Sample -> /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/bin/Debug/net7.0/SanityCheckSourceGenerators.Sample.dll Build succeeded. 0 Warning(s) 0 Error(s) Time Elapsed 00:00:02.91 ``` ### Actual behavior Doesn't source-generate the method, and therefore fails to compile since the method is no longer implemented. #### SDK `7.0.304` ```bash emik@spicy-linux:~/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample$ dotnet build MSBuild version 17.6.8+c70978d4d for .NET Determining projects to restore... All projects are up-to-date for restore. SanityCheckSourceGenerators -> /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators/bin/Debug/netstandard2.0/SanityCheckSourceGenerators.dll /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/Examples.cs(5,33): error CS8795: Partial method 'A.Generated()' must have an implementation part because it has accessibility modifiers. [/home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj] Build FAILED. /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/Examples.cs(5,33): error CS8795: Partial method 'A.Generated()' must have an implementation part because it has accessibility modifiers. [/home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj] 0 Warning(s) 1 Error(s) Time Elapsed 00:00:01.16 ``` #### SDK `8.0.100-preview.6.23330.14` ```bash emik@spicy-linux:~/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample$ dotnet build MSBuild version 17.7.0+5785ed5c2 for .NET Determining projects to restore... All projects are up-to-date for restore. /home/emik/.dotnet/sdk/8.0.100-preview.6.23330.14/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.RuntimeIdentifierInference.targets(314,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy [/home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj] SanityCheckSourceGenerators -> /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators/bin/Debug/netstandard2.0/SanityCheckSourceGenerators.dll /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/Examples.cs(5,33): error CS8795: Partial method 'A.Generated()' must have an implementation part because it has accessibility modifiers. [/home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj] Build FAILED. /home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/Examples.cs(5,33): error CS8795: Partial method 'A.Generated()' must have an implementation part because it has accessibility modifiers. [/home/emik/quarantine/SanityCheck/SanityCheckSourceGenerators/SanityCheckSourceGenerators.Sample/SanityCheckSourceGenerators.Sample.csproj] 0 Warning(s) 1 Error(s) ``` ### Regression? Unlikely. The source generator was introduced in `.NET 7`, and I tested it both on `7.0.306` and `8.0.100-preview.6.23330.14`. (the latest SDKs for both versions at the time of writing this) ### Known Workarounds Write your own global using explicitly: #### `./SanityCheckSourceGenerators.Sample/GlobalUsings.cs` ```cs global using System.Text.RegularExpressions; ``` Now it recognizes the method. When executing `dotnet build`, the build finally passes. A real-world example of a commit that implements this is a project that I maintain, which can be [seen here](https://github.com/Emik03/Emik.Morsels/commit/487621967e62235381b72c8437b2075b1aad6542). ### Configuration - SDK: `7.0.304`/`8.0.100-preview.6.23330.14` - OS: Pop!_OS 22.04 LTS x86_64 - CPU: AMD Ryzen 5 3600 (12) @ 3.700GHz I am pretty confident this affects all environments, however I have yet to actually confirm this. ### Other information _No response_
Author: Emik03
Assignees: -
Labels: `area-System.Text.RegularExpressions`
Milestone: -
MihaZupan commented 1 year ago

This imports every namespace

Why would you want this?

Emik03 commented 1 year ago

This imports every namespace

Why would you want this?

For the same reason that C# 10's global using feature does, which is that most IDEs already show every single type and auto-import namespaces, just that this is amped up a notch. Type name conflicts do happen, but they would happen anyway if you happen to use both, and the naming conventions as described by Microsoft already discourage naming things that most definitely would conflict with other types from the standard library. I personally find that declaring using X = Y.Z; is not that big of a deal anyway.

Regardless, I don't think it's entirely unreasonable to think that someone may run into a similar issue at some point in the future having made a source generator that just so happens to also globally import that exact namespace. It feels incredibly weird that the code compiles, but the generator doesn't, it's a pitfall waiting to happen in my opinion as was the case with me.

Tornhoof commented 1 year ago

If you import the NS via SrcGen, then aren't you implicitly relying on the output from one SrcGen in another SrcGen? that's one of the scenarios which afaik are not supported. You can't depend on the output of one SrcGen in another SrcGen.

Emik03 commented 1 year ago

If you import the NS via SrcGen, then aren't you implicitly relying on the output from one SrcGen in another SrcGen? that's one of the scenarios which afaik are not supported. You can't depend on the output of one SrcGen in another SrcGen.

While this is very true, I feel that in all cases GeneratedRegexAttribute is unambiguous. Would introducing an edgecase of "if we have no idea where GeneratedRegexAttribute comes from, but we indeed see its exact name present, to treat it as such anyway" cause any potential regressions or problems down the road?

ericstj commented 1 year ago

Seems to me a problem with Emik.SourceGenerators.Tattoo in that it needs https://github.com/dotnet/roslyn/issues/57239 to better meet its functionality needs. In lieu of that it might want to do its generation outside of compile in an MSBuild task to ensure that happens before other generators run.

I don't think our generators should loosen their binding behavior for attributes. Take that argument to the limit - it means generators should never consider namespace in type name comparisons (since this is what every generator that wants to work with that global usings generator would need to do) and that's clearly a bad idea.

stephentoub commented 1 year ago

I agree with @ericstj; we should not squat on the GeneratedRegex name and assume that it's the .NET SDK's to use regardless of namespace.

(And, as a technical aside, the Roslyn APIs we use to efficiently find relevant attributed methods expect the full namespace-qualified attribute name: https://learn.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.syntaxvalueprovider.forattributewithmetadataname?view=roslyn-dotnet-4.6.0. Use of that method is critical for generator performance.)

Thanks for the suggestion, though, and for highlighting the difficulties with this particular other generator.

Emik03 commented 1 year ago

This is a very valid point, and I've changed my mind regarding the change. I hope to see the mentioned issue be figured out sometime.

I'll append a workaround here in case if anyone in the future stumbles on this: While unclean, the temporary solution takes inspiration with the aforementioned suggestion regarding making it an MSBuild task, and ultimately decided the best (albeit hacky) approach is to write to the file system directly, creating a generated file within the project root, and then only regenerating it if no file is found to have the name GlobalUsings.g.cs such that you are able to move the file wherever you desire. This guarantees the file will exist in every context, and doesn't rely on any ordering of operations.

Thanks for considering this issue!