-
Notifications
You must be signed in to change notification settings - Fork 210
Add a new analyzer + baseline for existing instances #11857
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the MIT license. See License.txt in the project root for license information. | ||
|
||
using System.Collections.Immutable; | ||
using System.IO; | ||
using System.Linq; | ||
using Microsoft.CodeAnalysis; | ||
using Microsoft.CodeAnalysis.Diagnostics; | ||
using Microsoft.CodeAnalysis.Operations; | ||
|
||
namespace Razor.Diagnostics.Analyzers; | ||
|
||
[DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
public class IntermediateTokenAnalyzer : DiagnosticAnalyzer | ||
{ | ||
private const string Title = "IntermediateToken with Kind CSharp"; | ||
private const string MessageFormat = "Avoid directly creating an IntermediateToken with Kind 'CSharp'. Instead, lower to an appropriate intermediate node and emit the C# in the output writer."; | ||
private const string BaselineFileName = "IntermediateTokenBaseline.txt"; | ||
|
||
internal static readonly DiagnosticDescriptor Rule = new( | ||
DiagnosticIds.RawIntermediateTokenCreation, | ||
Title, | ||
MessageFormat, | ||
DiagnosticCategory.Usage, | ||
DiagnosticSeverity.Warning, | ||
isEnabledByDefault: true, | ||
helpLinkUri: "https://github.com/dotnet/razor/issues/11858"); | ||
|
||
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => [Rule]; | ||
|
||
public override void Initialize(AnalysisContext context) | ||
{ | ||
context.EnableConcurrentExecution(); | ||
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); | ||
|
||
context.RegisterOperationAction(AnalyzeObjectCreation, OperationKind.ObjectCreation); | ||
context.RegisterOperationAction(AnalyzeMethodInvocation, OperationKind.Invocation); | ||
} | ||
|
||
private void AnalyzeObjectCreation(OperationAnalysisContext context) | ||
{ | ||
var objectCreation = (IObjectCreationOperation)context.Operation; | ||
|
||
// Check if the type is IntermediateToken | ||
if (objectCreation.Type?.Name != "IntermediateToken") | ||
{ | ||
return; | ||
} | ||
|
||
// Resolve the TokenKind.CSharp enum value | ||
var tokenKindType = context.Compilation.GetTypeByMetadataName("Microsoft.AspNetCore.Razor.Language.Intermediate.TokenKind"); | ||
if (tokenKindType == null) | ||
{ | ||
return; // TokenKind enum not found | ||
} | ||
|
||
var csharpEnumMember = tokenKindType.GetMembers("CSharp").FirstOrDefault() as IFieldSymbol; | ||
if (csharpEnumMember == null) | ||
{ | ||
return; // TokenKind.CSharp enum member not found | ||
} | ||
|
||
// Check if the Kind property is set to TokenKind.CSharp via an initializer | ||
var kindInitializer = objectCreation.Initializer?.Initializers | ||
.OfType<ISimpleAssignmentOperation>() | ||
.FirstOrDefault(init => | ||
init.Target is IPropertyReferenceOperation property && | ||
property.Property.Name == "Kind" && | ||
init.Value.ConstantValue.HasValue && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's safest to have this as In cases of conversions, IIRC, you can only see the constant value, if any, from the conversion operation operand. |
||
init.Value.ConstantValue.Value == csharpEnumMember.ConstantValue); | ||
|
||
// If the initializer doesnt set Kind to TokenKind.CSharp, return | ||
if (kindInitializer == null) | ||
{ | ||
return; | ||
} | ||
|
||
// Report the diagnostic if not suppressed | ||
ReportIfNotSuppressed(context, objectCreation.Syntax.GetLocation()); | ||
} | ||
|
||
private void AnalyzeMethodInvocation(OperationAnalysisContext context) | ||
{ | ||
var invocation = (IInvocationOperation)context.Operation; | ||
|
||
// Check if the method is IntermediateToken.CreateCSharpToken | ||
if (invocation.TargetMethod.Name != "CreateCSharpToken" || | ||
invocation.TargetMethod.ContainingType.Name != "IntermediateToken") | ||
{ | ||
return; | ||
} | ||
|
||
ReportIfNotSuppressed(context, invocation.Syntax.GetLocation()); | ||
} | ||
|
||
private void ReportIfNotSuppressed(OperationAnalysisContext context, Location location) | ||
{ | ||
// If we're a test project, ignore. | ||
if (context.Compilation.AssemblyName?.Contains(".Test") == true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be done very early at compilation start. |
||
{ | ||
return; | ||
} | ||
|
||
// Check if the warning is suppressed in the baseline file | ||
// Note: code for regenerating the baseline from scratch can be found here: https://gist.github.com/chsienki/d06b2a3ee583191cacf80da79f6fc540 | ||
var additionalFiles = context.Options.AdditionalFiles; | ||
var baselineFile = additionalFiles.FirstOrDefault(file => Path.GetFileName(file.Path) == BaselineFileName); | ||
if (baselineFile != null) | ||
{ | ||
var baselineDirectory = Path.GetDirectoryName(baselineFile.Path); | ||
var locationFilePath = location.SourceTree?.FilePath; | ||
|
||
var relativePath = locationFilePath?.Substring(baselineDirectory?.Length + 1 ?? 0).Replace("\\", "/"); | ||
var linePosition = location.GetLineSpan().StartLinePosition; | ||
var locationString = $"{relativePath}:{linePosition.Line + 1}:{linePosition.Character + 1}"; | ||
|
||
var baselineContent = baselineFile.GetText(context.CancellationToken)?.ToString(); | ||
if (baselineContent != null && baselineContent.Contains(locationString)) | ||
{ | ||
return; | ||
} | ||
} | ||
|
||
// Report the diagnostic | ||
context.ReportDiagnostic(Diagnostic.Create(Rule, location)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"profiles": { | ||
"Razor.Diagnostics.Analyzers": { | ||
"commandName": "Project" | ||
}, | ||
"Run on Razor.Compiler": { | ||
"commandName": "DebugRoslynComponent", | ||
"targetProject": "..\\..\\Compiler\\Microsoft.CodeAnalysis.Razor.Compiler\\src\\Microsoft.CodeAnalysis.Razor.Compiler.csproj" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
Mvc/PagesPropertyInjectionPass.cs:32:34 | ||
Mvc/PagesPropertyInjectionPass.cs:40:36 | ||
Language/Intermediate/IntermediateToken.cs:22:103 | ||
Language/Components/ComponentBindLoweringPass.cs:404:22 | ||
Language/Components/ComponentBindLoweringPass.cs:421:23 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having the line and column number is going to get really annoying when editing files if you have to edit this baseline file by hand. Do we: A) Just track the filename I lean towards b, because I think its valuable to have it tracked all the places its done, but don't have a super strong preference. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we track just the file name and text content of the line? |
||
Language/Components/ComponentBindLoweringPass.cs:591:57 | ||
Language/Components/ComponentBindLoweringPass.cs:812:40 | ||
Language/Components/ComponentBindLoweringPass.cs:835:40 | ||
Language/Components/ComponentBindLoweringPass.cs:841:40 | ||
Language/Components/ComponentBindLoweringPass.cs:850:40 | ||
Language/Components/ComponentBindLoweringPass.cs:871:36 | ||
Language/Components/ComponentBindLoweringPass.cs:880:40 | ||
Language/Components/ComponentBindLoweringPass.cs:895:40 | ||
Language/Components/ComponentBindLoweringPass.cs:901:40 | ||
Language/Components/ComponentBindLoweringPass.cs:910:40 | ||
Language/Components/ComponentBindLoweringPass.cs:916:40 | ||
Language/Components/ComponentBindLoweringPass.cs:922:40 | ||
Language/Components/ComponentBindLoweringPass.cs:929:36 | ||
Language/Components/ComponentBindLoweringPass.cs:951:35 | ||
Language/Components/ComponentBindLoweringPass.cs:960:39 | ||
Language/Components/ComponentBindLoweringPass.cs:970:39 | ||
Language/Components/ComponentBindLoweringPass.cs:978:35 | ||
Language/Components/ComponentBindLoweringPass.cs:1001:36 | ||
Language/Components/ComponentBindLoweringPass.cs:1010:40 | ||
Language/Components/ComponentBindLoweringPass.cs:1019:40 | ||
Language/Components/ComponentBindLoweringPass.cs:1025:40 | ||
Language/Components/ComponentBindLoweringPass.cs:1034:40 | ||
Language/Components/ComponentBindLoweringPass.cs:1040:40 | ||
Language/Components/ComponentBindLoweringPass.cs:1049:40 | ||
Language/Components/ComponentBindLoweringPass.cs:1055:40 | ||
Language/Components/ComponentBindLoweringPass.cs:1061:40 | ||
Language/Components/ComponentBindLoweringPass.cs:1068:36 | ||
Language/Components/ComponentBindLoweringPass.cs:1074:36 | ||
Language/Components/ComponentBindLoweringPass.cs:1082:40 | ||
Language/Components/ComponentBindLoweringPass.cs:1091:40 | ||
Language/Components/ComponentBindLoweringPass.cs:1098:36 | ||
Language/Components/ComponentBindLoweringPass.cs:1113:20 | ||
Language/Components/ComponentBindLoweringPass.cs:1121:20 | ||
Language/Components/ComponentBindLoweringPass.cs:1143:20 | ||
Mvc/ModelDirective.cs:61:24 | ||
Mvc/ModelDirective.cs:67:20 | ||
Mvc/ModelDirective.cs:71:20 | ||
Language/Extensions/AttributeDirectivePass.cs:33:35 | ||
Language/Components/ComponentLayoutDirectivePass.cs:37:13 | ||
Language/Components/ComponentLayoutDirectivePass.cs:38:13 | ||
Language/Components/ComponentLayoutDirectivePass.cs:39:13 | ||
Language/Components/ComponentEventHandlerLoweringPass.cs:181:13 | ||
Language/Components/ComponentEventHandlerLoweringPass.cs:193:13 | ||
Language/Components/ComponentEventHandlerLoweringPass.cs:263:28 | ||
Language/Components/ComponentEventHandlerLoweringPass.cs:273:28 | ||
Language/Extensions/DesignTimeDirectivePass.cs:42:25 | ||
Language/Extensions/DesignTimeDirectivePass.cs:53:25 | ||
Language/Extensions/DesignTimeDirectivePass.cs:64:25 | ||
Language/Components/ComponentRenderModeDirectivePass.cs:51:17 | ||
Language/Components/ComponentRenderModeDirectivePass.cs:63:28 | ||
Language/Components/ComponentRenderModeDirectivePass.cs:70:17 | ||
Language/Components/ComponentRenderModeDirectivePass.cs:81:17 | ||
Language/Components/ComponentRenderModeDirectivePass.cs:93:36 | ||
Mvc/ModelExpressionPass.cs:43:41 | ||
Mvc/ModelExpressionPass.cs:53:45 | ||
Mvc/ModelExpressionPass.cs:81:41 | ||
Language/Components/ComponentRuntimeNodeWriter.cs:1048:25 | ||
Language/Components/ComponentRuntimeNodeWriter.cs:1066:17 | ||
Language/Components/ComponentRuntimeNodeWriter.cs:1076:17 | ||
Mvc.Version2_X/PagesPropertyInjectionPass.cs:28:34 | ||
Mvc.Version2_X/PagesPropertyInjectionPass.cs:36:36 | ||
Mvc.Version2_X/AssemblyAttributeInjectionPass.cs:70:36 | ||
Language/Extensions/EliminateMethodBodyPass.cs:47:21 | ||
Language/Extensions/EliminateMethodBodyPass.cs:54:21 | ||
Language/Extensions/EliminateMethodBodyPass.cs:61:21 | ||
Language/Intermediate/BaseTypeWithModel.cs:14:24 | ||
Language/Intermediate/BaseTypeWithModel.cs:15:27 | ||
Language/Intermediate/BaseTypeWithModel.cs:16:25 | ||
Language/Intermediate/BaseTypeWithModel.cs:17:24 | ||
Language/Intermediate/BaseTypeWithModel.cs:30:24 | ||
Mvc.Version2_X/InstrumentationPass.cs:42:32 | ||
Mvc.Version2_X/InstrumentationPass.cs:55:30 | ||
Language/Extensions/ImplementsDirectivePass.cs:26:39 | ||
Language/Components/ComponentDesignTimeNodeWriter.cs:1181:25 | ||
Language/Components/ComponentDesignTimeNodeWriter.cs:1205:29 | ||
Language/Components/ComponentDesignTimeNodeWriter.cs:1224:17 | ||
Language/Components/ComponentDesignTimeNodeWriter.cs:1234:17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is better done in compilation start