Skip to content

ExplicitCollectionElementAccessMethod: Do not report map?.get("foo") #2391

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

Closed
xouabita opened this issue Mar 3, 2020 · 6 comments · Fixed by #2401
Closed

ExplicitCollectionElementAccessMethod: Do not report map?.get("foo") #2391

xouabita opened this issue Mar 3, 2020 · 6 comments · Fixed by #2401

Comments

@xouabita
Copy link
Contributor

xouabita commented Mar 3, 2020

The ExplicitCollectionElementAccessMethod rule seems really useful but it looks like it is also reporting safe map element access:

https://github.com/arturbosch/detekt/blob/master/detekt-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ExplicitCollectionElementAccessMethodSpec.kt#L34

Is it possible to not report the following ?

var map: Map<String, Int>? = null
map?.get("foo") 

Or is there an elegant way to fix the issue with the current ?

This fixes it, but it doesn't seems optimal...

map?.let { it["foo"] }
@BraisGabin
Copy link
Member

This seems like a false positive. And map?.let { it["foo"] } is worst than map?.get("foo").

@schalkms
Copy link
Member

schalkms commented Mar 3, 2020

But this access isn’t save.
It still returns null for a key which hasn’t been found in the map.

@schalkms schalkms closed this as completed Mar 3, 2020
@schalkms schalkms added the rules label Mar 3, 2020
@xouabita
Copy link
Contributor Author

xouabita commented Mar 3, 2020

I am not sure I follow.... This rule is not about null safety but about style:

For now, with this rule enabled, we cannot do map?.get("foo") and we need to write map?.let { it["foo"] } which looks like a false positive

@schalkms
Copy link
Member

schalkms commented Mar 3, 2020

Okay, now I understand. I'll reopen it in a second.
@xouabita do you have resources to tackle this?

@schalkms schalkms reopened this Mar 3, 2020
@xouabita
Copy link
Contributor Author

xouabita commented Mar 3, 2020

Yes, I can have a look

@schalkms
Copy link
Member

schalkms commented Mar 3, 2020

Cool, thanks for helping out! 🙂

schalkms pushed a commit that referenced this issue Mar 4, 2020
…ction (#2401)

* ExplicitCollectionElementAccessMethod: Don't report on nullable collection

* Remove unused import

Fixes #2391
@arturbosch arturbosch added this to the 1.7.0 milestone Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants