Don't load generators that target net framework explicitly. by chsienki · Pull Request #47100 · dotnet/roslyn

chsienki · 2020-08-24T21:55:48Z

Issue a diagnostic and don't load generator if it has references to mscorlib

chsienki · 2020-08-24T21:56:52Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerFileReference.cs

+                            if (reference.Name?.Equals("mscorlib", StringComparison.OrdinalIgnoreCase) == true)
+                            {
+                                var publicKeyToken = reference.GetPublicKeyToken();
+                                if (publicKeyToken is object && BitConverter.ToString(publicKeyToken).Equals(ExpectedPublicKey, StringComparison.OrdinalIgnoreCase))


@jaredpar Not sure who else to ask. Does this seem like a sensible way to check for mscorlib references? (Or do we have a utility that does it in the code already?)

Rather than checking for mscorlib references I would look at the TargetFrameworkAttribute on the assembly. If the moniker begins with .NETFRamework when we should reject.

Ok, and we're ok that you can create .NET Framework referencing assemblies without a TargetFrameworkAttribute? A default csc.exe Generator.cs invocation will target framework, but not add the attribute.

Yes. Any check here is imprecise. The majority case is that they use our build infra,n not the raw compiler, which will add this attribute.

chsienki · 2020-08-24T21:58:33Z

@dotnet/roslyn-compiler for a fairly small review please :)

…rence

cston · 2020-08-25T21:56:48Z

src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerFileReferenceTests.cs

+
+                var builder = ImmutableArray.CreateBuilder<ISourceGenerator>();
+                reference.AddGenerators(builder, LanguageNames.CSharp);
+                var analyzers = builder.ToImmutable();


var analyzers = builder.ToImmutable(); [](start = 16, length = 38)

The statement appears to be unused. Should we Assert.Equal(1, analyzers.Length)? #Closed

Honestly, that was just left over from copy paste, but it seems sensible to assert that we did actually load a generator, so will add.

jaredpar · 2020-08-25T22:09:08Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerFileReference.cs


                    Debug.Assert(type != null);

+                    // check if this references net framework, and issue a diagnostic if we don't allow that


Suggested change

// check if this references net framework, and issue a diagnostic if we don't allow that

// check if this references net framework, and issue a diagnostic that this isn't supported

jaredpar · 2020-08-25T22:15:10Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerFileReference.cs

