Add a new analyzer + baseline for existing instances #11857
Conversation
- Adds a new analyzer that looks for creations of IntermediateToken with a type of CSharp - Adds a baseline file that covers all existing usages - Warns if any new usages are introduced - Adds a startup profile for the local analyzers that runs them on the compiler project
|
Do we have an issue / plan tracking this overall path in the compiler? |
No. Let me add one. |
|
Added an issue #11858 and updated the help link to that issue to explain it. |
| 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.
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
B) Add a fixer a la shipped APIs that allows you to automatically update the baseline?
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.
Could we track just the file name and text content of the line?
| } | ||
|
|
||
| // Resolve the TokenKind.CSharp enum value | ||
| var tokenKindType = context.Compilation.GetTypeByMetadataName("Microsoft.AspNetCore.Razor.Language.Intermediate.TokenKind"); |
There was a problem hiding this comment.
This is better done in compilation start
| .FirstOrDefault(init => | ||
| init.Target is IPropertyReferenceOperation property && | ||
| property.Property.Name == "Kind" && | ||
| init.Value.ConstantValue.HasValue && |
There was a problem hiding this comment.
It's safest to have this as init.Value.WalkDownConversion() (you can borrow it from here https://github.com/dotnet/roslyn-analyzers/blob/a5e9b9b92f25491429401a3e43914ad1ac161357/src/Utilities/Compiler/Extensions/IOperationExtensions.cs#L701-L714)
In cases of conversions, IIRC, you can only see the constant value, if any, from the conversion operation operand.
| 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.
This could be done very early at compilation start.
We have a class of bugs that occur because we are directly creating CSharp tokens in various parts of the lowering process. While it's tempting to do this for ease of implementation it means our code generation logic is smeared across both lowering and generation and often means that a fix to one part of generation doesn't fix it for something else.
Eventually I would like to get to a place where the only place we directly write C# is in the code generation phase, and lowering just remaps higher level constructs into lower ones that the writer knows how to emit.
There are lots of places in our code that do this today though, so rather than boil the ocean this adds an analyzer that will flag any new usages of the pattern so we don't make the world worse.
Note a good chunk of this was generated via CoPilot so close scrutiny of the analyzer code is appreciated for ways it can be improved (I don't know the analyzer space all that well).