Fix IgnoredReturnValue rule crash in parallel mode

[schalkms]

IgnoredReturnValue crashes due to the usage of a non-thread-safe collection operation. The usage of Iterable<T>.plus() causes this problem. The crash in parallel mode is fixed by iterating over each collection by itself.

See here for more details in detekt v1.21:

detekt/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/IgnoredReturnValue.kt

Line 90 in 32f6e22

(annotations + resultingDescriptor.containingDeclaration.annotations).none { it in returnValueAnnotations }

Closes #5403

[schalkms]

I also checked with find usages whether detekt's code base contains related issues. No further issues have been found.

The questions is how we can prevent issues like this happening again in parallel mode.
I can also open a separate discussion if necessary.

[cortinico]

The questions is how we can prevent issues like this happening again in parallel mode.

We do have ConTester in the codebase. @davidburstromspotify helped up set it up.
You should be able to stress test this scenario as well

[schalkms]

@cortinico my question aimed for preventing such issues in general in the future. I’m not sure how a potential solution for a test could like.
For the scenario, which has been fixed with this PR, there’s no mutable collection/instructions anymore. If you look at the stack trace in the linked issue, you see that the code causing this crash is not executed anymore.

[davidburstromspotify]

@cortinico ConTester unfortunately doesn't give you the required granular control in methods that are getting invoked, only in your own method implementations.

[cortinico]

Thanks for the insights. I think we're safe shipping this specific fix. As for preventing those kind of issues in the future, other than stress testing our rules with --parallel on CI, I don't have other ideas 🤔

[schalkms]

hink we're safe shipping this specific fix. As for preventing those kind of issues in the future, other than stress testing our rules with --parallel on CI, I don't have other ideas

I like your idea. It is worthwhile to proceed. 👍

[schalkms]

Still interesting that the line of code causing this crash was introduced 15 months ago and that only a few days the first issue appeared pointing to this line of code.

[BraisGabin]

I'm still puzzled by why that code is not parallel safe. Those are list provided by the PSI. Are those list changing? Is this related with the auto-correct feature?

[davidburstromspotify]

I'm guessing wildly here, but if I read the stack trace correctly, the LazyJavaAnnotations is generated when calling plus, and if there are two references to LazyJavaAnnotations being addressed in parallel, it will crash. This could then be fixed by having a thread safe facade to LazyJavaAnnotations.