RFR: 8015831: Add lint check for calling overridable methods from a constructor [v11]

[Archie L. Cobbs]

On Tue, 17 Jan 2023 11:44:22 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> Yes, other clients might be able to access the hashmap, and observe a possibly uninitialized value - but this is in the same bucket as the factory example I gave last week, where a private constructor (called by public factory) was leaking this via a static method.

It's similar but slightly different - in [your earlier example](https://github.com/openjdk/jdk/pull/11874#discussion_r1069250348), there was no chance a subclass was involved, so no leak was possible (according to our definition of a leak, which requires a subclass involvement). In this `Bundle` example, a subclass could be involved so the warning is generated. But that's just a technicality - I get what you're saying.

> Given this, I believe the above case is a "false positive". The problem seems to be that we don't know what the code for HashMap::put is, so we cannot know if that code might end up calling some virtual method on our this - and so we warn. Correct?

Yes, although it's a "false positive" according to our defined semantics... well, to be clear: it's a false positive in that there is actually no bug there, but it's not a false positive in the sense that an actual "leak" is occurring, based on our definition of "leak".

For example, suppose the subclass of `Bundle` happens to implement `Predicate` and `HashMap::put` happens to invoke `test()` on any values that implement `Predicate`. Because our semantics say to treat `HashMap::put` like a black box and make no assumptions on its behavior we have to admit this unlikely scenario.

But I'm being technical. What programmer's actually care about is how often the warning actually helps them prevent an _actual or probable bug_, not whether it meets some technical definition.

> the only way for Map::put to leverage some virtual method in this (whose type is Bundle) is if if the map implementation can access Bundle

Except for the subclass implementing some interface and `HashMap` casting for it.

> As I said, a bit sad, because this idiom is frequent...

I am in agreement with your general sentiment. In short, we want a higher ratio of "actual bug possibilities" to "really obscure and unlikely bug possibilities".

Based on this and our other discussions I'm thinking that, at least for the first round, we should be less aggressive. Then later, as we get more experience with the warning over time, we can tighten the screws if needed.

An easy way to be less aggressive is to draw a more conservative "maintenance boundary", which is the area inside of which we don't care about 'this' escape bugs.

In the current implementation, the maintenance boundary = the compilation unit (i.e., the source file `X.java`). So to limit ourselves to possible bugs we do care about, we ask: Is X not final, not sealed (or, sealed but permitting some class defined outside of X.java), and does X have a non-private constructor?

Instead, we could widen the maintenance boundary to the package/module boundary, and instead ask: Is X public, not final, not sealed, and does X have a non-private constructor?

This is not a perfect drawing of that boundary, because it ignores which packages are exported in a module, but that error is a conservative one, and it avoids tricky issues with how files are compiled.

This would eliminate your false positive example above, albeit not for the reasons you state, but rather simply because `Bundle` is package-private. From previous test, it reduces warnings in the JDK by 36%.

Your thoughts?

-------------

PR: https://git.openjdk.org/jdk/pull/11874