On Thu, 28 May 2026 15:03:43 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. >> ... > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix issues with warning harness > Omit preview warnings if early logic is running in warning mode This definitely is more maintainable than that visitor in the long run. Peeked through and saw no obvious problem, but I am no professional. ------------- Marked as reviewed by liach (Committer). PR Review: https://git.openjdk.org/valhalla/pull/2481#pullrequestreview-4382309767
