Merge pull request #6705 from sharwell/strict-references

Implement RS1038 (Strict compiler reference analysis)

Directory.Build.props

@@ -49,6 +49,9 @@
     <!-- When building in source build mode, treat this set of warnings not as errors.-->
     <!-- Some crefs reference internal APIs not present in the reference package. -->
     <NoWarn>$(NoWarn);CS1574;CS8602</NoWarn>
+    <!-- Source build reference assemblies are not correctly annotated.
+      https://github.com/dotnet/source-build/issues/3531 -->
+    <NoWarn>$(NoWarn);CS8603</NoWarn>
   </PropertyGroup>
 
   <PropertyGroup>

docs/rules/RS1022.md

@@ -0,0 +1,22 @@
+## RS1022: Do not use types from Workspaces assembly in an analyzer
+
+Diagnostic analyzer types should not use types from Workspaces assemblies. Workspaces assemblies are only available when the analyzer executes in Visual Studio IDE live analysis, but are not available during command line build. Referencing types from Workspaces assemblies will lead to runtime exception during analyzer execution in command line build.
+
+|Item|Value|
+|-|-|
+|Category|MicrosoftCodeAnalysisCorrectness|
+|Enabled|True|
+|Severity|Warning|
+|CodeFix|False|
+---
+
+> **Warning**
+>
+> The analysis performed by RS1022 is slow and relies on implementation details of the JIT compiler for correctness.
+> Authors of compiler extensions are encouraged to use the stricter (and faster) analyzer RS1038 instead of this rule.
+>
+> RS1038 is enabled by default. To enable RS1022 instead, the following configuration may be added to **.globalconfig**:
+>
+> ```ini
+> roslyn_correctness.assembly_reference_validation = relaxed
+> ```

docs/rules/RS1038.md

@@ -0,0 +1,46 @@
+## RS1038: Compiler extensions should be implemented in assemblies with compiler-provided references
+
+Types which implement compiler extension points should not be declared in assemblies that contain references to assemblies which are not provided by all compilation scenarios. Doing so may cause the feature to behave unpredictably.
+
+|Item|Value|
+|-|-|
+|Category|MicrosoftCodeAnalysisCorrectness|
+|Enabled|True|
+|Severity|Warning|
+|CodeFix|False|
+---
+
+This rule helps ensure compiler extensions (e.g. analyzers and source generators) will load correctly in all compilation
+scenarios. Depending on the manner in which the compiler is invoked, some assemblies may not be present during a build,
+and attempting to reference them will result in exceptions that prevent the compiler extension from loading. RS1038 is
+the most strict and best performing validation for this scenario.
+
+RS1038 is enabled by default unless relaxed validation has been manually enabled in **.globalconfig** as described in
+[RS1022](RS1022.md).
+
+### Rules for compiler feature references
+
+* Compiler features supporting C# code should only reference the NuGet packages **Microsoft.CodeAnalysis.Common** and/or **Microsoft.CodeAnalysis.CSharp**
+* Compiler features supporting Visual Basic code should only reference **Microsoft.CodeAnalysis.Common** and/or **Microsoft.CodeAnalysis.VisualBasic**
+* Compiler features supporting both C# and Visual Basic should only reference **Microsoft.CodeAnalysis.Common**
+* Compiler features should not be implemented in assemblies containing a reference to **Microsoft.CodeAnalysis.Workspaces.Common**
+
+> **Note**
+>
+> This analyzer only checks references to the core Roslyn assemblies. Compiler extensions with other dependencies may
+> face restrictions and/or packaging requirements outside the scope of this analyzer.
+
+### Compiler extension points
+
+The following compiler extension points are examined by this analyzer:
+
+* `DiagnosticAnalyzer`
+* `DiagnosticSuppressor`
+* `ISourceGenerator`
+* `IIncrementalGenerator`
+
+### Other extension points
+
+Some extension points provided by Roslyn are IDE extensions (e.g. code fixes and completion providers). These features
+may ship in the same package as compiler features, but should be implemented in their own assembly since they require a
+reference to non-compiler package **Microsoft.CodeAnalysis.Workspaces.Common**.

eng/Versions.props

