[Sema] SR-2327: Improve error message when using 'break' inside 'guar… #4286

MnO2 · 2016-08-13T16:27:54Z

What's in this pull request?

Improve the error message what SR-2327 has described.

With

do {
    guard 1 == 2 else { break }
}

prompt the more helpful error message of error: unlabeled 'break' is only allowed inside a loop or switch, a labeled break is required to exit an if or do

Resolved bug number: (SR-2327)

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

Builds the compiler incrementally.
Builds the standard library only for macOS. Simulator standard libraries and
device standard libraries are not built.
lldb is not built.
The test and validation-test targets are run only for macOS. The optimized
version of these tests are not run.

A smoke test on Linux does the following:

Builds the compiler incrementally.
Builds the standard library incrementally.
lldb is built incrementally.
The swift test and validation-test targets are run. The optimized version of these
tests are not run.
lldb is tested.

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

…d' inside 'do' or 'if'

CodaFi · 2016-08-13T16:44:44Z

Very cool!

@swift-ci please smoke test OS X platform.

rintaro · 2016-08-13T17:32:53Z

lib/Sema/TypeCheckStmt.cpp

@@ -767,7 +767,8 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
      // statement, produce a more specific error.
      if (S->getTargetName().empty() && !ActiveLabeledStmts.empty() &&
          (isa<IfStmt>(ActiveLabeledStmts.back()) ||
-           isa<DoStmt>(ActiveLabeledStmts.back())))
+           isa<DoStmt>(ActiveLabeledStmts.back()) ||
+           isa<GuardStmt>(ActiveLabeledStmts.back())))


Does this work for this?

func fn(a: Int) { guard a < 1 else { break } }

In this case, IMO, the message should be normal 'break' is only allowed inside a loop, if, do, or switch.

Can we put if, do, in single quotes, just like break?

@rintaro I've updated the code to handle the case and added your example into the test cases.

@tkremenek I prefer to submit another pull request to change the format of diagnosis message. I am also in favor of quoting if and do, but touching the message in this commit would make some noises on the logic change behind this commit.

@rintaro @MnO2 That works for me.

xwu · 2016-08-14T17:58:51Z

One suggestion: either in this commit or in the follow-up, probably best to change the guard_body_must_not_fallthrough diagnostic message from...

'guard' body may not fall through, consider using 'return' or 'break'

...to...

'guard' body may not fall through, consider using 'return' or labeled 'break'

rintaro · 2016-08-15T03:02:10Z

lib/Sema/TypeCheckStmt.cpp

+            }
+          }
+        }
+      }


Sorry for nit-picking. This doesn't work for

func fn(x: Int) { if x >= 0 { guard x < 1 else { guard x < 2 else { break } return } } }

The rule here is: If ActiveLabeledStmts contains if or do, we can use unlabeled_break_outside_loop.
I think, something like

if (S->getTargetName().empty() && std::any_of(ActiveLabeledStmts.rbegin(), ActiveLabeledStmts.rend(), [&](Stmt *S) -> bool { return isa<IfStmt>(S) || isa<DoStmt>(S); }))

should work.

You are absolutely right, I would change accordingly.

I've made the change and updated the test cases. :-)

rintaro · 2016-08-16T01:57:52Z

lib/Sema/TypeCheckStmt.cpp

+            diagid = diag::unlabeled_break_outside_loop;
+          }
+        }
+      }


I meant, this whole if block can be replaced with:

if (S->getTargetName().empty() && std::any_of(ActiveLabeledStmts.rbegin(), ActiveLabeledStmts.rend(), [&](Stmt *S) -> bool { return isa<IfStmt>(S) || isa<DoStmt>(S); })) diagid = diag::unlabeled_break_outside_loop;

I think we don't need to care about guard statement here.
Am I missing something?

Oh! duh, (to myself) what were you thinking. Thanks for pointing out. I've stolen your snippet and copied-pasted and pushed. :)

jrose-apple · 2016-08-16T02:07:43Z

The intent of "return or break" is to break out of a loop being guarded, not to exit the guard itself.

rintaro · 2016-08-16T02:19:56Z

@jrose-apple I don't understand the point. What do you mean?
We can break out of labeled if or do as well.

LABEL: do {
  guard [condition] else {
    break LABEL
  }
  // ...
}

jrose-apple · 2016-08-16T02:43:31Z