+                    // check if this references net framework, and issue a diagnostic if we don't allow that
+                    if (!_allowNetFramework)
+                    {
+                        var targetFrameworkAttribute = analyzerAssembly.GetCustomAttribute<TargetFrameworkAttribute>();


GetCustomAttribute<T> will throw when dealing with invalid metadata (basically multiple copies of TargetFarmeworkAttribute even though it has AllowMultiple = false). Looked around a bit and seems like we are okay with this type of invalid IL breaking the compiler though. So think we can leave as is.

I did look at the source code to see how it handled those cases and it seems to just take the first one, rather than throwing?

The comment I saw said it threw. Either way though, seems like there is lots of precedenc for this.

Mostyl I commented because it took me ten minutes to figure out where this call was coming from. I wanted to write something down to make me feel better about all the time I spent looking into it.

jaredpar · 2020-08-25T22:17:22Z

src/Compilers/Core/Portable/CodeAnalysisResources.resx

    <comment>'SourceText' is not localizable.</comment>
  </data>
+  <data name="TypeReferencesNetFramework" xml:space="preserve">
+    <value>The type '{0}' references .NET Framework, which is not supported.</value>


This isn't about the type though, it's about the assembly. I would phrase it as

The assembly containing type '{0}' ...

jaredpar · 2020-08-25T22:18:18Z

src/Compilers/Test/Resources/Core/TestResources.cs

+        public static byte[] NetStandardGenerator => ResourceLoader.GetOrCreateResource(ref s_netStandardGenerator, "Generators.NetStandardGenerator.dll");
+
+        private static byte[] s_noTargetGenerator;
+        public static byte[] NoTargetGenerator => ResourceLoader.GetOrCreateResource(ref s_noTargetGenerator, "Generators.NoTargetGenerator.dll");


Rather than checking in DLLs, which continue to inflate our repo size, why not just build them as a part of the tests that need them?

Ah yeah didn't think of that, plus now we're not actually dealing with references this is much simpler to do.

cston · 2020-08-25T22:20:09Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerFileReference.cs


-            _diagnosticAnalyzers = new Extensions<DiagnosticAnalyzer>(this, IsDiagnosticAnalyzerAttribute, GetDiagnosticsAnalyzerSupportedLanguages);
-            _generators = new Extensions<ISourceGenerator>(this, IsGeneratorAttribute, GetGeneratorsSupportedLanguages);
+            _diagnosticAnalyzers = new Extensions<DiagnosticAnalyzer>(this, IsDiagnosticAnalyzerAttribute, GetDiagnosticsAnalyzerSupportedLanguages, allowNetFramework: true);


allowNetFramework: true [](start = 149, length = 23)

Why are analyzers allowed to target .NET Framework but generators are not? #Closed

They were never supported, we just failed to enforce that fact. Now the fact that we didn't enforce is one of the hurdles for us movign fully to .NET Core. This change is an attempt to not make the same mistake, and create the same hurdle, for source generators,

- Remove test resources and create generators on the fly - Reword some text

cston · 2020-08-26T00:38:38Z

src/Compilers/Core/CodeAnalysisTest/Microsoft.CodeAnalysis.UnitTests.csproj

    <ProjectReference Include="..\..\..\Test\Utilities\Portable\Roslyn.Test.Utilities.csproj" />
    <ProjectReference Include="..\..\CSharp\Portable\Microsoft.CodeAnalysis.CSharp.csproj" />
    <ProjectReference Include="..\..\Test\Resources\Core\Microsoft.CodeAnalysis.Compiler.Test.Resources.csproj" />
+    <ProjectReference Include="..\..\Test\Utilities\CSharp\Microsoft.CodeAnalysis.CSharp.Test.Utilities.csproj" />


....\Test\Utilities\CSharp\Microsoft.CodeAnalysis.CSharp.Test.Utilities.csproj [](start = 31, length = 79)

Do we need this project reference? If it's just to create a CSharpCompilation, we should use new CSharpCompilation(...) directly instead. #Closed

Ah, yeah that was why I added it. I originally started by calling CSharpCompilation directly, but it was proving tricky to get all the references right. I'll switch back to that way of doing it.

* upstream/master: (220 commits) Don't load generators that target net framework explicitly. (dotnet#47100) Update src/Features/Core/Portable/UnifiedSuggestions/UnifiedSuggestedActionsSource.cs Clean up redundant code action filtering Remove IActiveStatementSpanTracker (dotnet#46826) Disable API analysis if telemetry is disabled Update src/Workspaces/Remote/ServiceHub/Services/CodeAnalysis/CodeAnalysisService_SemanticClassificationCache.cs Doc Compress two values. PR feedback Revert Revert Lint Fix formatting Fire and forget Fix mangling Ensure that local functions are also marked as invalid if their containing methods are generic. Fix dependency graph for AnalyzerRunner Add AnalyzerRunner target for net5.0 Make VerifyForwardedTypes asynchronous Fix bug when "End statement" is used in single-line if (dotnet#47062) ...

Don't load generators that target net framework explicitly.

73c08a0

chsienki added Area-Compilers Feature - Source Generators Source Generators labels Aug 24, 2020

chsienki requested a review from a team as a code owner August 24, 2020 21:55

chsienki commented Aug 24, 2020

View reviewed changes

Do lookup based on TargetFrameworkAttribute rather than mscorlib refe…

8d5265c

…rence

cston reviewed Aug 25, 2020

View reviewed changes

jaredpar reviewed Aug 25, 2020

View reviewed changes

cston reviewed Aug 25, 2020

View reviewed changes

PR feedback:

a2ec874

- Remove test resources and create generators on the fly - Reword some text

cston reviewed Aug 26, 2020

View reviewed changes

Build the compilation manually, rather than importing createCompilation

9e41bea

cston approved these changes Aug 26, 2020

View reviewed changes

jaredpar approved these changes Aug 28, 2020

View reviewed changes

chsienki merged commit f72a248 into dotnet:master Aug 28, 2020

ghost added this to the Next milestone Aug 28, 2020

chsienki deleted the sg_no_framework branch August 28, 2020 05:22

allisonchou modified the milestones: Next, 16.8.P3 Aug 31, 2020

chsienki restored the sg_no_framework branch April 1, 2021 18:45

RikkiGibson mentioned this pull request Jul 12, 2022

Public API for "Compiler needs to declaratively fail when loading newer analyzers" #62583

Closed


		Debug.Assert(type != null);

		// check if this references net framework, and issue a diagnostic if we don't allow that

Conversation

chsienki commented Aug 24, 2020

Uh oh!

chsienki Aug 24, 2020 • 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!

chsienki commented Aug 24, 2020

Uh oh!

cston Aug 25, 2020 • 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!

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!

cston Aug 25, 2020 • 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!

cston Aug 26, 2020 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chsienki Aug 24, 2020 •

edited

Loading

cston Aug 25, 2020 •

edited

Loading

cston Aug 25, 2020 •

edited

Loading

cston Aug 26, 2020 •

edited

Loading