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

Add new compiler codebase analyzer for primary constructor capturing #7138

Merged
merged 4 commits into from
Jan 25, 2024

Conversation

333fred
Copy link
Member

@333fred 333fred commented Jan 12, 2024

The compiler side of the codebase is looking avoid implicit capture for primary constructor parameters, so I've encoded that into an analyzer. It's off by default, and we'll turn it on for the Compilers/* section of the codebase.

The compiler side of the codebase is looking avoid implicit capture for primary constructor parameters, so I've encoded that into an analyzer. It's off by default, and we'll turn it on for the `Compilers/*` section of the codebase.
@333fred 333fred marked this pull request as ready for review January 23, 2024 22:12
@333fred 333fred requested a review from a team as a code owner January 23, 2024 22:12
Copy link

codecov bot commented Jan 24, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (8d8ac7d) 96.44% compared to head (32f8641) 96.44%.
Report is 57 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #7138    +/-   ##
========================================
  Coverage   96.44%   96.44%            
========================================
  Files        1413     1415     +2     
  Lines      337719   338191   +472     
  Branches    11177    11191    +14     
========================================
+ Hits       325700   326161   +461     
- Misses       9200     9209     +9     
- Partials     2819     2821     +2     

Copy link

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 4)

@333fred
Copy link
Member Author

333fred commented Jan 24, 2024

@dotnet/roslyn-compiler for reviews please.

{
var operation = (IParameterReferenceOperation)context.Operation;

if (operation.Parameter.ContainingSymbol == context.ContainingSymbol || operation.Parameter.ContainingSymbol is not IMethodSymbol { MethodKind: MethodKind.Constructor })
Copy link
Member

Choose a reason for hiding this comment

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

operation.Parameter.ContainingSymbol == context.ContainingSymbol

Having that be the way to detect primary constructors / symbols feels fairly indirect. Was surprised there wasn't a more declarative way. Did we consider one during impl?

Copy link
Member Author

Choose a reason for hiding this comment

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

We specifically didn't expose a way to know what the primary constructor is, but this actually feels more correct to me. The goal here is to understand when a parameter is being used outside of the current method, which is exactly what this expresses.

@sharwell
Copy link
Member

@333fred I would be interested in merging this pull request / feature with:
dotnet/roslyn#69469

@333fred 333fred merged commit bf54e2b into dotnet:main Jan 25, 2024
11 checks passed
@333fred 333fred deleted the roslyn-capture-analyzer branch January 25, 2024 18:25
@sandyarmstrong
Copy link
Member

@333fred we've copied this for our own use. It feels like something a lot of people would want, especially for dependency injection scenarios, as it's not exactly obvious that primary constructor parameters behave this way. Any chance it could be moved (still off-by-default) out of the Roslyn-specific analyzers and into the main analyzers assembly?

I also threw together a rudimentary code fixer for it:

Click to expand
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Formatting;

using System.Collections.Immutable;
using System.Composition;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;

namespace Roslyn.Diagnostics.CSharp.Analyzers;

[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(DoNotCapturePrimaryConstructorParametersFixer)), Shared]
public class DoNotCapturePrimaryConstructorParametersFixer : CodeFixProvider
{
    public override ImmutableArray<string> FixableDiagnosticIds { get; } = [DoNotCapturePrimaryConstructorParametersAnalyzer.DiagnosticId];

    public override FixAllProvider? GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;

    public override async Task RegisterCodeFixesAsync(CodeFixContext context)
    {
        var root = await context.Document.GetSyntaxRootAsync().ConfigureAwait(false);
        if (root is null)
        {
            return;
        }

        var diagnostic = context.Diagnostics.FirstOrDefault();
        if (diagnostic is null)
        {
            return;
        }

        var parameterReference = root
            .FindNode(diagnostic.Location.SourceSpan)
            .FirstAncestorOrSelf<IdentifierNameSyntax>();

        if (parameterReference is null)
        {
            return;
        }

        context.RegisterCodeFix(
            CodeAction.Create(
                "Add readonly field to hide captured primary constructor parameter",
                cancellationToken => AddFieldsToHideCapturedPrimaryConstructorParametersAsync(
                    root, context.Document, parameterReference, cancellationToken),
                nameof(DoNotCapturePrimaryConstructorParametersFixer)),
            diagnostic);
    }

    private static async Task<Document> AddFieldsToHideCapturedPrimaryConstructorParametersAsync(
        SyntaxNode root,
        Document document,
        IdentifierNameSyntax parameterReference,
        CancellationToken cancellationToken)
    {
        var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false);
        if (semanticModel is null)
        {
            return document;
        }

        var typeDeclaration = root.FindNode(parameterReference.Span).FirstAncestorOrSelf<TypeDeclarationSyntax>();
        if (typeDeclaration?.ParameterList is null)
        {
            return document;
        }

        FieldDeclarationSyntax? newField = null;
        foreach (var parameterSyntax in typeDeclaration.ParameterList.Parameters)
        {
            var parameter = semanticModel.GetDeclaredSymbol(parameterSyntax);
            if (parameter is null || parameter.Name != parameterReference.Identifier.Text)
            {
                continue;
            }

            var typeSyntax = SyntaxFactory.ParseTypeName(parameter.Type.ToMinimalDisplayString(
                semanticModel, parameterReference.GetLocation().SourceSpan.Start));

            newField = SyntaxFactory.FieldDeclaration(
                SyntaxFactory.VariableDeclaration(typeSyntax)
                .AddVariables(
                    SyntaxFactory.VariableDeclarator(parameter.Name)
                    .WithInitializer(SyntaxFactory.EqualsValueClause(SyntaxFactory.IdentifierName(parameter.Name)))
                ))
                .WithModifiers(SyntaxFactory.TokenList(
                    SyntaxFactory.Token(SyntaxKind.PrivateKeyword),
                    SyntaxFactory.Token(SyntaxKind.ReadOnlyKeyword)))
                .WithAdditionalAnnotations(Formatter.Annotation);

            break;
        }

        if (newField is null)
        {
            return document;
        }

        var newTypeDeclaration = typeDeclaration.InsertNodesBefore(typeDeclaration.Members.First(), [newField]);
        var newRoot = root.ReplaceNode(typeDeclaration, newTypeDeclaration);

        return document.WithSyntaxRoot(newRoot);
    }
}

@333fred
Copy link
Member Author

333fred commented Jun 21, 2024

@sandyarmstrong you could certainly propose it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants