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

Analyzer #116

Closed
wants to merge 6 commits into from
Closed

Analyzer #116

wants to merge 6 commits into from

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Dec 24, 2020

image
vs.
image
image
image
image

Still to do: drop into vsix.

Fixes #67

Comment on lines +4 to +7
<clear />
<add key="nuget.org" value="https://api.nuget.org/v3/index.json" />
<add key="dotnet-tools" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json" />
</packageSources>

Choose a reason for hiding this comment

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

⚠️ ⚠️ These feeds shouldn't be mixed.

@YuliiaKovalova
Copy link
Contributor

@Forgind, do you plan to finalize this PR?

{
if (targetMethod.ContainingAssembly.Name.Equals("Microsoft.Build.Locator") && msbuildLocatorRegisterMethods.Contains(targetMethod.Name))
{
MethodDeclarationSyntax method = (MethodDeclarationSyntax)invocation.Syntax.FirstAncestorOrSelf((Func<SyntaxNode, bool>)(a => a is MethodDeclarationSyntax));

Choose a reason for hiding this comment

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

Why do this? I don't think syntax is any important here. e.g, this will be a false negative in a local function or constructor for example. But I'm not suggesting to check against more syntax kinds, instead, I suggest removing the method != null check completely and remove this variable as well.

Comment on lines +60 to +68
foreach (var child in symbolParent.Descendants())
{
if ((child is IInvocationOperation op && msBuildAssemblies.Contains(op.TargetMethod?.ContainingAssembly?.Name)) ||
(child is ITypeOfOperation toOp && msBuildAssemblies.Contains(toOp.TypeOperand.ContainingAssembly?.Name)) ||
(msBuildAssemblies.Contains(child.Type?.ContainingAssembly?.Name)))
{
opact.ReportDiagnostic(Diagnostic.Create(Rule, child.Syntax.GetLocation()));
}
}
Copy link

@Youssef1313 Youssef1313 Jun 15, 2023

Choose a reason for hiding this comment

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

A different approach would be something like this:

context.RegisterOperationBlockStartAction(context =>
{
    if (context.OwningSymbol.Kind != SymbolKind.Method)
    {
        return;
    }

    var hasRegisterMethod = false;
    var hasBadReference = false;
    
    context.RegisterOperationAction(context =>
    {
        var invocation = (IInvocationOperation)opact.Operation;
        var targetMethod = invocation.TargetMethod;
        if (targetMethod.ContainingAssembly.Name.Equals("Microsoft.Build.Locator") && msbuildLocatorRegisterMethods.Contains(targetMethod.Name))
        {
            hasRegisterMethod = true;
        }
        else if (msBuildAssemblies.Contains(targetMethod.ContainingAssembly.Name)))
        {
            hasBadReference = true;
        }
    }, OperationKind.Invocation);

    context.RegisterOperationAction(context =>
    {
        var type = context.Operation switch
        {
            IObjectCreationOperation objectCreation => objectCreation.Type,
            ITypeOfOperation typeofOperation => typeofOperation.TypeOperand,
            _ => null,
        };

        if (msBuildAssemblies.Contains(type?.ContainingAssembly.Name)))
        {
            hasBadReference = true;
        }
    }, OperationKind.ObjectCreation, OperationKind.TypeOf);

    context.RegisterOperationBlockEndAction(context =>
    {
        if (hasRegisterMethod && hasBadReference)
        {
            // report diagnostic. This might need some ConcurrentSet to keep the syntax nodes where we want to report diagnostics.
        }
    });
});

Not sure if that's better. I also wrote directly in GitHub so could have made mistakes, but the idea is to first register operation block start action, then on that, register operation actions for invocation, typeof, and object creation and gather the needed info (whether a register method is called and whether there is another problematic reference)

Then, on operation block end, we have gathered the needed info, and decide whether to report a diagnostic or not.

Choose a reason for hiding this comment

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

I'd defer to someone from Roslyn team on which approach would be better.

private static readonly LocalizableString Description = new LocalizableResourceString(nameof(Resources.AnalyzerDescription), Resources.ResourceManager, typeof(Resources));
private const string Category = "Naming";

private readonly string[] msBuildAssemblies =

Choose a reason for hiding this comment

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

This should be static, and probably immutable array as well.

"Microsoft.Build.Utilities.Core"
};

private readonly string[] msbuildLocatorRegisterMethods =

Choose a reason for hiding this comment

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

This should be static, and probably immutable array as well.

var targetMethod = invocation.TargetMethod;
if (targetMethod?.ContainingAssembly != null)
{
if (targetMethod.ContainingAssembly.Name.Equals("Microsoft.Build.Locator") && msbuildLocatorRegisterMethods.Contains(targetMethod.Name))

Choose a reason for hiding this comment

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

Consider having a compilation start action that does:

var locatorSymbol = context.Compilation.GetTypeByMetadataName("Microsoft.Build.Locator.MSBuildLocator");
if (locatorSymbol is null)
    return;

// then check targetMethod.ContainingType.Equals(locatorSymbol, SymbolEqualityComparer.Default) instead of comparing the containing assembly name

@Forgind
Copy link
Member Author

Forgind commented Jun 15, 2023

@Forgind, do you plan to finalize this PR?

I think this was a worthwhile project in getting people to not make this (rather confusing) mistake, but I didn't know how to actually ship the analyzer such that it would run on users' code. I think it might be worthwhile to ship this (and I can take Youssef1313's comments into account if so) if someone can offer a suggestion as to how to make the analyzer run for users?

</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.CodeAnalysis.Analyzers" Version="2.9.8" />

Choose a reason for hiding this comment

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

nit: This version is quite old and has known annoying bugs.

Comment on lines +5 to +6
<IsPackable>true</IsPackable>
<IncludeBuildOutput>false</IncludeBuildOutput>

Choose a reason for hiding this comment

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

This mixes tabs & spaces.

<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFrameworks>net48;netcoreapp3.1</TargetFrameworks>

Choose a reason for hiding this comment

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

This must be netstandard2.0

<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFrameworks>net48;netcoreapp3.1</TargetFrameworks>

Choose a reason for hiding this comment

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

This must be netstandard2.0

I missed reviewing this earlier and only looked when you asked for how this is going to be shipped.

Copy link

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

@rainersigwald
Copy link
Member

I think it is still a reasonable idea to have an analyzer that helps people use MSBuildLocator. This PR may be a good starting point for that, or it may be stale enough that it's easier to start over. Either way, I don't think we're planning to invest in an analyzer right now and I'd be ok closing this until we are.

@YuliiaKovalova
Copy link
Contributor

Close with the comment: #116 (comment)

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.

Roslyn Analyzer
4 participants