On Thu, 28 May 2026 13:57:59 GMT, Maurizio Cimadamore <[email protected]> wrote:
> This PR moves the early access checks back to `Resolve` from `Attr`. > > This simplifies the code, as we no longer need a separate visitor to check > for early references. > > To do this, the code uses a simple record called `EarlyConstructorContext` > which is used to keep track "facts" about where we are in attribution with > respect to early initialization. This means tracking: > * whether we need to issue warnings or errors > * whether we are inside a lambda, or a nested class > * who is the owner of the early construction context > > An instance of this class is stored in `AttrContext`, generalizing the > current `ctorPrologue` field. `Attr` updates this field -- so, for instance, > inside `visitLambda` a "nested" early context is derived and stored in > `AttrContext`. > > The most intricate bit has to do with how to handle stuff like `Foo.this.x`. > There are many complected issues here: > * when we see `Foo.this` we don't know if `x` is a method or a field (so we > might report issues too early) > * when, later, we see `x` in Resolve, we don't know if the qualifier was > `Foo.this` (so we might miss some issues). > > To overcome these problems, some coupling between `Attr` and `Resolve` is > unavoidable. In this PR, `Attr::visitSelect` checks whether the qualifier > happens to be `this/super` or `Foo.this/super`. If so, it _unsets_ the early > construction context. Effectively this means that `Resolve` will NOT report > an error. > > Then, in such cases, `Attr` will also pass the `selected` part to `selectSym` > which will pass it to `Resolve::findIdentInType`. This means that, at this > point, we can finally check `x`, but knowing that the qualifier was > `Foo.this`. > > Another improvement is that there's no longer duplication between `Resolve` > and `Attr::checkAssignable`. This duplication is present in mainline, but the > current lworld repo (thanks to the work @vicente-romero-oracle did) removed > that. I wanted to maintain that cleanup. To do that, now Attr passes the > expected kind to some methods in `Resolve`. This kind is then used to tell > whether we're inside an assignment or not. > > This strategy doesn't require looking inside `env.tree`, so it naturally > side-steps issues such as https://bugs.openjdk.org/browse/JDK-8385431. > > I've also simplified the test harness to remove `super` from some of the > tests (this harness is used to check the warning code path). It is now a > simple annotation processor, so it can focus on just rewriting the tree and > nothing else. > > --------- > - [x] I confirm that I make this contri... src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1458: > 1456: env.dup(tree, > env.info.dup(env.info.scope.dupUnshared(fakeOwner))); > 1457: > 1458: if ((tree.flags & STATIC) != 0) localEnv.info.staticLevel++; I believe this change was unrelated, so I've reverted it src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 5586: > 5584: } > 5585: } > 5586: } else { I believe this change was unrelated, so I've reverted it src/jdk.compiler/share/classes/com/sun/tools/javac/comp/AttrContext.java line 158: > 156: info.visitingServiceImplementation = > visitingServiceImplementation; > 157: info.allowProtectedAccess = allowProtectedAccess; > 158: info.instanceInitializerBlock = instanceInitializerBlock; I believe this change was unrelated, so I've reverted it src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java line 145: > 143: (preview.isEnabled() || > !preview.isPreview(Feature.PATTERN_SWITCH)); > 144: typePairToName = TypePairs.initialize(syms); > 145: this.allowValueClasses = preview.isEnabled() && > Feature.VALUE_CLASSES.allowedInSource(source); I've simplified all these checks as this is the more idiomatic style I think src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java line 427: > 425: } > 426: > 427: switch ((short)(sym.flags() & AccessFlags)) { I believe this change was unrelated, so I've reverted it test/micro/org/openjdk/bench/jdk/classfile/Transforms.java line 137: > 135: ? options > 136: : Stream.concat(Stream.of(options), > Stream.of(ClassFile.ConstantPoolSharingOption.NEW_POOL)).toArray(ClassFile.Option[]::new)); > 137: ClassFile tempcc = cc; This emitted a warning now, so I had to update it (to keep the build/test from failing) ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/2481#discussion_r3318388710 PR Review Comment: https://git.openjdk.org/valhalla/pull/2481#discussion_r3318386364 PR Review Comment: https://git.openjdk.org/valhalla/pull/2481#discussion_r3318382538 PR Review Comment: https://git.openjdk.org/valhalla/pull/2481#discussion_r3318377022 PR Review Comment: https://git.openjdk.org/valhalla/pull/2481#discussion_r3318380803 PR Review Comment: https://git.openjdk.org/valhalla/pull/2481#discussion_r3318395152
