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.

Sign up for GitHub

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

Jump to bottom

CA1854 Improve suggested variable name and/or type of the fixer #6022

Open
1 of 2 tasks
buyaa-n opened this issue Jun 8, 2022 · 5 comments · May be fixed by #7261
Open
1 of 2 tasks

CA1854 Improve suggested variable name and/or type of the fixer #6022

buyaa-n opened this issue Jun 8, 2022 · 5 comments · May be fixed by #7261
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design help wanted The issue is up-for-grabs, and can be claimed by commenting

Comments

@buyaa-n
Copy link
Member

buyaa-n commented Jun 8, 2022 •
edited

Analyzer

Diagnostic ID: CA1854 Prefer the 'IDictionary.TryGetValue(TKey, out TValue)' method

Describe the improvement

The fixer now suggests to use TryGetValue(key, out var value), some repos prefer concrete type over var, it would be great to add some improvements to resolve this. Also the variable name value might already defined within the scope, it could suggest a bit modified name like value1 when a variable named value is already defined within the scope

Describe suggestions on how to achieve the rule

  • Analyzer could try to determine the concrete type of the value and add another option for fixer suggestion: one option with var, another with the concrete type
  • When a variable named value is already defined within the scope suggest a bit modified name like value1 etc

Additional context

Related to dotnet/runtime#70157 (comment)
@buyaa-n buyaa-n added the help wanted The issue is up-for-grabs, and can be claimed by commenting label Jun 8, 2022
@buyaa-n buyaa-n mentioned this issue Jun 8, 2022
Merged
@Youssef1313
Copy link
Member

I think Roslyn will apply CSharpVarReducer after running a codefix. This reducer converts explicit types with var if the user prefers that. So the codefix should use explicit type, and the result could be magically turned into var by CSharpVarReducer. For this reducer to work, I think the node should have a simplifier annotation.

@buyaa-n
Copy link
Member Author

buyaa-n commented Jun 9, 2022

I think Roslyn will apply CSharpVarReducer after running a codefix. This reducer converts explicit types with var if the user prefers that. So the codefix should use explicit type, and the result could be magically turned into var by CSharpVarReducer.

Interesting, if that is the case suggesting only explicit type sounds good to me

For this reducer to work, I think the node should have a simplifier annotation.

Did not understood this part, when the reducer would convert to var?

@Youssef1313
Copy link
Member

Did not understood this part, when the reducer would convert to var?

I'll respond per my understanding of Roslyn code I looked at:

PostProcessChangesAsync → CleanupDocumentAsync -> ReduceAsync

https://github.com/dotnet/roslyn/blob/b6d630f4132e6547412138532fa8e1ca6769eb76/src/Workspaces/Core/Portable/CodeActions/CodeAction.cs#L306-L327

https://github.com/dotnet/roslyn/blob/b6d630f4132e6547412138532fa8e1ca6769eb76/src/Workspaces/Core/Portable/CodeActions/CodeAction.cs#L329-L346

https://github.com/dotnet/roslyn/blob/b6d630f4132e6547412138532fa8e1ca6769eb76/src/Workspaces/Core/Portable/Simplification/Simplifier.cs#L181-L185

Then it ends up here:

https://github.com/dotnet/roslyn/blob/b6d630f4132e6547412138532fa8e1ca6769eb76/src/Workspaces/Core/Portable/Simplification/AbstractSimplificationService.cs#L49-L93

and finally ReduceCoreAsync will run a list of reducers. This list is defined here:

https://github.com/dotnet/roslyn/blob/b6d630f4132e6547412138532fa8e1ca6769eb76/src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.cs#L27-L39

and here is the reducer implementation:

https://github.com/dotnet/roslyn/blob/b6d630f4132e6547412138532fa8e1ca6769eb76/src/Workspaces/CSharp/Portable/Simplification/Reducers/CSharpVarReducer.Rewriter.cs

https://github.com/dotnet/roslyn/blob/b6d630f4132e6547412138532fa8e1ca6769eb76/src/Workspaces/CSharp/Portable/Simplification/Reducers/CSharpVarReducer.cs

In short:

  1. If we always generate TypeSyntax that has Simplifier.Annotation, the var reducer will run.
  2. var reducer will change TypeSyntax to var if the user has .editorconfig options that indicate he likes var. The options available are csharp_style_var_for_built_in_types, csharp_style_var_when_type_is_apparent, and csharp_style_var_elsewhere.

@Youssef1313 Youssef1313 mentioned this issue Jun 9, 2022
Merged
@Youssef1313
Copy link
Member

@buyaa-n I got a PR ready. #6023

@Youssef1313
Copy link
Member

The reamining work here is:

When a variable named value is already defined within the scope suggest a bit modified name like value1 etc

This was reported in #6287

@Youssef1313 Youssef1313 added Bug The product is not behaving according to its current intended design Area-Microsoft.CodeAnalysis.NetAnalyzers labels Dec 8, 2022
@Youssef1313 Youssef1313 mentioned this issue Dec 8, 2022
Closed
@mpidash mpidash linked a pull request Mar 22, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design help wanted The issue is up-for-grabs, and can be claimed by commenting
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

CA1854: Use unused variable name for out parameter mpidash/roslyn-analyzers
2 participants
@Youssef1313 @buyaa-n