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

ModifyCollectionInEnhancedForLoop is triggered for ConcurrentMap #1767

Closed
ejona86 opened this issue Aug 3, 2020 · 0 comments · Fixed by #2070
Closed

ModifyCollectionInEnhancedForLoop is triggered for ConcurrentMap #1767

ejona86 opened this issue Aug 3, 2020 · 0 comments · Fixed by #2070

Comments

@ejona86
Copy link
Contributor

ejona86 commented Aug 3, 2020

Description of the problem / feature request:

ModifyCollectionInEnhancedForLoop is triggered for ConcurrentMap. While this could be on purpose, as technically ConcurrentHashMap defines the no-throw-ConcurrentModificationException policy, it seems this is accidental in some way as ConcurrentMap is part of java.util.concurrent which is skipped by the check.

Bugs: what's the simplest, easiest way to reproduce this bug?

We have the code:

  private final ConcurrentMap<Locality, ClientLoadCounter> localityLoadCounters;
...
    for (Map.Entry<Locality, ClientLoadCounter> entry : localityLoadCounters.entrySet()) {
        ...
        localityLoadCounters.remove(entry.getKey());

Which causes a ModifyCollectionInEnhancedForLoop warning:

grpc-java/xds/src/main/java/io/grpc/xds/LoadStatsStoreImpl.java:94: warning: [ModifyCollectionInEnhancedForLoop] Modifying a collection while iterating over it in a loop may cause a ConcurrentModificationException to be thrown or lead to undefined behavior.
        localityLoadCounters.remove(entry.getKey());
                                   ^
    (see https://errorprone.info/bugpattern/ModifyCollectionInEnhancedForLoop)

Swapping from ConcurrentMap to ConcurrentHashMap silences the warning.

Looking at the code, there does not seem to be any ConcurrentMap vs ConcurrentHashMap special-casing, just a large special-case for java.util.concurrent:

if (state.getTypes().closure(ASTHelpers.getSymbol(tree).enclClass().asType()).stream()
.anyMatch(
s ->
s.asElement()
.packge()
.getQualifiedName()
.toString()
.startsWith("java.util.concurrent"))) {
return NO_MATCH;
}

It's unclear how ConcurrentMap triggers the warning.

What version of Error Prone are you using?

2.4.0

Have you found anything relevant by searching the web?

No.

copybara-service bot pushed a commit that referenced this issue Jan 7, 2021
…cedForLoop

look at the receive type of an invocation, instead of the type the method
is defined in. E.g. when looking at `map.remove` where `map` is a,
`ConcurrentMap`, we now consider `ConcurrentMap` even though it inherits
`remove` from regular `Map`.

Fixes #1767

PiperOrigin-RevId: 350593573
stevie400 pushed a commit to HubSpot/error-prone that referenced this issue Jan 15, 2021
…cedForLoop

look at the receive type of an invocation, instead of the type the method
is defined in. E.g. when looking at `map.remove` where `map` is a,
`ConcurrentMap`, we now consider `ConcurrentMap` even though it inherits
`remove` from regular `Map`.

Fixes google#1767

PiperOrigin-RevId: 350598002
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant