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.

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

Add diagnostic for Dictionary.ContainsKey when used in an ? statement. #35854

Closed
AraHaan opened this issue May 5, 2020 · 9 comments
Closed
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections code-analyzer Marks an issue that suggests a Roslyn analyzer
Milestone

Comments

@AraHaan
Copy link
Member

AraHaan commented May 5, 2020

Analyzer package

Microsoft.CodeAnalysis.FxCopAnalyzers

Package Version

v3.0.0 (Latest)

Diagnostic ID

None yet that I know of. I would like one added that lets users know that Dictionary.ContainsKey(key) ? Dictionary[key] : null is a double lookup and can be simplified to Dictionary.TryGetKey(key, out var value) ? value : null. Because of the lack of a diagnostic for this I did not know about it according to my usage on it.

Repro steps

  1. Well it is as simple as writing a dictionary that checks for if keys are added and wanting to get the value, and if it has no value return null on the method / statement like I wrote above.
  2. Notice that currently no diagnostic is reported to the user so they could reduce the amount of lookups and increase performance hits.

Expected behavior

A diagnostic for this that alerts the user of the possible performance hits from this example above.

Actual behavior

No diagnostic explaining all of this is reported. Probably because there is none yet that exists?

@mavasani
Copy link

mavasani commented May 5, 2020

This is in a similar bucket as #35343. @AraHaan can you please move this issue to runtime repo?

@AraHaan
Copy link
Member Author

AraHaan commented May 5, 2020

ye, um it seems that issue before was moved there without recreating it, any way to do the same here as it seems to not let me move it there?

@mavasani
Copy link

mavasani commented May 5, 2020

@stephentoub @jeffhandley to help transfer the issue to runtime repo.

@stephentoub stephentoub transferred this issue from dotnet/roslyn-analyzers May 5, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Collections untriaged New issue has not been triaged by the area owner labels May 5, 2020
@ghost
Copy link

ghost commented May 5, 2020

Tagging subscribers to this area: @eiriktsarpalis
Notify danmosemsft if you want to be subscribed.

@AraHaan
Copy link
Member Author

AraHaan commented May 9, 2020

Any updates on this one?

@jeffhandley jeffhandley added api-suggestion Early API idea and discussion, it is NOT ready for implementation code-analyzer Marks an issue that suggests a Roslyn analyzer and removed untriaged New issue has not been triaged by the area owner labels May 16, 2020
@jeffhandley jeffhandley added this to the Future milestone May 16, 2020
@jeffhandley
Copy link
Member

Hi @AraHaan. Thanks for filing this suggestion. This has been tagged to be considered as a possible future analyzer that we could implement. We've already selected the analyzers we're investing in for .NET 5 though, so this is marked as Future. We will need to further review this analyzer concept before we can mark it as approved though, and I don't yet have a timeline for when we'll be reviewing the backlog for planning beyond .NET 5.

If you would be interested in thinking through possible call site samples, showing those possible scenarios and edge cases, and illustrating with code snippets, that would help us though, and we then might even be able to mark this as up-for-grabs.

Thanks!

@stephentoub
Copy link
Member

Isn't this a dup of #33798?

@AraHaan
Copy link
Member Author

AraHaan commented May 17, 2020

oh shoot ye you are right it is possibly a duplicate.

@layomia
Copy link
Contributor

layomia commented Oct 28, 2020

Closing as dup of #33798.

@layomia layomia closed this as completed Oct 28, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections code-analyzer Marks an issue that suggests a Roslyn analyzer
Projects
None yet
Development

No branches or pull requests

6 participants