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

Replace Dictionary<,>.Keys.Contains with ContainsKey #35343

Closed
drewnoakes opened this issue Apr 1, 2020 · 19 comments · Fixed by dotnet/roslyn-analyzers#4687
Closed

Replace Dictionary<,>.Keys.Contains with ContainsKey #35343

drewnoakes opened this issue Apr 1, 2020 · 19 comments · Fixed by dotnet/roslyn-analyzers#4687
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Milestone

Comments

@drewnoakes
Copy link
Member

drewnoakes commented Apr 1, 2020

This is a proposal for a new analyzer/codefix for:

// given
Dictionary<int, string> dic = ...;

dic.Keys.Contains(1);
dic.Values.Contains("");

// Could instead be
dic.ContainsKey(1);
dic.ContainsValue("");

I found this code in some design-time build targets today.

EDIT I'd previously incorrectly stated that the former form was O(N), but System.Linq.Enumerable.Contains does type test for ICollection<> and delegate to ICollection<>.Contains, which is also O(1). There is some overhead associated with that however, which could be avoided.

@mavasani
Copy link
Member

mavasani commented Apr 2, 2020

@jeffhandley @terrajobst want to move this proposal to runtime repo under your radar?

@Evangelink
Copy link
Member

I can take care of this implementation if you wish. I am wondering if we could somehow improve the banned API analyzer to allow catching such cases, basically you would want to forbid a specific method call in some specific context...

@drewnoakes
Copy link
Member Author

basically you would want to forbid a specific method call in some specific context...

In this case I think it would be enough to set the default diagnostic level to warning.

@Evangelink
Copy link
Member

@drewnoakes Do you think there are cases where it would make sense to call Contains on a Dictionary<TKey,TValue>.KeyCollection or Dictionary<TKey,TValue>.ValueCollection? If not maybe the easiest is to actually ban System.Collections.Generic.Dictionary<TKey,TValue>.KeyCollection.Contains(TKey) and System.Collections.Generic.Dictionary<TKey,TValue>.ValueCollection.Contains(TValue). WDYT?

Tagging @mavasani for a second opinion.

@drewnoakes
Copy link
Member Author

The banned API analyzer tends to be for banning APIs that are specific to a given code base. I don't think any code base has a use case for calling either of those methods. It may be possible to ship a framework-specific analyzer that's based on the banned API analyzer and bundles a known set of APIs, such as these.

@Evangelink
Copy link
Member

@drewnoakes My idea behind the use of the "Banned API" analyzer was to provide a way where we won't have to implement a new analyzer for each new member call we want to prevent. At the same time, the code in itself is pretty easy to write so there might be no point in having an infrastructure in place.

@Evangelink
Copy link
Member

@drewnoakes @mavasani Do you think that's better to report only on specific dictionary instances or also on the interface? Reporting also on the interface might catch more cases but could potentially lead to FP for custom implementations of the dictionary. I think the risk is pretty low so I would be in favor of catching everything that implements IDictionary<TKey, TValue>/IReadOnlyDictionary<TKey, TValue>. WDYT?

@Evangelink
Copy link
Member

In addition to my previous comment, I am wondering if we should extend the rule to also cover Count(). For IDictionary and Dictionary that seems to be fine because Keys returns KeyCollection with the property so there is another rule kicking-in to say use the property instead. But for Keys where the type is IEnumerable there is no rule reporting.

@mavasani
Copy link
Member

@Evangelink This issue likely needs to be moved to https://github.com/dotnet/runtime with code-analyzer label and would need an approval on the design and/or implementation.

@jeffhandley @bartonjs @stephentoub Can one of you please port this issue OR give an approval on the proposal here?

@stephentoub stephentoub transferred this issue from dotnet/roslyn-analyzers Apr 23, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Apr 23, 2020
@stephentoub stephentoub added api-suggestion Early API idea and discussion, it is NOT ready for implementation code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer and removed untriaged New issue has not been triaged by the area owner labels Apr 23, 2020
@Evangelink
Copy link
Member

