search

eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
164 stars 130 forks source link

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

Closed stephan-herrmann closed 2 weeks ago

stephan-herrmann commented 4 weeks ago

fixes https://github.com/eclipse-jdt/eclipse.jdt.core/issues/3066 fixes https://github.com/eclipse-jdt/eclipse.jdt.core/issues/3074

stephan-herrmann commented 4 weeks ago

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

srikanth-sankaran commented 4 weeks ago

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 https://github.com/eclipse-jdt/eclipse.jdt.core/issues/2720 early next week I’ll join you in the grammar work in full earnest.

stephan-herrmann commented 3 weeks ago

@srikanth-sankaran after my first commit focused on (local) correctness, in https://github.com/eclipse-jdt/eclipse.jdt.core/commit/df50953411e7818a54e426a88611a7642e8c7d1b 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 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 commented 3 weeks ago

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

stephan-herrmann commented 2 weeks ago

@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 commented 2 weeks ago

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 https://github.com/eclipse-jdt/eclipse.jdt.core/issues/3038 which should solve https://github.com/eclipse-jdt/eclipse.jdt.core/issues/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 commented 2 weeks ago

I'll try and propose a patch for https://github.com/eclipse-jdt/eclipse.jdt.core/issues/3038 that will also address https://github.com/eclipse-jdt/eclipse.jdt.core/issues/3121 and the present ticket https://github.com/eclipse-jdt/eclipse.jdt.core/pull/3069

Bonus points if I manage to get https://github.com/eclipse-jdt/eclipse.jdt.core/issues/3119 and https://github.com/eclipse-jdt/eclipse.jdt.core/issues/3074 fixed in the process.

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

stephan-herrmann commented 2 weeks ago

(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 commented 2 weeks ago

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 commented 2 weeks ago

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 :smile: and finds the more specific pattern type, TADA :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 commented 2 weeks ago

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

srikanth-sankaran commented 2 weeks ago

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 https://github.com/eclipse-jdt/eclipse.jdt.core/issues/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 https://github.com/eclipse-jdt/eclipse.jdt.core/issues/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.