@@ -69,7 +69,7 @@
     <MicrosoftCodeAnalysisVersionForResxSourceGenerators>4.0.1</MicrosoftCodeAnalysisVersionForResxSourceGenerators>
     <MicrosoftCodeAnalysisVersionForNetAnalyzers>3.3.1</MicrosoftCodeAnalysisVersionForNetAnalyzers>
     <MicrosoftCodeAnalysisVersionForTextAnalyzers>3.3.1</MicrosoftCodeAnalysisVersionForTextAnalyzers>
-    <MicrosoftCodeAnalysisVersionForCodeAnalysisAnalyzers>3.3.1</MicrosoftCodeAnalysisVersionForCodeAnalysisAnalyzers>
+    <MicrosoftCodeAnalysisVersionForCodeAnalysisAnalyzers>3.7.0</MicrosoftCodeAnalysisVersionForCodeAnalysisAnalyzers>
     <MicrosoftCodeAnalysisVersionForToolsAndUtilities>3.3.1</MicrosoftCodeAnalysisVersionForToolsAndUtilities>
     <!-- Versions for tests and general utility execution. -->
     <!-- This version is for utility and executable assemblies. The version here should not overlap with any of the surface

src/Microsoft.CodeAnalysis.Analyzers/CSharp/MetaAnalyzers/Fixers/CSharpPreferIsKindFix.cs