Sorry, yes, you can. But to have the error message suggest a labeled break (per @xwu) might imply that you can't use it with unlabeled break, which of course you can if there's a loop:

for person in friends {
  guard person.name != "Jordan" else { break }
}

xwu · 2016-08-16T02:47:36Z

(D'oh!)

rintaro · 2016-08-16T02:50:01Z

Ah, make sense. You are replying to @xwu's comment 😄

jrose-apple · 2016-08-16T15:49:44Z

lib/Sema/TypeCheckStmt.cpp

-           isa<DoStmt>(ActiveLabeledStmts.back())))
+      if (S->getTargetName().empty() &&
+          std::any_of(ActiveLabeledStmts.rbegin(), ActiveLabeledStmts.rend(),
+                      [&](Stmt *S) -> bool { return isa<IfStmt>(S) || isa<DoStmt>(S); })) {


Nitpick: swiftc style limits lines to 80 columns. Can you reflow this, or use a helper variable?

Sure, I've inserted a few new lines in between.

…d' inside 'do' or 'if'

jrose-apple · 2016-08-16T16:16:58Z

@swift-ci Please test

jrose-apple · 2016-08-16T16:20:48Z

Linux, OS X

rintaro · 2016-08-16T18:16:52Z

Test passed.
The change looks good to me.
Thank you! @MnO2

jrose-apple · 2016-08-16T19:26:57Z

Force-merging. Thanks, Paul!

tkremenek · 2016-08-16T23:56:15Z

@MnO2 please create a pull request for swift-3.0-branch to pull this change in there.

MnO2 · 2016-08-17T05:36:06Z

I cherry-picked the change and created pull-request against swift-3.0-branch: #4345

rintaro reviewed Aug 13, 2016
View reviewed changes

MnO2 force-pushed the sr-2327 branch from 9a240e5 to 4603b73 Compare August 14, 2016 13:28

rintaro reviewed Aug 15, 2016
View reviewed changes

MnO2 force-pushed the sr-2327 branch from 4603b73 to 10102da Compare August 15, 2016 17:04

rintaro reviewed Aug 16, 2016
View reviewed changes

MnO2 force-pushed the sr-2327 branch from 10102da to 8c2d6bc Compare August 16, 2016 15:16

jrose-apple reviewed Aug 16, 2016
View reviewed changes

[Sema] SR-2327: Improve error message when using 'break' inside 'guar…

7a4d1ae

…d' inside 'do' or 'if'

MnO2 force-pushed the sr-2327 branch from 8c2d6bc to 7a4d1ae Compare August 16, 2016 15:53

jrose-apple merged commit af36803 into swiftlang:master Aug 16, 2016

MnO2 mentioned this pull request Aug 17, 2016

[Sema] SR-2327: cherry-pick the change from pull request 4286 for swift-3.0-branch #4345

Merged

1 task

swift-ci mentioned this pull request Aug 13, 2016

[SR-2327] Improve error message when using 'break' inside 'guard' inside 'do' or 'if' #44934

Closed

[Sema] SR-2327: Improve error message when using 'break' inside 'guar… #4286

[Sema] SR-2327: Improve error message when using 'break' inside 'guar… #4286

Uh oh!

Conversation

MnO2 commented Aug 13, 2016 • edited by CodaFi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this pull request?

Resolved bug number: (SR-2327)

Uh oh!

CodaFi commented Aug 13, 2016

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!

tkremenek Aug 14, 2016 • 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!

xwu commented Aug 14, 2016

Uh oh!

rintaro Aug 15, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Aug 16, 2016

Uh oh!

rintaro commented Aug 16, 2016

Uh oh!

jrose-apple commented Aug 16, 2016

Uh oh!

xwu commented Aug 16, 2016

Uh oh!

rintaro commented Aug 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Aug 16, 2016

Uh oh!

jrose-apple commented Aug 16, 2016

Uh oh!

rintaro commented Aug 16, 2016

Uh oh!

jrose-apple commented Aug 16, 2016

Uh oh!

tkremenek commented Aug 16, 2016

Uh oh!

MnO2 commented Aug 17, 2016

Uh oh!

Uh oh!

MnO2 commented Aug 13, 2016 •

edited by CodaFi

Loading

tkremenek Aug 14, 2016 •

edited

Loading

rintaro Aug 15, 2016 •

edited

Loading