RFR: 8325805: Compiler Implementation for Flexible Constructor Bodies (Second Preview) [v5]

[Archie Cobbs]

On Wed, 8 May 2024 08:25:41 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> Archie Cobbs has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 20 commits:
>> 
>>  - Allow declaring and instantiating local classes prior to super().
>>  - Merge branch 'master' into JDK-8325805
>>  - Merge branch 'master' into JDK-8325805
>>  - Disallow assignments to fields with initializers in constructor prologues.
>>  - Merge branch 'master' into JDK-8325805
>>  - Merge branch 'master' into JDK-8325805
>>  - Fix cut & paste of bug ID and summary.
>>  - Avoid possible unwanted side effects from method Symbol.flags().
>>  - Add a few more test cases.
>>  - Fix bug where early assignments in lambda's were being allowed.
>>  - ... and 10 more: https://git.openjdk.org/jdk/compare/9b423a85...7e6a6a0d
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java line 3778:
> 
>> 3776:                     if (staticOnly)
>> 3777:                         sym = new StaticError(sym);
>> 3778:                     else if (env1.info.ctorPrologue && sym.kind == VAR && !isAllowedInPrologue(env1, (VarSymbol)sym))
> 
> Question: why `sym.kind == VAR`? Can `sym` be something else then a variable here (given `name` can only be `this` or `super`, unless I am mistaken)?

@lahodaj,

Thanks for your review.

> Can sym be something else then a variable here (given `name` can only be `this` or `super`)?

It's true that `name` can only be `this` or `super` - I've added an assertion to that effect in 887699b4085.

It's probably also true that `sym` has to be a `VarSymbol` but that was not immediately provable just from looking at the code. I've been burned before by making such assumptions (e.g., when things like `ResolveError` can sometimes pop up) so I'm playing it safe here.

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java line 3853:
> 
>> 3851: 
>> 3852:         // The symbol must appear in the LHS of an assignment statement
>> 3853:         if (!env.tree.hasTag(ASSIGN))
> 
> I think you should be able to use pattern matching, to avoid the manual cast below. Something like:
> Suggestion:
> 
>         if (!(env.tree instanceof JCAssign at))

I agree in principle, however the code base uses `hasTag()` instead of `instanceof` everywhere - presumably for a good (although possibly obsolete) reason - so I'm just being conservative/consistent.

As a separate discussion, it's worth considering getting rid of 'tags' in a giant wholesale cleanup... maybe someone could write a clever script to make the changes automatically. But first someone would need to confirm that what originally motivated this quirk no longer applies.

Personally I've always found that `hasTag()` instead of `instanceof` just makes the code harder to read. These days I'd guess it's probably not saving any time either.

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java line 3865:
> 
>> 3863: 
>> 3864:         // The symbol must not be an instance field inherited from a superclass
>> 3865:         if (v.owner.kind == TYP &&
> 
> The conditions here are eerily similar to those in `checkAssignable`. I wonder if there's a way to unify the codepaths, so that we only have these conditions on one place.

Good idea - there can indeed be some consolidation - added in b80e7527ed6.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18088#discussion_r1594430682
PR Review Comment: https://git.openjdk.org/jdk/pull/18088#discussion_r1594430853
PR Review Comment: https://git.openjdk.org/jdk/pull/18088#discussion_r1594430775