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
8301580: Error recovery does not clear returnResult #12361
Conversation
👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into |
Webrevs
|
@@ -5220,9 +5220,14 @@ public void visitAnnotatedType(JCAnnotatedType tree) { | |||
public void visitErroneous(JCErroneous tree) { | |||
if (tree.errs != null) { | |||
Env<AttrContext> errEnv = env.dup(env.tree); |
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.
question: shouldn't we do: env.dup(node, env.info.dup());
instead, that way both env's won't share the info?
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 was a little bit on the fence on this, as some code, like Attr.attribClass
stores and re-sets the result, while other dupes the info. Let's dupe the info then. Thanks!
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 see, I didn't realize of the other pattern, not duping the info, we should probably standardize what we do at some point
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
@lahodaj This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 107 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit 522fa13.
Your commit was automatically rebased without conflicts. |
Considering code like:
void m
is wrapped in an erroneous tree (which is good), and if/whenAttr
is processing it invisitErroneous
, it will create a newEnv
to attribute the erroneous part.But,
Attr
will set areturnResult
into theEnv
's info - and that info is shared with the outterEnv
, so there will be areturnResult
set aftervisitErroneous
.{ return ; }
is interpreted as an initializer, and there should be an error for thereturn
there, but this error is missing, due to the setreturnResult
. This will then fail later inFlow
with an exception:The proposal is to clear the
returnResult
from theEnv
's info.This exception may affect JShell.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12361/head:pull/12361
$ git checkout pull/12361
Update a local copy of the PR:
$ git checkout pull/12361
$ git pull https://git.openjdk.org/jdk pull/12361/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 12361
View PR using the GUI difftool:
$ git pr show -t 12361
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12361.diff