Extract 'use var' from 'simplify type names'#40535
Conversation
06bf10d to
7ece386
Compare
There was a problem hiding this comment.
called throuh two codepaths. one is only called from SimplifyTypeNames. The other is called through a codepath from simplify type names and by CSharpNameReducer. So i removed this entirely from the simplification codepaths, but then added into the reduction codepath with a specialized reducer just for this purpose.
7ece386 to
42e771c
Compare
|
@dotnet/roslyn-ide This is ready for review. Not hte open question in the original post. |
| public override bool IsApplicable(OptionSet optionSet) | ||
| => optionSet.GetOption(CSharpCodeStyleOptions.VarForBuiltInTypes).Value || | ||
| optionSet.GetOption(CSharpCodeStyleOptions.VarWhenTypeIsApparent).Value || | ||
| optionSet.GetOption(CSharpCodeStyleOptions.VarElsewhere).Value; |
There was a problem hiding this comment.
don't bother running us if the user never even wants 'var'.
|
tagging @dpoeschl @jinujoseph can this get a buddy? |
They feel to me like things that should be presented as independent. Tying them together would be to make the assumption that a fix for one analyzer/preference should not produce code that another analyzer/preference would have an opinion on. That assumption wouldn't match the general picture, would it? |
| // ever want the simplifier to produce 'var' in the 'Simplify type | ||
| // names' fixer. only the 'Use var' fixer should produce 'var'. | ||
| var annotatedexpressionSyntax = expressionSyntax.WithAdditionalAnnotations( | ||
| Simplifier.Annotation, Formatter.Annotation, DoNotAllowVarAnnotation.Annotation); |
There was a problem hiding this comment.
note: the fact that SimplifyTypeNames works by acutally simplifying during analysis, and then doing and just rerunning the full simplifier is pretty awful. I intend to clean this up a lot in a later PRs. For now, this is a nice and simple way to say: SimplifyTypeNames should not produce var, even if it thinks it can simplify the name of something.
|
|
||
| namespace Microsoft.CodeAnalysis.CSharp.Simplification | ||
| { | ||
| internal partial class CSharpVarReducer |
There was a problem hiding this comment.
this is now a nice and tiny type with the isolated logic to try to convert a TypeSyntax to 'var' during normal simplification (i.e. the Simplification subsystem, not the SimplifyTypeNames feature). It's single-purpose and easy to understand, and isn't just interspersed randomly into other parts of the infrastructure.
| internal partial class CSharpTypeStyleHelper | ||
| { | ||
| protected class State | ||
| protected struct State |
There was a problem hiding this comment.
An instance of this is produced for every node we want ot try to simplify/expand 'var' for. No need to allocate here.
Agreed. Thanks for the feedback! |
|
I have no issue with proposal 2 (simplify type names does not consider 'var' as a simplified name). However, we need to ensure that |
We have test for this everywhere. For example: There are tests in convert-foreach-to-for (and back), in 'introduce-local', convert-to-conditional. And we just have TypeNameSimplifierTests which tests precicely this :) |
|
@sharwell can we move forward with this? thanks! |
|
@sharwell perf numbers: |
|
@sharwell can we move forward with this? Thanks! |
|
Thanks! |
Fixes #27819
Note: the problem is as follows: If you have the
use varsetting enabled, then both theUse varanalyzer and theSimplify type namesanalyzer fire for types and offer to simplify tovar.For example: If you write
System.Int32you will get two diagnostics:Both these offer the exact same thing. This PR separates out the code from simplify-type-names so it never does 'var' replacement. That's at least better than our current state. However, there's still a slight open question on what we want the experience to be.
Specifically, in this case:
use var, and not offersimplify type namesat all? oruse varand offersimplify type nameswith the simplification producingint.I've taken the approach of
2under the thinking that both are valid and offer two separate changes the user might want in a case like this. However, i can be convinced that '1' is the approach we should go.--
Note: this is also part of a larger effort i want to take to improve the performance of hte "simplify type names" feature. Right now it's super slow, and i'm trying to make it more amenable to optimizing by simplifying and reducing what it does.