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

[Patterns][Java22] Abstraction gap in modelling multi case patterns #2069

Closed
srikanth-sankaran opened this issue Feb 26, 2024 · 4 comments · Fixed by #2011
Closed

[Patterns][Java22] Abstraction gap in modelling multi case patterns #2069

srikanth-sankaran opened this issue Feb 26, 2024 · 4 comments · Fixed by #2011
Assignees

Comments

@srikanth-sankaran
Copy link
Contributor

srikanth-sankaran commented Feb 26, 2024

As of commit 684524f made as part of PR #2011 we have a working compiler implementation for multiple patterns in a case statement.

However as @datho7561 points out in review feedback here #2011 (review) and acknowledged here #2011 (comment) AST clients will need a better abstraction.

The proposal is to come with a new class EitherOrMultiPattern which can stand on its own when there is no guard in the source or be the primaryPattern of a GuardedPattern otherwise,

On the DOM side, we will need the dopplegangers of course,

@datho7561
Copy link
Contributor

The main rationale for connecting the guard to the CaseStatement instead of some new construct would be to match how it's specified in the spec (see 14.11.1 Switch Blocks in https://docs.oracle.com/javase/specs/jls/se21/preview/specs/unnamed-jls.html, but note that they use the name "SwitchLabel" instead of CaseStatement)

@srikanth-sankaran
Copy link
Contributor Author

srikanth-sankaran commented Feb 26, 2024

The main rationale for connecting the guard to the CaseStatement instead of some new construct would be to match how it's specified in the spec (see 14.11.1 Switch Blocks in https://docs.oracle.com/javase/specs/jls/se21/preview/specs/unnamed-jls.html, but note that they use the name "SwitchLabel" instead of CaseStatement)

I can see the guard being attached to the CaseStatement as a valid alternate model. BUT I don't see the multiple-patterns vs restricted-to-single-pattern paradigm necessarily in and of itself making a case for the guard being moved to the CaseStatement.

Academic point: I was wondering why we need an abstraction for GuardedPattern in the first place - why couldn't it have been a behavior directly added to existing Pattern hierarchy rather than invent a new class ? I guess there are no clearcut preferences and one is as valid as another.

@srikanth-sankaran
Copy link
Contributor Author

The main rationale for connecting the guard to the CaseStatement instead of some new construct would be to match how it's specified in the spec (see 14.11.1 Switch Blocks in https://docs.oracle.com/javase/specs/jls/se21/preview/specs/unnamed-jls.html, but note that they use the name "SwitchLabel" instead of CaseStatement)

I can see the guard being attached to the CaseStatement as a valid alternate model. BUT I don't see the multiple-patterns vs restricted-to-single-pattern paradigm necessarily in and of itself making a case for the guard being moved to the CaseStatement.

Academic point: I was wondering why we need an abstraction for GuardedPattern in the first place - why couldn't it have been a behavior directly added to existing Pattern hierarchy rather than invent a new class ? I guess there are no clearcut preferences and one is as valid as another.

That guards enter the picture only with patterns (i.e. you can't say when predicate in a non pattern case) argue for guards being saddled with patterns. But that guards are not used with patterns outside of case (i.e. you can't say if (x instanceof Type when predicate)) argue for guards being lumped up with case statement. In the end, the "right" thing is to do what is harmonious with present/other aspects - that we already have a GuardedPattern abstraction seals the case (double pun intended) for the opted design choice of a new abstraction EitherOrMultiPattern which stands on its own in the absence of guards and as the primaryPattern of a GuardedPattern otherwise.

BTW, the original GuardedPattern has an issue - it is not ideal that the patterns being guarded don't know they are being guarded. This is corrected via org.eclipse.jdt.internal.compiler.ast.Pattern.isEffectivelyUnguarded()

@srikanth-sankaran
Copy link
Contributor Author

Re-reading 14.11.1 Switch Blocks, it is clear that JLS intends guards to be attached to switch rules and not patterns. So you are right there @datho7561 - but for reasons discussed above, I will leave status quo undisturbed. It doesn't make a material difference as far as I can see.

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 a pull request may close this issue.

2 participants