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

List of patterns in case statements #1742

Closed
wants to merge 20 commits into from

Conversation

datho7561
Copy link
Contributor

@datho7561 datho7561 commented Dec 13, 2023

  • Parse multiple Patterns in a case statement
  • AST & generator changes for list of patterns
  • Fix error reporting for a pattern variable + multiple patterns

Closes #1643

What it does

Parse case statements with multiple patterns, as per JEP 443.

How to test

Confirm the following code snippet compiles and executes as expected:

Object o = "Hello, World";
switch (o) {
case String _, List _ when 1 == 1: System.out.println("it's working"); break;
default: System.out.println("it's not working");
}

Confirm that errors are placed on a and b in the following snippet:

Object o = "Hello, World";
switch (o) {
case String a, List b when 1 == 1: System.out.println("it's working"); break;
default: System.out.println("it's not working");
}

Author checklist

@datho7561 datho7561 marked this pull request as draft December 13, 2023 19:04
@datho7561
Copy link
Contributor Author

datho7561 commented Dec 13, 2023

Seems like when clauses and empty record patterns (i.e. MyRecord()) are broken and causing test failures

update: Ah I see, in d4e6217 Srikanth added one of the fixes that I included in this PR, so now it pops off more expressions than are on the stack.

update: I have addressed this, it should be working now

@datho7561
Copy link
Contributor Author

datho7561 commented Jan 9, 2024

UPDATE: this test failure is related to an actual bug, I'll look into it

Do we care about the test failure? I'm not sure what it's trying to test. The following code doesn't compile:

class SwitchCompletion {
	public static void main(String... args) {
		int i =  0;
		switch (i) {
			case fred().xyzabc: System.out.println();
		}
	}
	MyObj fred() {
		return new MyObj(0);
	}
}

record MyObj(int myProp) {
	public static final int xyzabc = 0;
}

This is because you are not allowed to invoke fred() in the case statement.

and the test case seems to be about converting case fred().xyz to case fred().xyzabc

@datho7561 datho7561 force-pushed the pattern-lists-in-switch branch 3 times, most recently from 02a0ce7 to bf4ca9b Compare January 18, 2024 13:32
@datho7561 datho7561 marked this pull request as ready for review January 18, 2024 13:37
@datho7561
Copy link
Contributor Author

datho7561 commented Jan 18, 2024

@srikanth-sankaran @jarthana This PR is ready for review. The tests are passing.

I still have to add code generation, but in order to test code generation, I will need #1517, since you need to use unnamed variables in order to use lists of Patterns in case statements.

@srikanth-sankaran
Copy link
Contributor

I'll take a look in a couple of days. Thanks

@srikanth-sankaran
Copy link
Contributor

@srikanth-sankaran @jarthana This PR is ready for review. The tests are passing.

I still have to add code generation, but in order to test code generation, I will need #1517, since you need to use unnamed variables in order to use lists of Patterns in case statements.

In that case, let us focus and prioritize #1517 please. That PR needs to be rebased and updated and then I will help close it.

@srikanth-sankaran
Copy link
Contributor

@datho7561 - I have made us joint owners of this. I would like to include this as part of the comprehensive cleaup to pattern code generation rather than rush through this.

@jukzi
Copy link
Contributor

jukzi commented Feb 14, 2024

that "4 new warnings" seem unrelated and should go away when you rebase on master
https://ci.eclipse.org/jdt/job/eclipse.jdt.core-Github/job/PR-1742/16/eclipse/new/
image

@datho7561
Copy link
Contributor Author

I've been working on rebasing this PR. It seems many things are working properly. However, I've hit a roadblock when it comes to fallthrough with cases with pattern. I've figured out some hacks to get this working:

public class Issue712_001 {
	public static void main(String[] args) {
		Object o = "Hello World";
		foo(o);
	}

	public static void foo(Object o) {
		switch (o) {
		case String s:
			System.out.println(s); // No break!
		case R():
			System.out.println("It's either an R or a string"); // Allowed
			break;
		default:
		}
	}
}

record R() {}
record S() {}

However, then I ran into a javac bug (I've hopefully submitted this properly upstream). I have no idea what's supposed to happen in this case:

public class fallthroughPattern {

	public static void main(String... args) {
		Object o = "";
		switch (o) {
		case String s:
			System.out.println("this is the first outcome");
		case R(String _):
			System.out.println("this is the second outcome");
		case R(Integer _):
			System.out.println("this is the third outcome");
		default:
			System.out.println("this is the fourth outcome");
		}

	}

	private static final record R<T>(T a) {}

}

@srikanth-sankaran
Copy link
Contributor

Thanks David. Note that Jay reported three problems of which fixes for only two are pushed to that PR. I am working on the other one which very likely is involved in the case you cite.

I expect to push a fix before end of business Wednesday.

Do you have a link for the javac defect you reported ??

@srikanth-sankaran
Copy link
Contributor

Please pick up the latest from the cumulative pattern fixes PR. I fixed a few problems today. May be it will be of help in what issues you are facing

@datho7561
Copy link
Contributor Author

I am working on the other one which very likely is involved in the case you cite.

Thank you!

Do you have a link for the javac defect you reported ??

I tried opening the bug right after I reported it, but I couldn't find it. I'll try again

I fixed a few problems today. May be it will be of help in what issues you are facing

I'll rebase against your PR today. I might as well push the somewhat working rebase as well.

- Multiple patterns are reported as an error unless all variables are
  unnamed
- GuardedPattern now has a list of Patterns as a child,
  deprecate the methods that assume it has one
- Modified tests to adapt to the parser not handling multiple guards in
  one case statement any more
  (this was never allowed, but now the error has changed)
- Modify tests that allow variables after record patterns
  (this feature was removed before record patterns moved out of preview)
- AST & generator changes

Signed-off-by: David Thompson <davthomp@redhat.com>
@datho7561
Copy link
Contributor Author

I still wasn't able to find the bug I submitted to javac regarding switch statements with patterns with fallthrough. Maybe I'll try submitting again?

I think that I got all the tests working after rebasing on your latest change, just running locally to ensure nothing bad happened.

@srikanth-sankaran
Copy link
Contributor

I still wasn't able to find the bug I submitted to javac regarding switch statements with patterns with fallthrough. Maybe I'll try submitting again?

Could you describe here what the defect is ? Sometimes there is a latency as I recall.

I think that I got all the tests working after rebasing on your latest change, just running locally to ensure nothing bad happened.

Excellent. I'll look into absorbing it into the cumulative fixes branch this week. Any case I would like to wrap up this work by end of Feb.

@srikanth-sankaran
Copy link
Contributor

Not being an expert in git/github, I wonder how I would see just the diffs wrt to https://github.com/srikanth-sankaran/eclipse.jdt.core/tree/patterns-accumulated-fixes branch head.

Let me know if you think this is ready for me to start with the review and absorbing the changes

@srikanth-sankaran
Copy link
Contributor

I have picked up the changes from here and pushed them to #2011 - action can continue there. I am reviewing the changes and will push additional commits for any issues I find during review/testing. I may ask for help with some tasks - any additional commits can be against the srikanth-sankaran:patterns-accumulated-fixes branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JEP 443: lists of patterns as a case label
3 participants