[CS] Diagnose misuse of CheckedCastExpr with ~= #76644

jameesbrown · 2024-09-23T02:06:14Z

Issue

When applying unwrap operator to 'as' or using 'is' in a type-cast pattern (specifically one that binds a value), the diagnostic for the resulting error could provide more guidance and suggest a fix-it.

Example with current diagnostic:

let maybeInt: Any = 1
if case let intValue as? Int = maybeInt {}
            `- error: pattern variable binding cannot appear in an expression

Fix

Added diagnostic invalid_cast_in_pattern and an associated fix-it to remove !/? or replace 'is' with 'as'.
Modified test/Constraints/rdar106598067.swift to account for the new diagnostic.

Example with new diagnostic:

if case let intValue as? Int = maybeInt {}
            ~~~~~~~~ ^ ~~~~~ 
                   //`- error: cannot conditionally downcast in a type-casting pattern
                   // - fix-it: remove '?'
if case let intValue as! Int = maybeInt {}
            ~~~~~~~~ ^ ~~~~~ 
                   //`- error: cannot force downcast in a type-casting pattern
                   // - fix-it: remove '!'
if case let intValue is Int = maybeInt {}
            ~~~~~~~~ ^ ~~~~
                   //`- error: use 'as' keyword to bind a matched value
                   // - fix-it: replace 'is' with 'as'

Resolves #44631.

xedin

Thank you for the contribution! I left a few comments inline but overall this looks good!

xedin · 2024-09-23T17:26:24Z

include/swift/AST/DiagnosticsSema.def

@@ -1419,6 +1419,11 @@ ERROR(optional_chain_isnt_chaining,none,
      ())
 ERROR(pattern_in_expr,none,
      "%0 cannot appear in an expression", (DescriptivePatternKind))
+ERROR(invalid_cast_in_pattern,none,


Let's split this into three distinct diagnostics instead.

Yeah, in retrospect I did overload this unnecessarily.

xedin · 2024-09-23T17:29:54Z

lib/Sema/CSDiagnostics.cpp

+    diagnose(2).fixItReplace(isExpr->getAsLoc(), "as");
+    return true;
+  }
+  llvm_unreachable("There is no other kind of checked cast!");


Nit: let's just return false; here so that we get a "failed to produce a diagnostic" instead of a crash.

xedin · 2024-09-23T21:19:31Z

lib/Sema/CSGen.cpp

+      if (!BE || !isPatternMatchingOperator(BE->getFn()))
+        return ignoreInvalidPattern();
+      CS.recordFix(IgnoreInvalidCastInPattern::create(
+          CS, CS.getConstraintLocator(castExpr)));


This looks like it belongs in InvalidPatternInExprFailure, I don't think we need a special fix kind for this, only a tailored diagnostic type.

Yeah I like that better too.

jameesbrown · 2024-09-23T21:37:49Z

@xedin Thanks for the review! Will address your comments shortly.

xedin · 2024-09-23T21:40:05Z

Please re-request a review once you are done to help me keep track of this.

xedin · 2024-09-24T04:02:32Z

lib/Sema/CSGen.cpp

+      if (BE && isPatternMatchingOperator(BE->getFn())) {
+        anchor = castExpr;
+        isInvalidCast = true;
+      }


I'd say all this logic here can go directly into diagnoseInvalidCheckedCast...

Okay, the only issue I run into with it there is with tuples and emitting multiple of the same error. I was doing it here so I could put the anchor at the CheckedCastExpr and then since the locator is uniqued on that, we wouldn't get the multiple errors. However, maybe that was not a good way for me to go about it. Also, I may have misunderstood your instruction, with the diagnostic -- is it better to create just a specialized function like diagnoseInvalidCheckedCast? Or a separate subclass of FailureDiagnostic?

I will try moving the logic into the diagnostic function and then use hasFixFor potentially to see if we can skip diagnosing a tuple.

is it better to create just a specialized function like diagnoseInvalidCheckedCast? Or a separate subclass of FailureDiagnostic?

Either way is fine, up to you.

I will try moving the logic into the diagnostic function and then use hasFixFor potentially to see if we can skip diagnosing a tuple.

Hm, it's the same diagnostic but at a different location in the expression because the fix gets anchored on each invalid pattern, right? I think that's fine.

Yeah you're right, fix is anchored to each invalid pattern. Good to know it's okay, will remove that check then.

Using an unwrap operator with 'as' or the wrong keyword (i.e. `is`) when already checking a cast via ~= results in error: 'pattern variable binding cannot appear in an expression'. Add a diagnostic that provides more guidance and a fix-it for the removal of ?/! or replacement of 'is' with 'as'.

xedin · 2024-09-24T16:25:53Z

@swift-ci please test

jameesbrown · 2024-09-24T19:28:42Z

@xedin Thanks for your help with this! It seems like the test failures should be fixed by #76679.

xedin · 2024-09-24T19:37:30Z

@swift-ci please test

xedin · 2024-09-24T21:39:30Z

@swift-ci please test Windows platform

xedin · 2024-09-24T21:40:19Z

swiftlang/swift-testing#731
@swift-ci please test Windows platform

xedin · 2024-09-25T02:03:27Z

@swift-ci please test macOS platform

jameesbrown requested review from hborla, xedin and slavapestov as code owners September 23, 2024 02:06

xedin requested changes Sep 23, 2024

View reviewed changes

jameesbrown force-pushed the issue-44631 branch 3 times, most recently from cc5f26f to 74b00a8 Compare September 24, 2024 02:26

xedin reviewed Sep 24, 2024

View reviewed changes

jameesbrown force-pushed the issue-44631 branch 2 times, most recently from 0863d43 to 39cc887 Compare September 24, 2024 05:24

jameesbrown force-pushed the issue-44631 branch from 39cc887 to f0eef0f Compare September 24, 2024 05:46

xedin approved these changes Sep 24, 2024

View reviewed changes

xedin merged commit 762fd4a into swiftlang:main Sep 25, 2024
5 checks passed

jameesbrown deleted the issue-44631 branch September 27, 2024 13:34

[CS] Diagnose misuse of CheckedCastExpr with ~= #76644

[CS] Diagnose misuse of CheckedCastExpr with ~= #76644

Conversation

jameesbrown commented Sep 23, 2024

Issue

Fix

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jameesbrown commented Sep 23, 2024

Uh oh!

xedin commented Sep 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jameesbrown Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jameesbrown Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xedin commented Sep 24, 2024

Uh oh!

jameesbrown commented Sep 24, 2024

Uh oh!

xedin commented Sep 24, 2024

Uh oh!

xedin commented Sep 24, 2024

Uh oh!

xedin commented Sep 24, 2024

Uh oh!

xedin commented Sep 25, 2024

Uh oh!

Uh oh!

Uh oh!

jameesbrown Sep 24, 2024 •

edited

Loading

jameesbrown Sep 24, 2024 •

edited

Loading