[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.

stephan-herrmann · 2024-10-10T13:41:13Z

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

-			scope.problemReporter().incompatiblePatternType(this, other, patternType);
-			return false;
+		} else {
+			if (!checkCastTypesCompatibility(scope, patternType, other, null, true)) {


see the swap of arguments?

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?

srikanth-sankaran · 2024-10-14T06:08:22Z

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

+				case X<String>(int x):
+				     ^^^^^^^^^^^^^^^^
+			Type X<String> cannot be safely cast to Object
+			----------


This looks backwards - no ? The error is that Object cannot be safely cast to X and not the other way around ??

ups, right, we keep confusing ourselves by naming parameters as "right" and "left", which was ok-ish, while only one syntax was involved, but in pattern matching, where is right, where is left??
(ironically, I just fixed the same kind of bug a few lines up, and then introduced a new instanced of the bug here).
Next commit will include a bit of renaming.

Fixed in bc53322

srikanth-sankaran · 2024-10-14T06:14:25Z

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

-				"	    ^^^^^^^^^^^^^^^^^^^^^^^^^^\n" +
-				"Incompatible conditional operand types R<capture#1-of ? extends I> and R\n" +
+				"	                 ^^^^^^^^^^^^^\n" +
+				"Type mismatch: cannot convert from R<capture#1-of ? extends I> to R\n" +


This test doesn't compile with javac so unable to verify behavior - bug in javac

I get:

X.java:10: error: illegal start of type if (p instanceof R<>(String a)) { ^ 1 error

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.

srikanth-sankaran · 2024-10-18T04:37:12Z

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

-		leftShortName = leftName;
-		rightShortName = rightName;
+	int problemId = IProblem.IncompatibleTypesInEqualityOperator; // name is misleading, also used for instanceof patterns
+	if (location instanceof Pattern p && p.getEnclosingPattern() instanceof RecordPattern) {


Is the comment about the name IncompatibleTypesInEqualityOperator being misleading valid ?? 4 lines down we seems to be overriding with IncompatibleTypesInConditionalOperator ??

It would seem the choice of emission of IncompatibleTypesInConditionalOperator by org.eclipse.jdt.internal.compiler.ast.InstanceOfExpression.checkForPrimitives(BlockScope, TypeBinding, TypeBinding) via a call to org.eclipse.jdt.internal.compiler.problem.ProblemReporter.notCompatibleTypesError(Expression, TypeBinding, TypeBinding) is the source of confusion. See also the comment in PIPTSH.java

right, the comment looks a bit stale, but the general question stays: is it appropriate to use problem ids designed for conditional expression and equality expression when reporting problems of pattern matching?

I'll remove the comment and raise a new issue for that discussion.

srikanth-sankaran · 2024-10-18T09:04:00Z

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

-				"3. ERROR in X.java (at line 9)\n" +
-				"	case List<String> s: \n" +
-				"	     ^^^^^^^^^^^^^^\n" +
-				"This case label is dominated by one of the preceding case labels\n" +
 				"----------\n");


I gather this diagnostic going away is because of the early exit in CaseStatement ? If so, OK

Right, when isApplicable() returns false, we've seen enough, resolveCasePattern() returns NotAConstant, hence no ResolvedCase is created and SwitchStatement.resolve() has nothing to check dominance for.

srikanth-sankaran · 2024-10-18T09:06:36Z

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

+				Incompatible conditional operand types X and int
+				----------
+				""");
+	}


I so much like javac's message : incompatible types: X cannot be converted to int - I don't know where this conditional operand types terminology originated but I am OK with that staying and perhaps being taken up separately.

'conditional operand types' has been in use for instanceof expressions for at least very very long time (like: for ever?), see, e.g., org.eclipse.jdt.core.tests.compiler.regression.CastTest.test027()

I filed #3119 for follow-up.

+ 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

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

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

problematic re-use of ProblemIDs #3119

Open

stephan-herrmann added 4 commits October 19, 2024 14:47

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

9658789

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

7e73c2b

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

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

stephan-herrmann commented Oct 10, 2024

stephan-herrmann 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

srikanth-sankaran Oct 14, 2024

stephan-herrmann Oct 15, 2024

stephan-herrmann Oct 15, 2024

srikanth-sankaran Oct 14, 2024

stephan-herrmann commented Oct 15, 2024

srikanth-sankaran commented Oct 17, 2024

srikanth-sankaran left a comment

srikanth-sankaran Oct 18, 2024

stephan-herrmann Oct 19, 2024

srikanth-sankaran Oct 18, 2024

stephan-herrmann Oct 19, 2024

srikanth-sankaran Oct 18, 2024

stephan-herrmann Oct 19, 2024

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

Are you sure you want to change the base?

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

Conversation

stephan-herrmann commented Oct 10, 2024

stephan-herrmann commented Oct 10, 2024

Choose a reason for hiding this comment

srikanth-sankaran commented Oct 10, 2024

srikanth-sankaran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment