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

[Archie L. Cobbs]

On Thu, 12 Jan 2023 10:25:27 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Archie L. Cobbs has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Use the more appropriate Type comparison method Types.isSameType().
>>  - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 218:
> 
>> 216:         new TreeScanner() {
>> 217: 
>> 218:             private Lint lint = ThisEscapeAnalyzer.this.lint;
> 
> On a first look I'm not sure about the granularity of suppression here. I believe that suppressing at the class level, or at the constructor level is enough. Allowing to further annotate var declarations and non-constructor methods, while doable, might actually be counter productive - in the sense that the annotation does not occur in the constructor (where you'd want to see it) but in some other method. I think the fact that a constructor is escaping (willingly) should be a visible thing. Something to consider.

Two things...

We need to support annotations on field declarations because their initializers are effectively mini-constructors. But we don't need it on local variables, parameters, etc. - too confusing.

For annotations on methods, yes it's debatable. It can be handy if you have e.g. an `init()` method that all of your constructors invoke. However, I agree it's also confusing, so I will remove.

But we should keep the notion that if a constructor invokes `this()`, and the invoked constructor has annotations suppressed, then we skip over the constructor invocation.

I will make these updates.

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 304:
> 
>> 302: 
>> 303:             // We are looking for analyzable constructors only
>> 304:             final Symbol sym = entry.getKey();
> 
> This seems unused. And, if so, perhaps we only need a `Set<MethodInfo>`, not a map.

Thanks, you're right, the map keys are not used here. I'll clean up the loop.

But we still need a map, not a set, because the map is used elsewhere.

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

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