Analyzer #116
Analyzer #116
Conversation
| <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> |
There was a problem hiding this comment.
|
@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)); |
There was a problem hiding this comment.
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.
| 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())); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
This should be static, and probably immutable array as well.
| "Microsoft.Build.Utilities.Core" | ||
| }; | ||
|
|
||
| private readonly string[] msbuildLocatorRegisterMethods = |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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
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" /> |
There was a problem hiding this comment.
nit: This version is quite old and has known annoying bugs.
| <IsPackable>true</IsPackable> | ||
| <IncludeBuildOutput>false</IncludeBuildOutput> |
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFrameworks>net48;netcoreapp3.1</TargetFrameworks> |
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFrameworks>net48;netcoreapp3.1</TargetFrameworks> |
There was a problem hiding this comment.
This must be netstandard2.0
I missed reviewing this earlier and only looked when you asked for how this is going to be shipped.
There was a problem hiding this comment.
Regarding packaging this. You can check the analyzers template:
Basically, we would need a project that packs both the analyzer and codefix dlls in analyzers/dotnet/cs directory, let's name the project MSBuildLocatorAnalyzer.Package for example. It would be similar to this:
The analyzers csproj would be as simple as this:
as well as the codefix project:
|
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. |
|
Close with the comment: #116 (comment) |
vs.
Still to do: drop into vsix.
Fixes #67