@@ -17,6 +17,9 @@ public sealed class CSharpPreferIsKindFix : PreferIsKindFix
         protected override SyntaxNode? TryGetNodeToFix(SyntaxNode root, TextSpan span)
         {
             var binaryExpression = root.FindNode(span, getInnermostNodeForTie: true).FirstAncestorOrSelf<BinaryExpressionSyntax>();
+            if (binaryExpression is null)
+                return null;
+
             if (binaryExpression.Left.IsKind(SyntaxKind.InvocationExpression) ||
                 binaryExpression.Left.IsKind(SyntaxKind.ConditionalAccessExpression))
             {

src/Microsoft.CodeAnalysis.Analyzers/Core/AnalyzerReleases.Unshipped.md

@@ -1 +1,7 @@
 ; Please do not edit this file manually, it should only be updated through code fix application.
+
+### New Rules
+
+Rule ID | Category | Severity | Notes
+--------|----------|----------|-------
+RS1038 | MicrosoftCodeAnalysisCorrectness | Warning | CompilerExtensionStrictApiAnalyzer

src/Microsoft.CodeAnalysis.Analyzers/Core/CodeAnalysisDiagnosticsResources.resx

@@ -556,4 +556,19 @@
   <data name="NoSettingSpecifiedSymbolIsBannedInAnalyzersTitle" xml:space="preserve">
     <value>Specify analyzer banned API enforcement setting</value>
   </data>
+  <data name="DoNotDeclareCompilerFeatureInAssemblyWithWorkspacesReferenceMessage" xml:space="preserve">
+    <value>This compiler extension should not be implemented in an assembly containing a reference to Microsoft.CodeAnalysis.Workspaces. The Microsoft.CodeAnalysis.Workspaces assembly is not provided during command line compilation scenarios, so references to it could cause the compiler extension to behave unpredictably.</value>
+  </data>
+  <data name="DoNotDeclareCSharpCompilerFeatureInAssemblyWithVisualBasicReferenceMessage" xml:space="preserve">
+    <value>This C# compiler extension should not be implemented in an assembly containing a reference to Microsoft.CodeAnalysis.VisualBasic. The Microsoft.CodeAnalysis.VisualBasic assembly is not always provided during C# compilation scenarios, so references to it could cause the compiler extension to behave unpredictably.</value>
+  </data>
+  <data name="DoNotDeclareVisualBasicCompilerFeatureInAssemblyWithCSharpReferenceMessage" xml:space="preserve">
+    <value>This Visual Basic compiler extension should not be implemented in an assembly containing a reference to Microsoft.CodeAnalysis.CSharp. The Microsoft.CodeAnalysis.CSharp assembly is not always provided during Visual Basic compilation scenarios, so references to it could cause the compiler extension to behave unpredictably.</value>
+  </data>
+  <data name="DoNotRegisterCompilerTypesWithBadAssemblyReferenceRuleDescription" xml:space="preserve">
+    <value>Types which implement compiler extension points should not be declared in assemblies that contain references to assemblies which are not provided by all compilation scenarios. Doing so may cause the feature to behave unpredictably.</value>
+  </data>
+  <data name="DoNotRegisterCompilerTypesWithBadAssemblyReferenceRuleTitle" xml:space="preserve">
+    <value>Compiler extensions should be implemented in assemblies with compiler-provided references</value>
+  </data>
 </root>

src/Microsoft.CodeAnalysis.Analyzers/Core/DiagnosticIds.cs

@@ -41,6 +41,7 @@ internal static class DiagnosticIds
         public const string SymbolIsBannedInAnalyzersRuleId = "RS1035";
         public const string NoSettingSpecifiedSymbolIsBannedInAnalyzersRuleId = "RS1036";
         public const string AddCompilationEndCustomTagRuleId = "RS1037";
+        public const string DoNotRegisterCompilerTypesWithBadAssemblyReferenceRuleId = "RS1038";
 
         // Release tracking analyzer IDs
         public const string DeclareDiagnosticIdInAnalyzerReleaseRuleId = "RS2000";

src/Microsoft.CodeAnalysis.Analyzers/Core/FixAnalyzers/FixerWithFixAllAnalyzer.Fixer.cs

@@ -5,6 +5,7 @@
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
+using Analyzer.Utilities;
 using Analyzer.Utilities.Extensions;
 using Microsoft.CodeAnalysis.Analyzers.FixAnalyzers;
 using Microsoft.CodeAnalysis.CodeActions;
@@ -28,7 +29,7 @@ public sealed override FixAllProvider GetFixAllProvider()
 
         public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
         {
-            SyntaxNode root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
+            SyntaxNode root = await context.Document.GetRequiredSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
             SyntaxToken token = root.FindToken(context.Span.Start);
             if (!token.Span.IntersectsWith(context.Span))
             {
@@ -52,7 +53,7 @@ private static async Task<Document> AddMethodAsync(Document document, SyntaxNode
             var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false);
             var generator = editor.Generator;
 
-            var model = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false);
+            var model = await document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);
             var typeIsSealed = ((INamedTypeSymbol)model.GetDeclaredSymbol(classDecl, cancellationToken)).IsSealed;
 
             INamedTypeSymbol? codeFixProviderSymbol = model.Compilation.GetOrCreateTypeByMetadataName(FixerWithFixAllAnalyzer.CodeFixProviderMetadataName);

src/Microsoft.CodeAnalysis.Analyzers/Core/InternalImplementationOnlyAnalyzer.cs

@@ -52,7 +52,7 @@ private static void AnalyzeSymbol(SymbolAnalysisContext context)
                 // We are doing a string comparison of the name here because we don't care where the attribute comes from.
                 // CodeAnalysis.dll itself has this attribute and if the user assembly also had it, symbol equality will fail
                 // but we should still issue the error.
-                if (attributes.Any(a => a.AttributeClass.Name.Equals(InternalImplementationOnlyAttributeName, StringComparison.Ordinal)
+                if (attributes.Any(a => a.AttributeClass is { Name: InternalImplementationOnlyAttributeName }
                                         && a.AttributeClass.ToDisplayString().Equals(InternalImplementationOnlyAttributeFullName, StringComparison.Ordinal)) &&
                     !iface.ContainingAssembly.GivesAccessTo(namedTypeSymbol.ContainingAssembly))
                 {

src/Microsoft.CodeAnalysis.Analyzers/Core/MetaAnalyzers/CompareSymbolsCorrectlyAnalyzer.cs

@@ -271,7 +271,7 @@ private static bool IsSymbolType(IOperation? operation, INamedTypeSymbol? symbol
         private static bool IsSymbolType(ITypeSymbol typeSymbol, INamedTypeSymbol? symbolType)
             => typeSymbol != null
                 && (SymbolEqualityComparer.Default.Equals(typeSymbol, symbolType)
-                    || typeSymbol.AllInterfaces.Contains(symbolType));
+                    || typeSymbol.AllInterfaces.Any(SymbolEqualityComparer.Default.Equals, symbolType));
 
         private static bool IsSymbolClassType(IOperation operation)
         {