Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8334248: Invalid error for early construction local class constructor method reference #19904

Closed
wants to merge 7 commits into from

Conversation

mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Jun 26, 2024

This PR fixes some of the checks that are applied when a new inner class (whether member, local or anonymous) is constructed, either via new, or, indirectly, via super. These checks currently reside in Resolve::resolveImplicitThis, but this routine fails to take into account the differences between the various cases (e.g. the checks for member inner classes are different than those for local/anon classes), and this results in spurious compilation errors. Below is an attempt to describe what should happen, in the various cases.

Member inner classes

Whenever we see new M() where M is a member inner class, we need to infer an expression for M‘s enclosing instance, given none is provided. This inference problem is also present when checking a super(...) constructor call: if the superclass is a member inner class M, then validating the constructor call implies inferring a suitable enclosing instance for M, as if we were checking new M().

This inference process should look at all the enclosing instances available to us, and pick the innermost enclosing instance of type C such that:

  • C.this is accessible (e.g. not in C‘s early construction context) and;
  • C is a subclass of M‘s enclosing class.

The crucial observation here is that there can be multiple valid enclosing instances, and the innermost one might not be available due to early construction context, so we need to be able to skip that, and jump to the next. See the test EarlyIndirectOuterCapture, which fails to compile today, but is accepted with the fixes in this PR.

This check is defined in Reslve::findSelfContaining.

Local and anonymous inner classes

When creating local and anonymous inner classes, we should not check for the availability of an enclosing instance, as JLS 15.9.2 is silent about this. What matters, for local and anon classes, is that the class creation expression occurs in a context where we can access local variables defined in the method in which the local class is defined. This means that code like the one in the LocalFreeVarStaticInstantiate test is now correctly rejected.

This check is defined in Reslve::findLocalClassOwner.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Change requires CSR request JDK-8341469 to be approved
  • Commit message must refer to an issue

