-
Notifications
You must be signed in to change notification settings - Fork 131
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
[Switch][Record Patterns] Unchecked conversion error missing from ECJ #3069
[Switch][Record Patterns] Unchecked conversion error missing from ECJ #3069
Conversation
@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
Outdated
Show resolved
Hide resolved
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reworked in df50953 - do you like it better now?
return false; | ||
} | ||
applicable = false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reworked in df50953 - do you like it better now?
...ore.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/RecordPatternTest.java
Show resolved
Hide resolved
...ore.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/RecordPatternTest.java
Outdated
Show resolved
Hide resolved
@srikanth-sankaran after my first commit focused on (local) correctness, in df50953 I made an attempt to define a basic rule for assigning responsibilities:
I could imagine that this can be taken even one step further. There are still differences between 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 Let me know if you like better now, or if you see potential for further clarification. |
Yes, this is on my radar. I will review this before tomorrow. Thanks for your patience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - except for the minor points called out.
...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/problem/ProblemReporter.java
Show resolved
Hide resolved
...ore.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/SwitchPatternTest.java
Show resolved
Hide resolved
...s.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/PrimitiveInPatternsTestSH.java
Show resolved
Hide resolved
f80e7ba
to
7e73c2b
Compare
@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
But from Do your think you implementation from #3024 could be extracted / generalized so it can be used for |
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 |
Sorry, I simply forgot to remove that part, done in 7d2f381 :) |
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. |
e329739
to
c6e81db
Compare
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 ? |
Jenkins is green and I'll look through the changes later today. |
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 The comment 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
c6e81db
to
f45e77e
Compare
try to balance responsibility about type checking between
added the 1st(!) test for inapplicable record pattern in switch
fixes #3066
fixes #3074