Skip to content
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

Implement RS1038 (Strict compiler reference analysis) #6705

Merged
merged 5 commits into from
Jun 22, 2023
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
3 changes: 3 additions & 0 deletions Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
22 changes: 22 additions & 0 deletions docs/rules/RS1022.md
Original file line number Diff line number Diff line change
@@ -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
> ```
46 changes: 46 additions & 0 deletions docs/rules/RS1038.md
Original file line number Diff line number Diff line change
@@ -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**.
2 changes: 1 addition & 1 deletion eng/Versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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))
{
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
Loading