[QoI] Improvements to function call & closure diagnostics #7224

jtbandes · 2017-02-03T08:28:26Z

@rintaro @jrose-apple This is a follow-up to #7202.

Expands the situations in which we suggest do for erroneous trailing closures, and unused closures. Eliminates some duplicate closure diagnostics.

jtbandes · 2017-02-03T08:53:48Z

Hm, as I guess could be expected, most of these failures are due to _ =. Perhaps it's not such a good idea to add this.

rintaro · 2017-02-03T09:43:18Z

lib/Sema/TypeCheckStmt.cpp

+    if (isa<ClosureExpr>(E)) {
+      diagnose(E->getLoc(), diag::expression_unused_closure);
+      diagnose(E->getStartLoc(), diag::brace_stmt_suggest_do)
+        .fixItInsert(E->getStartLoc(), "do ");


Unless the closure hasAnonymousClosureVars(), it's not likely a do statement, IMO.
hasAnonymousClosureVars() roughly means that the closure does not have explicit in clause.

rintaro · 2017-02-03T09:49:57Z

test/Parse/recovery.swift

@@ -44,15 +44,15 @@ func test(a: BadAttributes) -> () { // expected-note * {{did you mean 'test'?}}
 //===--- Recovery for braced blocks.

 func braceStmt1() {
-  { braceStmt1(); } // expected-error {{braced block of statements is an unused closure}} expected-error {{expression resolves to an unused function}}
+  { braceStmt1(); } // expected-error {{closure expression is unused}} expected-note {{use 'do' keyword to designate a separate block of statements}} {{3-3=do }}


note message here doesn't make sense to me.
How about "did you mean 'do' statement?" ?

I was also thinking of using the word scope or scoped somehow.

But I think did you mean to use a 'do' statement? could work... (especially since "do statement" is the terminology used in the documentation)

rintaro · 2017-02-03T10:36:28Z

test/Parse/recovery.swift

 }

 func braceStmt3() {
-  { // expected-error {{braced block of statements is an unused closure}}


It's a little unfortunate that we lost this diagnostics.
This is because typeCheckExpression() fails for this single expression closure in StmtChecker::visitBraceStmt().

Agreed, we definitely need some diagnostic here.

The reason there's no longer a diagnostic here is that there was an error typechecking the closure body. I'm not sure how to convince it to produce additional diagnostics in that case...

rintaro · 2017-02-03T11:02:14Z

lib/Sema/TypeCheckStmt.cpp

+    if (suggestDiscardResult) {
+      diagnose(valueE->getLoc(), diag::discard_result_of_call)
+      .fixItInsert(valueE->getStartLoc(), "_ = ");
+    }


I recommend to split this fix-it addition into a separate PR.

Please don't defer side effects like this, either. That's a bit too subtle for C++ code.

I also considered a fix-it to wrap the call in parentheses, like ({ print("result") }()). What do you think of that option?

It was a bit tricky to get it to happen at the right time, because starting an expression with a closure only seems to be disallowed when it's part of the top-level code (which was a bit surprising to me). But I could probably make it work with some more effort.

rintaro · 2017-02-03T11:22:10Z

Moving diagnosis to Sema makes sense to me. This is to prevent double error:

error: braced block of statements is an unused closure
{
^
error: expression resolves to an unused function
{
^

Also, I like do instead of _ = for closures :)

jrose-apple · 2017-02-03T16:46:51Z

lib/Sema/TypeCheckStmt.cpp

+    if (suggestDiscardResult) {
+      diagnose(valueE->getLoc(), diag::discard_result_of_call)
+      .fixItInsert(valueE->getStartLoc(), "_ = ");
+    }


Please don't defer side effects like this, either. That's a bit too subtle for C++ code.

jrose-apple · 2017-02-03T16:48:20Z

test/Constraints/closures.swift

+  { consume($0) }(42)  // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}} expected-error {{cannot call value of non-function type '()'}} expected-note {{use 'do' keyword to designate a separate block of statements}}
+  ;
+
+  // This is technically a valid call, so nothing goes wrong until (42)


Ew. Should we forbid trailing closures after closure literals, too?

I wish, but that'd be a source-breaking change. See the bottom of this function.

I'm not sure it's a language change that would affect anyone in practice. @DougGregor?

I think it's reasonable to forbid trailing closures after closure literals. Better to beg forgiveness (== allow it only in Swift 3 compatibility mode if it becomes a problem) than keep it in the grammar.

Is it intended that closures are allowed to begin an expression that's not at the top level? I think the IIFE pattern is common, but do is better if it's a ->Void closure.

We could certainly start parsing that way. The existing warning was intended to keep people from doing it, and to require parentheses if they really wanted to create-and-call all in one go.

Well, the thing is it's not a warning today. This compiles with no complaints:

func foo() { { print("hi") }() }

It's only an error at the top level, when it gets confused with a trailing closure, or when it produces an unused closure (i.e. isn't called).

I'm also not sure what the point is of rejecting this at the top level.

jrose-apple · 2017-02-03T16:49:31Z

test/Parse/recovery.swift

 }

 func braceStmt3() {
-  { // expected-error {{braced block of statements is an unused closure}}


Agreed, we definitely need some diagnostic here.

rintaro · 2017-02-04T11:35:18Z

test/Parse/recovery.swift

 }

 func braceStmt3() {
-  { // expected-error {{braced block of statements is an unused closure}}
+  {


Why we want diagnostics here is: it's inconsistent with multi statements closure.
Consider:

func foo() { // ... { print("do it.") undefinedFunc() } }

In this case, it's diagnosed as closure expression is unused did you mean to use a 'do' statement? despite the error in undefinedFunc().

jtbandes · 2017-02-04T19:11:23Z

Trying checkIgnoredExpr regardless of whether typechecking succeeded...

@swift-ci please smoke test

jtbandes · 2017-02-06T00:36:28Z

@swift-ci please smoke test

rintaro · 2017-02-07T03:52:11Z

lib/Sema/TypeCheckStmt.cpp

+      // If a closure expression is unused, the user might have intended
+      // to write "do { ... }".
+      if (auto *CE = dyn_cast<ClosureExpr>(SubExpr)) {
+        TC.diagnose(CE->getLoc(), diag::expression_unused_closure);


IMO, we should handle CaptureListExpr here as well.

test.swift:2:3: error: expression resolves to an unused function { [x] in ^~~~~~~~

closure expression is unused is better I think.

Wow, I didn't realize CaptureListExpr exists as a parent of ClosureExpr. Good idea.

Thanks for pointing this out. I also found that my use of cast<> was crashing because I failed to account for CaptureListExpr.

rintaro · 2017-02-07T03:52:24Z

lib/Sema/TypeCheckStmt.cpp

+            .fixItInsert(CE->getStartLoc(), "do ");
+        }
+      }
+      else if (isDiscarded && !hadTypeError)


style nitpick } else

jtbandes · 2017-02-07T07:20:12Z

@swift-ci please smoke test

DougGregor · 2017-02-07T19:01:03Z

I'm happy with this now, FWIW

jrose-apple · 2017-02-07T19:44:00Z

Abdicating; if @rintaro's also okay with this, go ahead.

jtbandes · 2017-02-07T19:48:33Z

lib/Sema/TypeCheckStmt.cpp

+
+      // If a closure expression is unused, the user might have intended
+      // to write "do { ... }".
+      auto *CE = dyn_cast<ClosureExpr>(SubExpr);


I noticed that I'm not actually checking isDiscarded here. Surprised this didn't break some tests. Will fix this evening.

isDiscarded here means it's in Playground or REPL context or not.
In the current master, "closure as a statement" is diagnosed in Parser regardless of that.
It's considered as a syntactical error. I don't think you need to take it into account.

Oh good point; if we're already in a BraceStmt and the immediate child is a closure, it must be "unused". I hadn't actually read the isDiscarded definition above.

rintaro · 2017-02-08T01:19:10Z

lib/Sema/TypeCheckStmt.cpp

+
+      // If a closure expression is unused, the user might have intended
+      // to write "do { ... }".
+      auto *CE = dyn_cast<ClosureExpr>(SubExpr);


isDiscarded here means it's in Playground or REPL context or not.
In the current master, "closure as a statement" is diagnosed in Parser regardless of that.
It's considered as a syntactical error. I don't think you need to take it into account.

jtbandes requested review from rintaro and jrose-apple February 3, 2017 08:28

jtbandes force-pushed the func-y-town branch from 079946b to 0dcb88e Compare February 3, 2017 08:34

rintaro reviewed Feb 3, 2017

View reviewed changes

jrose-apple reviewed Feb 3, 2017

View reviewed changes

jtbandes force-pushed the func-y-town branch 3 times, most recently from 226df6a to fbbbacc Compare February 4, 2017 08:47

rintaro reviewed Feb 4, 2017

View reviewed changes

jtbandes force-pushed the func-y-town branch 2 times, most recently from d8e00a4 to 4f06d4e Compare February 4, 2017 19:10

jtbandes force-pushed the func-y-town branch 4 times, most recently from 9cfddfb to 48da5bb Compare February 6, 2017 00:36

rintaro reviewed Feb 7, 2017

View reviewed changes

jtbandes force-pushed the func-y-town branch 2 times, most recently from a53d306 to 6ebf000 Compare February 7, 2017 07:18

[QoI] Improvements to function call & closure diagnostics

035db70

jtbandes force-pushed the func-y-town branch from 6ebf000 to 035db70 Compare February 7, 2017 07:19

jtbandes assigned rintaro and jrose-apple Feb 7, 2017

jrose-apple removed their assignment Feb 7, 2017

jtbandes commented Feb 7, 2017

View reviewed changes

rintaro approved these changes Feb 8, 2017

View reviewed changes

jtbandes merged commit c98e515 into swiftlang:master Feb 8, 2017

jtbandes deleted the func-y-town branch February 8, 2017 01:36

[QoI] Improvements to function call & closure diagnostics #7224

[QoI] Improvements to function call & closure diagnostics #7224

Uh oh!

Conversation

jtbandes commented Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtbandes commented Feb 3, 2017

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!

jtbandes Feb 3, 2017 • 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 commented Feb 3, 2017

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!

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!

rintaro Feb 4, 2017 • 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!

jtbandes commented Feb 4, 2017

Uh oh!

jtbandes commented Feb 6, 2017

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!

jtbandes commented Feb 7, 2017

Uh oh!

DougGregor commented Feb 7, 2017

Uh oh!

jrose-apple commented Feb 7, 2017

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

jtbandes commented Feb 3, 2017 •

edited

Loading

jtbandes Feb 3, 2017 •

edited

Loading

rintaro Feb 4, 2017 •

edited

Loading