Sorry for the rush on implementing something! To be honest, I am a little lost with which issue is going where. As an external user, it feels more natural to have all of the analyzers in one place instead of having some in vs-threading some here, some in roslyn and others in roslyn-analyzers. Just my 2 cents.

@ghost
Copy link

ghost commented Apr 24, 2020

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

@bartonjs bartonjs added this to To do in Roslyn Analyzers Apr 24, 2020
@bartonjs bartonjs added this to the Future milestone Apr 24, 2020
@bartonjs
Copy link
Member

It's a reasonable suggestion in the Performance category.

Because Dictionary's KeyCollection and ValueCollection types are lazy, there's not a whole lot of perf difference from the concrete type. The constructors just save a reference to the dictionary and the Contains methods forward to ContainsKey/ContainsValue (as appropriate).

dict.Keys.Contains(foo);

is roughtly equivalent to

new object();
dict.ContainsKey(foo);

though the latter example does need to work in two more dereference/invocations to add the extra couple of CPU cycles to the mix.

Is there a good way to avoid hard-coding specific dictionary types here?

  • Invocation of a single-parameter method named "Contains" that returns a Boolean.
  • The parent expression is a property invocation of IDictionary<TKey,TValue>.Keys (or Values) or IReadOnlyDictionary<TKey,TValue>.Keys (or Values) where TKey or TValue (as appropriate) is the (exact) same type (after generic substitution) as the parameter for the Contains method
  • Fixer replaces a.Keys.Contains(b) with a.ContainsKey(b), and c.Values.Contains(d) with c.ContainsValue(d)

It becomes a lot more valuable when it's through the interfaces, since the Contains method would likely be the item-walking version from System.Linq.Enumerable.

The trickiest case is probably this helper method:

private static bool IsRelevant<T>(T candidate, IReadOnlyDictionary<string, T> context)
{
    return context.Values.Contains(candidate);
}

since it involves an analysis with a partially bound generic (it's probably not really any trickier than a concrete example, but generic type parameter symbols may have some interesting quirks to them in the symbol tree).

This would probably be an IDE suggestion that we may kick up to a bigger warning via TFM opt-in.

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented help wanted [up-for-grabs] Good issue for external contributors and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 28, 2020
@Mrnikbobjeff
Copy link

Mrnikbobjeff commented Aug 30, 2020

I would like to have this assigned to me. I just completed my work on this analyzer including the code fix provider. One flaw still unadressed is the fact that only Dictionary<T,K> has a ContainsValue method, the interfaces do not. I would suggest taking the simple route and only apply ContainsValue if we know the member which is accessed is a Dictionary<T,K>. This is the way I implemented it.

@carlossanlop carlossanlop moved this from Approved-Future to In progress in Roslyn Analyzers Feb 4, 2021
@carlossanlop carlossanlop removed the help wanted [up-for-grabs] Good issue for external contributors label Feb 4, 2021
@carlossanlop
Copy link
Member

@NewellClark thanks for starting working on this. I'm trying to assign to you but I can't for some reason, but consider it yours.

@Mrnikbobjeff apologies for not assigning this to you when you requested it, but your offer is appreciated. We have many more analyzers marked as up-for-grabs and would love to get your help on those others too.

@jeffhandley
Copy link
Member

@NewellClark We figured out why we cannot assign this to you. You must comment on the issue before you show up in the assignees list.

@NewellClark
Copy link
Contributor

@jeffhandley Gotcha.

@stephentoub
Copy link
Member

Can this be closed (dotnet/roslyn-analyzers#4687)?

@buyaa-n
Copy link
Member

buyaa-n commented Aug 17, 2021

Yes, closing

@buyaa-n buyaa-n closed this as completed Aug 17, 2021
@buyaa-n buyaa-n moved this from In progress to Done in Roslyn Analyzers Aug 17, 2021
@dotnet dotnet locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Projects
No open projects