[Diagnostic] Improve diagnostic for trailing closures in statement conditions #25165

owenv · 2019-05-31T04:27:48Z

This improves the diagnostic for trailing closures in statement conditions to catch cases where one or more trailing closures are used within a chain composed of multiple calls (this is currently only diagnosed if the trailing closure appears at the very end)

For example:

let x: [Int]? = nil
if let sumOfSquares = x?.map{$0*$0}.reduce(0, +) {
  print(sumOfSquares)
}

Used to result in these errors:

error: anonymous closure argument not contained in a closure
if let sumOfSquares = x?.map{$0*$0}.reduce(0, +) {
                             ^
error: anonymous closure argument not contained in a closure
if let sumOfSquares = x?.map{$0*$0}.reduce(0, +) {
                                ^
error: consecutive statements on a line must be separated by ';'
if let sumOfSquares = x?.map{$0*$0}.reduce(0, +) {
                                   ^
                                   ;
error: expression type '(((Int) throws -> _) throws -> [_])?' is ambiguous without more context
if let sumOfSquares = x?.map{$0*$0}.reduce(0, +) {
                      ~~~^~~
error: use of unresolved identifier 'sumOfSquares'
  print(sumOfSquares)
        ^~~~~~~~~~~~

but now can be diagnosed as follows:

error: trailing closure requires parentheses for disambiguation in this context
if let sumOfSquares = x?.map{$0*$0}.reduce(0, +) {
                            ^
                            (      )

Resolves SR8736

rintaro

Welcome to the project! And thank you for working on this!
The overall strategy is good, but there're several more cases we should handle:

func id<T>(fn: () -> T) -> T { return fn() }
func any<T>(fn: () -> T) -> Any { return fn() }

func test() {
  if !id { true } { return } // prefix
  if id { true } == true { return } // binary LHS
  if true == id { true } { return } // binary RHS
  if id { true } ? true : false { return } // ternary condition
  if true ? id { true } : false { return } // ternary then
  if true ? true : id { false } { return } // ternary else
  if id { [false,true] }[0] { return } // subscript
  if id { { true } } () { return } // call
  if any { true } as! Bool { return } // 'as!' casting
  if let _ = any { "test" } as? Int { return } // 'as?' casting
  if any { "test" } is Int { return } // 'is' casting
  if let _ = id { [] as [Int]? }?.first { return } // optional chain
  if id { true as Bool? }! { return } // force unwrapping
  if case id { 1 } = 1 { return } // pattern
}

These all should be diagnosed as single error: trailing closure requires parentheses for disambiguation.

rintaro · 2019-05-31T21:18:52Z

lib/Parse/ParseExpr.cpp

  Parser::BacktrackingScope backtrack(P);
  P.consumeToken(tok::l_brace);
  P.skipUntil(tok::r_brace);
  SourceLoc endLoc;
  if (!P.consumeIf(tok::r_brace, endLoc) ||
-      P.Tok.isNot(tok::l_brace, tok::kw_where, tok::comma)) {
+      P.Tok.isNot(tok::l_brace, tok::kw_where, tok::comma, tok::period)) {


This isn't enough. For example, the . in foo { $0 + 1 } .bar() is tok::period_prefix.

rintaro · 2019-05-31T22:04:57Z

lib/Sema/MiscDiagnostics.cpp

+        return { true, E };
+      } else if (isa<DotSyntaxCallExpr>(E)) {
+        return { true, E };
+      }


To handle binary expressions et al., I think we should maintain "not walk into" list, instead of walk into specific expression kinds.
I mean, currently we walk into only CallExpr (and DotSyntaxCallExpr). But I think we should walk into except ParenExpr, TupleExpr, ArrayExpr, DictionaryExpr, etc.

I ended up adopting a solution like this, my one concern is that a future new expression kind might cause a false positive to be diagnosed if it isn't included in the 'skipped' list. This should be mitigated by the fact that isValidTrailingClosure() still whitelists the individual tokens which may follow a closing brace for something to be considered a possibly valid trailing closure. It doesn't completely eliminate the possibility of future breakage though.

I think it's OK. False positive is better than false negative in this case. The user can easily workaround it by not using trailing closure (or enclose whole expression into parentheses).

…as long as we don't break source compat.

owenv · 2019-06-05T15:19:17Z

Thanks for the feedback @rintaro , I think I've addressed most of the issues with this PR. It's working correctly in all of the cases I've tested so far, but I think it would be a good idea to run this through the source compatibility suite as well since the scope has expanded quite a bit.

The new changes do slightly hurt recovery in the case where a user incorrectly writes an array using c-style postfix brackets enclosing an expression with a trailing closure (see the test case I modified in recovery.swift). The error message in that case was already pretty poor though so I think it's probably a worthwhile tradeoff.

owenv · 2019-06-05T15:22:09Z

Actually, this isn't quite ready yet, sorry, found one more test failure I missed earlier I need to take a look at

owenv · 2019-06-07T02:21:29Z

@rintaro I think I've fixed the remaining regressions in this PR, it's passing all the tests for me locally now. Do you mind taking a look/running the test suite?

rintaro · 2019-06-07T18:24:49Z

lib/Sema/MiscDiagnostics.cpp

+      // expression, don't diagnose it as there is no ambiguity
+      static const std::set<ExprKind> skippedExpressionKinds{
+          ExprKind::Paren, ExprKind::Tuple, ExprKind::Array,
+          ExprKind::Dictionary, ExprKind::InterpolatedStringLiteral};


Instead of using std::set, please use switch for this.

switch (E->getKind()) { case ExprKind::Paren: case ExprKind::Tuple: case ExprKind::Array: case ExprKind::Dictionary: case ExprKind::InterpolatedStringLiteral: // If a trailing closure appears as a child of one of these types of // expression, don't diagnose it as there is no ambiguity. return { E->isImplicit(), E }; case ExprKind::Call diagnoseIt(CE) break; default; break; } return { true, E };

rintaro · 2019-06-07T18:29:22Z

lib/Parse/ParseExpr.cpp

+                  tok::kw_as, tok::question_postfix, tok::question_infix,
+                  tok::exclaim_postfix, tok::colon, tok::equal,
+                  tok::oper_postfix, tok::oper_binary_spaced,
+                  tok::oper_binary_unspaced)) {


Could you insert isAstStartOfLine() check for newly added token kinds?
For example, the following example doesn't look like subscript calling.

if foo { something() } [1, 2, 3].foo()

Like:

switch (Tok.getKind()) { case tok::l_brace: case tok::kw_where: case tok::comma: return true; case tok::l_brace: case tok::l_square: case ...: return !Tok.isAtStartOfLine(); default: return false; }

owenv · 2019-06-08T18:29:23Z

Thanks again for walking me through this @rintaro , I definitely underestimated the complexity of implementing some of these improvements! I pushed another update with some fixes and added tests, and will add more cases if I think of any.

rintaro · 2019-06-10T23:36:01Z

@swift-ci Please Test Source Compatibility

rintaro · 2019-06-10T23:36:09Z

@swift-ci Please smoke test

rintaro · 2019-06-10T23:51:29Z

Thank you! I think this is good to go.
@jrose-apple Do you have any concern?

owenv · 2019-06-11T02:34:08Z

hmm, not really sure what to make of the source compatibility failures, but it looks the exact same set of failures is present in a few other runs (like https://ci.swift.org/job/swift-PR-source-compat-suite-debug/1283/console) as well, so I don't think this PR causes any new failures.

jrose-apple · 2019-06-11T02:38:05Z

lib/Sema/MiscDiagnostics.cpp

+      default:
+        break;
+      }
+      return {true, E};


This part is a change in behavior; is that intentional?

Yes. The intent is to always walk into an expression, unless it's a non-implicit parent/tuple/etc. expression.

jrose-apple · 2019-06-11T02:39:43Z

test/expr/closure/trailing.swift

+
+  if (id { true }) { return } // OK
+
+  if true { }


Since true is a keyword, I'm not sure this is testing exactly what you want to test. Maybe do a let myTrue = true above?

Oops, yeah that's a copy-paste error, 🤦‍♂. I updated the test

…nditions This improves the diagnostic for trailing closures in statement conditions to catch cases where one or more trailing closures are used in a chain composed of multiple calls

jrose-apple · 2019-06-11T16:18:10Z

@swift-ci Please smoke test

owenv · 2019-06-27T18:25:55Z

Just wanted to bump this, lost track of the PR for awhile, sorry about that! Looks like it should still merge cleanly

jrose-apple · 2019-06-27T18:49:41Z

@swift-ci Please smoke test

jrose-apple · 2019-06-27T18:50:00Z

Whoops, guess we lost track of it too. Restarted tests just in case.

jrose-apple · 2019-06-28T16:08:57Z

Thanks, @owenv!

jrose-apple · 2019-06-28T20:58:48Z

*sigh* We found a case where this is source-breaking, so I'm going to have to revert it for now:

let array = [1, 2, 3]
if true != array.contains { $0 > 0 } {}

…ement conditions (#25165)" This reverts commit 4da49fc.

owenv · 2019-06-28T22:44:45Z

Oh, interesting. Thanks for catching this! I’ll take a look at this soon and see if it’s fixable

jrose-apple · 2019-06-28T22:49:13Z

It Would Be Nice™ to still issue the diagnostic, but as a warning rather than an error in cases where it might have previously been accepted. One way to do that is to always issue it as a warning rather than an error, since anything that was previously an error will still be diagnosed and anything that previously slipped through inconsistently will only get a warning.

… in statement conditions (swiftlang#25165)"" This reverts commit e3a6b67.

jrose-apple requested a review from rintaro May 31, 2019 17:14

rintaro requested changes May 31, 2019

View reviewed changes

owenv force-pushed the improve_trailing_closure_in_condition_diag branch 2 times, most recently from 8fe5950 to c8f59ae Compare June 5, 2019 04:47

owenv force-pushed the improve_trailing_closure_in_condition_diag branch 2 times, most recently from cf27a17 to 4dc7e4a Compare June 7, 2019 02:15

rintaro reviewed Jun 7, 2019

View reviewed changes

owenv force-pushed the improve_trailing_closure_in_condition_diag branch 2 times, most recently from 28ad977 to 9189c9b Compare June 8, 2019 18:18

rintaro approved these changes Jun 10, 2019

View reviewed changes

jrose-apple reviewed Jun 11, 2019

View reviewed changes

[Diagnostic] Improve diagnostic for trailing closures in statement co…

7785b9d

…nditions This improves the diagnostic for trailing closures in statement conditions to catch cases where one or more trailing closures are used in a chain composed of multiple calls

owenv force-pushed the improve_trailing_closure_in_condition_diag branch from 9189c9b to 7785b9d Compare June 11, 2019 03:37

jrose-apple self-assigned this Jun 27, 2019

jrose-apple merged commit 4da49fc into swiftlang:master Jun 28, 2019

jrose-apple added a commit that referenced this pull request Jun 28, 2019

Revert "[Diagnostic] Improve diagnostic for trailing closures in stat…

e3a6b67

…ement conditions (#25165)" This reverts commit 4da49fc.

jrose-apple mentioned this pull request Jun 28, 2019

Revert "[Diagnostic] Improve diagnostic for trailing closures in statement conditions" #25863

Merged

rintaro mentioned this pull request Nov 19, 2019

[SR-5869][Parse/Sema] Expand recovery support for trailing closure in statement conditions #12457

Closed

owenv added a commit to owenv/swift that referenced this pull request Nov 19, 2019

Revert "Revert "[Diagnostic] Improve diagnostic for trailing closures…

a3946cd

… in statement conditions (swiftlang#25165)"" This reverts commit e3a6b67.

owenv mentioned this pull request Nov 19, 2019

[Parse] Reapply trailing closure in condition fix and downgrade to warning #28362

Merged

swift-ci mentioned this pull request May 31, 2019

[SR-8736] Bad diagnostic for use of trailing closures in an if #51245

Closed

[Diagnostic] Improve diagnostic for trailing closures in statement conditions #25165

[Diagnostic] Improve diagnostic for trailing closures in statement conditions #25165

Uh oh!

Conversation

owenv commented May 31, 2019

Uh oh!

rintaro left a comment • 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!

rintaro Jun 7, 2019 • 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!

owenv commented Jun 5, 2019

Uh oh!

owenv commented Jun 5, 2019

Uh oh!

owenv commented Jun 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro Jun 7, 2019 • 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!

owenv commented Jun 8, 2019

Uh oh!

rintaro commented Jun 10, 2019

Uh oh!

rintaro commented Jun 10, 2019

Uh oh!

rintaro commented Jun 10, 2019

Uh oh!

owenv commented Jun 11, 2019

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!

jrose-apple commented Jun 11, 2019

Uh oh!

owenv commented Jun 27, 2019

Uh oh!

jrose-apple commented Jun 27, 2019

Uh oh!

jrose-apple commented Jun 27, 2019

Uh oh!

jrose-apple commented Jun 28, 2019

Uh oh!

jrose-apple commented Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owenv commented Jun 28, 2019

Uh oh!

jrose-apple commented Jun 28, 2019

Uh oh!

Uh oh!

rintaro left a comment •

edited

Loading

rintaro Jun 7, 2019 •

edited

Loading

rintaro Jun 7, 2019 •

edited

Loading

jrose-apple commented Jun 28, 2019 •

edited

Loading