Issues

  • JDK-8334248: Invalid error for early construction local class constructor method reference (Bug - P4)
  • JDK-8322882: Null pointer error when compiling Static initializer in a local class (Bug - P4) ⚠️ Issue is not open.
  • JDK-8341469: Invalid error for early construction local class constructor method reference (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19904/head:pull/19904
$ git checkout pull/19904

Update a local copy of the PR:
$ git checkout pull/19904
$ git pull https://git.openjdk.org/jdk.git pull/19904/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19904

View PR using the GUI difftool:
$ git pr show -t 19904

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19904.diff

Webrev

Link to Webrev Comment

Sorry, something went wrong.

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 26, 2024

👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@mcimadamore
Copy link
Contributor Author

/solves 8322882

@openjdk
Copy link

openjdk bot commented Jun 26, 2024

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk
Copy link

openjdk bot commented Jun 26, 2024

@mcimadamore
Adding additional issue to solves list: 8322882: Null pointer error when compiling Static initializer in a local class.

@openjdk
Copy link

openjdk bot commented Jun 26, 2024

@mcimadamore The following label will be automatically applied to this pull request:

  • compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the compiler compiler-dev@openjdk.org label Jun 26, 2024
* BadConstructorReferenceError error class indicating that a constructor reference symbol has been found,
* but pointing to a class for which an enclosing instance is not available.
*/
class BadConstructorReferenceError extends InvalidSymbolError {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was no longer needed. We had a duplication between a check issued when resolving method references (see ConstructorReferenceLookupHelper) and a post-resolution check in Attr::visitReference. I think keeping the latter makes much more sense, as that brings more consistency between the code paths for instance creation expression and constructor reference expression.

@@ -3065,6 +3064,23 @@ public void report(DiagnosticPosition _unused, JCDiagnostic details) {
};
}

void checkNewInnerClass(DiagnosticPosition pos, Env<AttrContext> env, Type type) {
boolean isLocal = type.tsym.owner.kind == MTH;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit subtle - we need to run the new checks for all inner classes. E.g. if a local class C has no enclosing instance (because is declared in a static context), we might still need to check that new C() is valid, because C might capture variables from the enclosing (static) method.

@mcimadamore mcimadamore marked this pull request as ready for review June 26, 2024 11:39
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 26, 2024
@mlbridge
Copy link

mlbridge bot commented Jun 26, 2024

Webrevs

@mcimadamore
Copy link
Contributor Author

/csr

@mcimadamore
Copy link
Contributor Author

As the changes here change the space of compilable programs, it would be better to document these changes in a CSR.

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Jun 26, 2024
@openjdk
Copy link

openjdk bot commented Jun 26, 2024

@mcimadamore has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@mcimadamore please create a CSR request, with the correct fix version, for at least one of the issues associated with this pull request. This pull request cannot be integrated until all the CSR request are approved.

Assert.check(name == names._this);
Env<AttrContext> env1 = env;
boolean staticOnly = false;
Symbol bestSoFar = varNotFound;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it seems like this variable either holds this value, varNotFound, or an error but never the symbol we are looking for, so probably the name should reflect this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've renamed this to firstError

rs.findLocalClassOwner(env, type.tsym) :
rs.findSelfContaining(pos, env, type.getEnclosingType().tsym, names._this);
if (res.exists()) {
rs.accessBase(res, pos, env.enclClass.sym.type, names._this, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the call to accesBase should be part of Resolve::findLocalClassOwner and Resolve::findSelfContaining? in other case clients needs to know a bit more about Resolve's internals. Although there are other places in Attr where we invoke Resolve:accessBase, so probably in the fence with this one

Symbol findSelfContaining(DiagnosticPosition pos,
Env<AttrContext> env,
TypeSymbol c,
Name name) {
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't the name always be this, so I guess we can remove the parameter?

Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, some minor comments

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 26, 2024

@mcimadamore This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 23, 2024

@mcimadamore This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Aug 23, 2024
@archiecobbs
Copy link
Contributor

@mcimadamore, this PR is still needed (i.e., the compiler is not following the current spec), right? Would it help for me to take a stab at drafting the CSR for you?

@mcimadamore
Copy link
Contributor Author

/open

@openjdk openjdk bot reopened this Sep 24, 2024
@openjdk
Copy link

openjdk bot commented Sep 24, 2024

@mcimadamore This pull request is now open

@openjdk
Copy link

openjdk bot commented Sep 25, 2024

@mcimadamore Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@mcimadamore
Copy link
Contributor Author

I've updated the PR. After an offline discussion, we concluded that the behavior introduced by this PR is overly powerful. That is, the ability, when looking for a suitable C.this, to skip over enclosing classes that are in early-construction context, seems too subtle. In other cases e.g. when we resolve a method name, we pick the first enclosing class that has a method with that name, regardless of whether the signature of that method actually matches or not.

For that same reason, when an enclosing instance is missing, we should just try to see if C.this is valid. If not, instead of finding some other X.Y.this which might be suitable, it is better to just report an error. The user can manually disambiguate, if required.

For local classes, the considerations in this PR still apply.

new Local(); // can't get there from here
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the test case should be enhanced with something like:

    Runnable r = () -> {
        Object there = "";
        class Local {
            {
                there.hashCode();
            }
            static {
                new Local();    // can't get there from here
            }
        }
    };

It works currently, as lambdas get a fake method owner if needed, so it would be just to make sure it remains working even if the handling of the lambdas changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've beefed up the test. While doing so I realized that the generated diagnostic was suboptimal, so I've fixed that too.

@@ -50,6 +50,8 @@
import com.sun.tools.javac.comp.Check.CheckContext;
import com.sun.tools.javac.comp.DeferredAttr.AttrMode;
import com.sun.tools.javac.comp.MatchBindingsComputer.MatchBindings;
import com.sun.tools.javac.comp.Resolve.InvalidSymbolError;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: these imports appear to be unused.

Symbol findSelfContaining(DiagnosticPosition pos,
Env<AttrContext> env,
TypeSymbol c,
Name name,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is probably a matter of style, but for me, it would be easier if name would be dropped, and names._this would be used in findFirst. It took me a while before I realized name can only be this, so there cannot be more than one, so findFirst is "find it or return null", not "if there are multiple, return one of them".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - this was mostly a leftover from the previous code

@mcimadamore mcimadamore closed this Oct 8, 2024
@mcimadamore
Copy link
Contributor Author

I'm closing this pull request. I will be opening a new one shortly, with same code, but new JBS issue: JDK-8338288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler compiler-dev@openjdk.org csr Pull request needs approved CSR before integration rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

None yet

4 participants