-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Conversation
👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into |
/solves 8322882 |
❗ This change is not yet ready to be integrated. |
@mcimadamore |
@mcimadamore The following label will be automatically applied to this pull request:
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. |
* 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 { |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
Webrevs
|
/csr |
As the changes here change the space of compilable programs, it would be better to document these changes in a CSR. |
@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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this 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
@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! |
@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 |
@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? |
/open |
@mcimadamore This pull request is now open |
4e7643c
to
65c809a
Compare
@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. |
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 For that same reason, when an enclosing instance is missing, we should just try to see if For local classes, the considerations in this PR still apply. |
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
Outdated
Show resolved
Hide resolved
new Local(); // can't get there from here | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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
I'm closing this pull request. I will be opening a new one shortly, with same code, but new JBS issue: JDK-8338288 |
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, viasuper
. These checks currently reside inResolve::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()
whereM
is a member inner class, we need to infer an expression forM
‘s enclosing instance, given none is provided. This inference problem is also present when checking asuper(...)
constructor call: if the superclass is a member inner classM
, then validating the constructor call implies inferring a suitable enclosing instance forM
, as if we were checking newM()
.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 inC
‘s early construction context) and;C
is a subclass ofM
‘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
Issues
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