Skip to content
This repository has been archived by the owner. It is now read-only.

8269146: Missing unreported constraints on pattern and other case label combination #194

Closed
wants to merge 9 commits into from

Conversation

@lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Jul 1, 2021

There is a number of constraints on the case label forms - patterns cannot be combined with patterns, etc. Currently, the checks are implemented by checking if there are clashing bindings. But this misses some of the cases which should lead to errors. This patch proposes to implement explicit checks on the structure of the case labels in Attr.handleSwitch, rather than relying on clashing bindings.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issues

  • JDK-8269146: Missing unreported constraints on pattern and other case label combination
  • JDK-8269301: Switch statement with a pattern, constant and default label elements crash javac

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17 pull/194/head:pull/194
$ git checkout pull/194

Update a local copy of the PR:
$ git checkout pull/194
$ git pull https://git.openjdk.java.net/jdk17 pull/194/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 194

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk17/pull/194.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 1, 2021

👋 Welcome back jlahoda! 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.

Loading

@openjdk openjdk bot added the rfr label Jul 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 1, 2021

@lahodaj 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.

Loading

@openjdk openjdk bot added the compiler label Jul 1, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 1, 2021

Webrevs

Loading

@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Jul 2, 2021

/issue JDK-8269301

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jul 2, 2021

@lahodaj
Adding additional issue to issue list: 8269301: Switch statement with a pattern, constant and default label elements crash javac.

Loading

Copy link
Contributor

@mcimadamore mcimadamore left a comment

I wonder if all the wasDefault etc tests, to check compatibility between different kind of labels couldn't be moved when at parse time. The code in attr is already quite convoluted, and it seems to be doing many things at once - if there are checks which depend on flow analysis, of course these should remain in Attr - but there seems to be a lot of structural checks which do not really depend on attribution which could be caught a lot earlier, and probably giving better error messages too.

Loading

@@ -1681,10 +1681,18 @@ private void handleSwitch(JCTree switchTree,
boolean hasDefault = false; // Is there a default label?
boolean hasTotalPattern = false; // Is there a total pattern?
boolean hasNullPattern = false; // Is there a null pattern?
boolean wasConstant = false; // Seen a constant in the same case label
Copy link
Contributor

@mcimadamore mcimadamore Jul 6, 2021

Choose a reason for hiding this comment

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

I'd suggest to move the wasXYZ variables inside the case label loop - since they are not meant to be variables that are global to the entire switch construct?

Loading

Copy link
Contributor Author

@lahodaj lahodaj Jul 6, 2021

Choose a reason for hiding this comment

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

I agree it would be much better to have the flags closer, but the flags sadly generally apply across case clauses. The conditions don't allow case <type pattern>, default: as well as case <type pattern>: /*no statements*/ case default:, because both are SwitchLabels. So we need to keep information across case clauses if the case has an empty list of statements, and in some cases even if the list of statements is not empty, but there is a fall-through to the following case (which is to prevent cases like case null: System.err.println(); /*no break*/ case Integer i:).

Loading

Copy link
Contributor

@mcimadamore mcimadamore Jul 6, 2021

Choose a reason for hiding this comment

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

OK, thanks for the clarification, I think I now get what the code is trying to do and the fact that multiple cases (even if separated by commas) are really dealt with separately by the logic, so you need to keep these flags in scope for longer.

I still think that some of these checks could be perhaps moved closer to parser - and then in Attr we could check stuff that depends on types (e.g. dominance), or stuff that depends on control flow (e.g. cannot fall through to a pattern).

I understand that the code was like this (e.g. duplicate labels, or duplicate defaults has always been caught in Attr) - but as the number of checks grows larger, it feels like keep adding checks in Attr doesn't scale very well.

Loading

@@ -1806,11 +1825,17 @@ private void handleSwitch(JCTree switchTree,
addVars(c.stats, switchEnv.info.scope);

boolean completesNormally = c.caseKind == CaseTree.CaseKind.STATEMENT ? flow.aliveAfter(caseEnv, c, make) : false;

if (c.stats.nonEmpty()) {
Copy link
Contributor

@mcimadamore mcimadamore Jul 6, 2021

Choose a reason for hiding this comment

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

This initialization should probably go at the beginning of the for loop block?

Loading

if (prevCompletedNormally) {
boolean isTypePattern = pat.hasTag(BINDINGPATTERN);
if (wasPattern || wasConstant || wasDefault ||
(wasNullPattern && (!isTypePattern || wasNonEmptyFallThrough))) {
log.error(pat.pos(), Errors.FlowsThroughToPattern);
Copy link
Contributor

@mcimadamore mcimadamore Jul 6, 2021

Choose a reason for hiding this comment

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

Is this a good error message for the condition? E.g. it seems like here we're mostly complaining about some illegal mix of constant, default and pattern labels - which seems more something to do with well-formedness than with fallthrough?

Loading

Copy link
Contributor

@mcimadamore mcimadamore left a comment

Approving for now, perhaps with a view to investigate ways to split the checks at a later point.

Loading

@@ -1681,10 +1681,18 @@ private void handleSwitch(JCTree switchTree,
boolean hasDefault = false; // Is there a default label?
boolean hasTotalPattern = false; // Is there a total pattern?
boolean hasNullPattern = false; // Is there a null pattern?
boolean wasConstant = false; // Seen a constant in the same case label
Copy link
Contributor

@mcimadamore mcimadamore Jul 6, 2021

Choose a reason for hiding this comment

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

OK, thanks for the clarification, I think I now get what the code is trying to do and the fact that multiple cases (even if separated by commas) are really dealt with separately by the logic, so you need to keep these flags in scope for longer.

I still think that some of these checks could be perhaps moved closer to parser - and then in Attr we could check stuff that depends on types (e.g. dominance), or stuff that depends on control flow (e.g. cannot fall through to a pattern).

I understand that the code was like this (e.g. duplicate labels, or duplicate defaults has always been caught in Attr) - but as the number of checks grows larger, it feels like keep adding checks in Attr doesn't scale very well.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jul 6, 2021

@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:

8269146: Missing unreported constraints on pattern and other case label combination
8269301: Switch statement with a pattern, constant and default label elements crash javac

Reviewed-by: mcimadamore

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 1 new commit pushed to the master branch:

  • 9e75f92: 8269738: AssertionError when combining pattern matching and function closure

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Jul 6, 2021
@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Jul 8, 2021

I've updated the patch so that the new (/structural) tests are moved from Attr to Check. To me, looks better. What do you think?
Thanks!

Loading

Copy link
Contributor

@mcimadamore mcimadamore left a comment

Looks good - I agree the code looks simpler, and there's a bit more separation of concerns

Loading

@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Jul 9, 2021

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jul 9, 2021

Going to push as commit 885f7b1.
Since your change was applied there have been 6 commits pushed to the master branch:

  • 62ff55d: 8269952: compiler/vectorapi/VectorCastShape*Test.java tests failed on avx2 machines
  • 46c610c: 8269840: Update Platform.isDefaultCDSArchiveSupported() to return true for aarch64 platforms
  • 6401633: 8269722: NPE in HtmlDocletWriter
  • 9acb2a6: 8270109: ProblemList 4 SA tests on macOS-aarch64
  • f46a917: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF
  • 9e75f92: 8269738: AssertionError when combining pattern matching and function closure

Your commit was automatically rebased without conflicts.

Loading

@openjdk openjdk bot closed this Jul 9, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 9, 2021

@lahodaj Pushed as commit 885f7b1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
2 participants