[Switch][Record Patterns] Unchecked conversion error missing from ECJ #3069

stephan-herrmann · 2024-10-10T13:31:43Z

try to balance responsibility about type checking between
- Pattern.isApplicable() - don't yet report in nested position
- RecordPattern.resolveType() - failed to check isApplicable()
- InstanceOfExpression.resolveType() - avoid pattern-agnostic checks
added the 1st(!) test for inapplicable record pattern in switch

stephan-herrmann · 2024-10-10T13:35:29Z

@srikanth-sankaran please have a look at the different parties involved in compatibility checking for (record) patterns. Quite likely the code can be simplified by being more explicit about which part is responsible for which aspect of checking.

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/Pattern.java

srikanth-sankaran · 2024-10-10T14:24:35Z

Thanks for the quick follow up Stephan. I'll review this Monday first thing - tomorrow is a holiday in India. Not just a regular holiday but one where you are not supposed to use any tools of your trade! 😊

(https://en.m.wikipedia.org/wiki/Ayudha_Puja)

And after I wrap up #2720 early next week I’ll join you in the grammar work in full earnest.

srikanth-sankaran

I am happy to look over once the current set of feedback is addressed.

Still feel it is a bit of a smell that we have in CaseStatement this:

if (expressionType != TypeBinding.NULL && !(e instanceof RecordPattern)) {

There is something in the flow that could be adjusted so we don't have to have this check for type test patterns alone - what obviates it for record patterns, why doesn't the same thing obviate it for non-record patterns too ? Don't know yet

srikanth-sankaran · 2024-10-14T05:48:20Z

....jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/InstanceOfExpression.java

@@ -277,6 +277,9 @@ public TypeBinding resolveType(BlockScope scope) {
 	if (expressionType == null || checkedType == null)
 		return null;

+	if (this.pattern instanceof RecordPattern) // subsequent tests are not relevant for RecordPatterns
+		return this.resolvedType = TypeBinding.BOOLEAN;
+


Looks ok. However the short circuited code has a tendency to mischaracterize a type incompatibility as an unsafe cast. I have raised a separate ticket for that. See #3074

reworked in df50953 - do you like it better now?

srikanth-sankaran · 2024-10-14T06:04:44Z

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/Pattern.java

+					return false;
+				}
+				applicable = false;
+			}


The way this is coded it appears there could be a scenario where the cast is illegal but also unsafe. I doubt that to be valid ever.

If I change applicable = false; to be return applicable = false; I don't see any test failing.

If this is a valid observation, it could allow for some simplification of the code by we could get rid of the new variable

reworked in df50953 - do you like it better now?

...ore.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/RecordPatternTest.java

stephan-herrmann · 2024-10-15T18:27:07Z

@srikanth-sankaran after my first commit focused on (local) correctness, in df50953 I made an attempt to define a basic rule for assigning responsibilities:

Pattern.isApplicable() is now responsible for most type checking of patterns
"clients" InstanceOfExpression and CaseStatement are now responsible for calling isApplicable() and integrating this call with there overall task of semantic analysis. Similarly for RecordPattern regarding nested patterns.

I could imagine that this can be taken even one step further. There are still differences between InstanceOfExpression and CaseStatement for which I don't readily see the reason, e.g., regarding integration with isReifiable() and handling of primitives.

I also slightly unified problem reporting: let the problem reporter select the problemID based on the error location rather than defining separate reporting methods. As one effect this allowed me to remove a reporting method, which was a bit sloppy with short vs long readable names.

I'm not yet 100% convinced of the idea to share IncompatibleTypesInEqualityOperator and IncompatibleTypesInConditionalOperator. Trying to clean up this area (overlap with checking for ConditionalExpression) also led to filing #3089

Let me know if you like better now, or if you see potential for further clarification.

srikanth-sankaran · 2024-10-17T10:04:00Z

Yes, this is on my radar. I will review this before tomorrow. Thanks for your patience.

srikanth-sankaran

Looks good - except for the minor points called out.

...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java

...ore.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/SwitchPatternTest.java

...s.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/PrimitiveInPatternsTestSH.java

stephan-herrmann · 2024-10-22T21:15:53Z

@srikanth-sankaran the failure in SwitchPatternTest22.testIssue3109 reminds me of our discussion around #3024:

			import java.util.function.Function;
			import java.util.function.Predicate;

			public sealed interface Maybe<T>  {
...
			  default Maybe<T> filter(Predicate<T> predicate){
			    return switch (this) {
			      case Some(T value) when predicate.test(value) -> this;
			      case Some(_), None<T> _ -> empty();
			    };
			  }
			  record Some<T>(T value) implements Maybe<T> {}
			  record None<T>() implements Maybe<T>{}
			}

With the patch here we report

 case Some(T value) when predicate.test(value) -> this;
           ^^^^^^^
Type Object cannot be safely cast to T

But from this : MayBe<T> we could "deduce" that a matching Some would automatically be a Some<T>.

Do your think you implementation from #3024 could be extracted / generalized so it can be used for isApplicable() to "improve" the provided expressionType? Currently we get Object to be matched against T but from the switchExpressionType MayBe<T> I would hope we would come up with a better type??

srikanth-sankaran · 2024-10-23T00:15:10Z

But from this : MayBe<T> we could "deduce" that a matching Some would automatically be a Some<T>.

Do your think you implementation from #3024 could be extracted / generalized so it can be used for isApplicable() to "improve" the provided expressionType? Currently we get Object to be matched against T but from the switchExpressionType MayBe<T> I would hope we would come up with a better type??

Quite possibly. I am planning to work on #3038 which should solve #3121 later this week - I will have the concern from here in the reckoning too.

(In my mind an ideal fix for the present problem would get rid of the instanceof check if (expressionType != TypeBinding.NULL && !(e instanceof RecordPattern)) { from org.eclipse.jdt.internal.compiler.ast.CaseStatement.resolveCasePattern and that may become possible with the approach you hint at)

srikanth-sankaran · 2024-10-23T08:11:15Z

I'll try and propose a patch for #3038 that will also address #3121 and the present ticket #3069

Bonus points if I manage to get #3119 and #3074 fixed in the process.

I’ll be in touch as I need consulting help.

stephan-herrmann · 2024-10-23T20:33:02Z

(In my mind an ideal fix for the present problem would get rid of the instanceof check if (expressionType != TypeBinding.NULL && !(e instanceof RecordPattern)) { from org.eclipse.jdt.internal.compiler.ast.CaseStatement.resolveCasePattern and that may become possible with the approach you hint at)

Sorry, I simply forgot to remove that part, done in 7d2f381 :)

stephan-herrmann · 2024-10-23T20:38:12Z

I'll try and propose a patch for #3038 that will also address #3121 and the present ticket #3069

Actually I do prefer to let this PR resolve itself, rather than mixing it with unrelated issues. Well, there is an indirect connection: in e329739 I simply extracted part of your work on sealed types (#3024) and applied it for type checking of patterns, which indeed fixes the single regression from the previous version of this PR

If jenkins agrees then this PR should be ready for another round of review. Thanks.

stephan-herrmann · 2024-10-23T21:08:36Z

I'll try and propose a patch for #3038 that will also address #3121 and the present ticket #3069

Actually I do prefer to let this PR resolve itself, rather than mixing it with unrelated issues. Well, there is an indirect connection: in e329739 I simply extracted part of your work on sealed types (#3024) and applied it for type checking of patterns, which indeed fixes the single regression from the previous version of this PR

Actually the entire connection to sealed types (via #3024) was a red herring. The real problem was a missing Pattern#outerExpressionType. This is being fixed by letting GuardedPattern and EitherOrPattern propagate to the contained patterns (filling in an omission from #2827). Then with a valid outerExpressionType type inference kicks in 😄 and finds the more specific pattern type, TADA 🎉

In that light I guess I can proceed based on your previous approval, or do you want to do a final round of reviewing, @srikanth-sankaran ?

srikanth-sankaran · 2024-10-23T22:11:45Z

Jenkins is green and I'll look through the changes later today.

srikanth-sankaran · 2024-10-24T10:16:06Z

In that light I guess I can proceed based on your previous approval, or do you want to do a final round of reviewing, @srikanth-sankaran ?

The changes look good. Seeing that it also addresses the wrong diagnostic that is the subject matter of #3074, I have assigned that ticket to you and linked that bug to this PR.

Looks like the existing test org.eclipse.jdt.core.tests.compiler.regression.RecordPatternTest.test47() covers this problem - so if you prefer to release as is without adding a regression test for #3074 it is OK.

The comment // this one should report an unsafe cast error\n" in RecordPatternTest.java:1561 is bogus, but ...

Please proceed to integrate. Thanks.

+ try to balance responsibility about type checking between - Pattern.isApplicable() - don't yet report in nested position - RecordPattern.resolveType() - failed to check isApplicable() - InstanceOfExpression.resolveType() - avoid pattern-agnostic checks + added the 1st(!) test for inapplicable record pattern in switch

+ enclosing instanceof and case stmt are responsible for its invocation + make isApplicable() more capable to issue the desired problems + let problem reporter see the error location to select problemID + more renaming for clarity Test changes: + compatibility errors more consistently mark the entire instanceof expr + remove unwanted type qualifiers + don't expect some secondary errors

eclipse-jdt#3074

stephan-herrmann added this to the 4.34 M2 milestone Oct 10, 2024

stephan-herrmann requested a review from srikanth-sankaran October 10, 2024 13:31

stephan-herrmann commented Oct 10, 2024

View reviewed changes

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/Pattern.java Outdated Show resolved Hide resolved

srikanth-sankaran requested changes Oct 14, 2024

View reviewed changes

stephan-herrmann requested a review from srikanth-sankaran October 17, 2024 09:42

srikanth-sankaran approved these changes Oct 18, 2024

View reviewed changes

stephan-herrmann mentioned this pull request Oct 19, 2024

[Patterns] problematic re-use of ProblemIDs #3119

Open

stephan-herrmann force-pushed the issue3066 branch from f80e7ba to 7e73c2b Compare October 19, 2024 12:48

stephan-herrmann force-pushed the issue3066 branch from e329739 to c6e81db Compare October 23, 2024 21:06

srikanth-sankaran linked an issue Oct 24, 2024 that may be closed by this pull request

[Patterns] Inappropriate diagnostic with type test patterns #3074

Closed

stephan-herrmann mentioned this pull request Oct 24, 2024

Diamond shouldn't be allowed in pattern #3157

Open

stephan-herrmann added 7 commits October 24, 2024 18:24

fix "left" and "right" types in error reporting

a565312

remove imprecise comment, defer to issue eclipse-jdt#3119

2614da8

remove conditional checks that are now obsolete

fdf6348

fix regression be more complete propagation of outerExpressionType

db6582d

review comment: remove stale comment in test

16beb44

test case for

f45e77e

eclipse-jdt#3074

stephan-herrmann force-pushed the issue3066 branch from c6e81db to f45e77e Compare October 24, 2024 16:26

stephan-herrmann mentioned this pull request Oct 24, 2024

[Patterns] Inappropriate diagnostic with type test patterns #3074

Closed

stephan-herrmann merged commit 55f53bd into eclipse-jdt:master Oct 24, 2024
10 checks passed

stephan-herrmann deleted the issue3066 branch October 24, 2024 17:18

stephan-herrmann modified the milestones: 4.34 M2, 4.34 M3 Oct 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Switch][Record Patterns] Unchecked conversion error missing from ECJ #3069

[Switch][Record Patterns] Unchecked conversion error missing from ECJ #3069

stephan-herrmann commented Oct 10, 2024 •

edited by srikanth-sankaran

Loading

stephan-herrmann commented Oct 10, 2024

srikanth-sankaran commented Oct 10, 2024

srikanth-sankaran left a comment

srikanth-sankaran Oct 14, 2024

stephan-herrmann Oct 15, 2024

srikanth-sankaran Oct 14, 2024

stephan-herrmann Oct 15, 2024

stephan-herrmann commented Oct 15, 2024

srikanth-sankaran commented Oct 17, 2024

srikanth-sankaran left a comment

stephan-herrmann commented Oct 22, 2024

srikanth-sankaran commented Oct 23, 2024

srikanth-sankaran commented Oct 23, 2024 •

edited

Loading

stephan-herrmann commented Oct 23, 2024

stephan-herrmann commented Oct 23, 2024

stephan-herrmann commented Oct 23, 2024 •

edited

Loading

srikanth-sankaran commented Oct 23, 2024 •

edited

Loading

srikanth-sankaran commented Oct 24, 2024 •

edited

Loading

[Switch][Record Patterns] Unchecked conversion error missing from ECJ #3069

[Switch][Record Patterns] Unchecked conversion error missing from ECJ #3069

Conversation

stephan-herrmann commented Oct 10, 2024 • edited by srikanth-sankaran Loading

stephan-herrmann commented Oct 10, 2024

srikanth-sankaran commented Oct 10, 2024

srikanth-sankaran left a comment

Choose a reason for hiding this comment

srikanth-sankaran Oct 14, 2024

Choose a reason for hiding this comment

stephan-herrmann Oct 15, 2024

Choose a reason for hiding this comment

srikanth-sankaran Oct 14, 2024

Choose a reason for hiding this comment

stephan-herrmann Oct 15, 2024

Choose a reason for hiding this comment

stephan-herrmann commented Oct 15, 2024

srikanth-sankaran commented Oct 17, 2024

srikanth-sankaran left a comment

Choose a reason for hiding this comment

stephan-herrmann commented Oct 22, 2024

srikanth-sankaran commented Oct 23, 2024

srikanth-sankaran commented Oct 23, 2024 • edited Loading

stephan-herrmann commented Oct 23, 2024

stephan-herrmann commented Oct 23, 2024

stephan-herrmann commented Oct 23, 2024 • edited Loading

srikanth-sankaran commented Oct 23, 2024 • edited Loading

srikanth-sankaran commented Oct 24, 2024 • edited Loading

stephan-herrmann commented Oct 10, 2024 •

edited by srikanth-sankaran

Loading

srikanth-sankaran commented Oct 23, 2024 •

edited

Loading

stephan-herrmann commented Oct 23, 2024 •

edited

Loading

srikanth-sankaran commented Oct 23, 2024 •

edited

Loading

srikanth-sankaran commented Oct 24, 2024 •

edited

Loading