[TypeChecker] SR-7307 : Warn when a block contains defer as last statement #17967

vguerra · 2018-07-15T20:22:52Z

This PR adds a diagnostic (~~NOTE~~ WARNING ) when a defer statement is the last statement within a block of code. The diagnostic is implemented as follows:

During type-checking a BraceStmt simply check if last statement of it's elements is a DeferStmt. If that is the case, emit diagnostic.
A flag (IsBraceStmtFromTopLevelDecl) has been added to the StmtChecker in order to keep track of whether the BraceStmt being checked belongs to a TopLevelCodeDecl or not. This is needed to avoid diagnosing every defer statement found at the top level. (check discussion in : SR-7307)
As a consequence of prev. point, this solution is not able to diagnose a defer statement that appears last at the top level ( not sure if this is worth adding ).

Resolves SR-7307.

xwu

Mostly some formatting nits. My two substantive pieces of feedback are:

The diagnostic should, I think, be more descriptive; a user who has the misunderstanding described in the linked bug won't be educated by seeing a diagnostic that says "pointless." Maybe something along the lines of saying that the body will not be deferred because it's already at the end of the block.

My second concern is how this interacts with initializers. As you know, setting a property inside an initializer doesn't invoke any didSet or willSet routines. However, whether a bug or no, I recall there's been some issue where a defer statement can be used inside an initializer to get around that. I don't recall if that's still the case. But, if so, a defer statement at the end of an initializer isn't actually pointless as removing it would change the observed behavior.

xwu · 2018-07-15T22:38:49Z

include/swift/AST/DiagnosticsCommon.def

@@ -54,6 +54,9 @@ NOTE(previous_decldef,none,
 NOTE(brace_stmt_suggest_do,none,
     "did you mean to use a 'do' statement?", ())

+NOTE(defer_stmt_at_blockend,none,


Nit: "block_end" (two words).

Fixed in next commit

xwu · 2018-07-15T22:42:19Z

include/swift/AST/DiagnosticsCommon.def

@@ -54,6 +54,9 @@ NOTE(previous_decldef,none,
 NOTE(brace_stmt_suggest_do,none,
     "did you mean to use a 'do' statement?", ())

+NOTE(defer_stmt_at_blockend,none,
+     "defer statement at the end of block is pointless", ())


Pointless because...? It's important to give a little more explanation here, I think. Also, stylistically, "defer" should be surrounded by single quotes, as shown above with "a 'do' statement".

I'd suggest something like:

'defer' at the end of its scope is redundant and will execute immediately

Probably best for a follow up PR, but it would be nice to have a fixit that simply removes the defer and braces.

It seems it'd be simpler just to suggest do in place of defer.

thanks @xwu and @jszumski for the reviews. I improved the note message as you suggested in the following commit.

I thought as well about the fix-it from the beginning but did not have the time to see how to implement it, I am still getting familiar with the code base :). As @xwu, probably the easier/simpler thing would be to suggest a do. Will look into it.

hi @xwu, @jszumski
I pushed a 3rd commit that simply adds a fix-it to the diagnostic. The fix-it just suggest to replace the defer with a do statement.

xwu · 2018-07-15T22:42:43Z

lib/Sema/TypeCheckStmt.cpp

@@ -352,13 +355,13 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
  };

  StmtChecker(TypeChecker &TC, AbstractFunctionDecl *AFD)
-    : TC(TC), TheFunc(AFD), DC(AFD), IsREPL(false) { }
+    : TC(TC), TheFunc(AFD), DC(AFD), IsREPL(false), IsBraceStmtFromTopLevelDecl(false) { }


Formatting nit: please wrap to 80 characters per Clang style.

Fixed in next commit.

xwu · 2018-07-15T22:43:34Z

lib/Sema/TypeCheckStmt.cpp

@@ -1333,6 +1336,19 @@ void TypeChecker::checkIgnoredExpr(Expr *E) {

 Stmt *StmtChecker::visitBraceStmt(BraceStmt *BS) {
  const SourceManager &SM = TC.Context.SourceMgr;
+
+  // Diagnose defer statement being last one in block ( Only if
+  // BraceStmt does not start a TopLevelDecl )


Formatting nit: please end comments with a period here and elsewhere; also, no spaces or capitalization inside parentheses.

Fixed in next commit.

CodaFi · 2018-07-16T18:34:31Z

lib/Sema/TypeCheckStmt.cpp


  StmtChecker(TypeChecker &TC, DeclContext *DC)
-    : TC(TC), TheFunc(), DC(DC), IsREPL(false) {
+    : TC(TC), TheFunc(), DC(DC), IsREPL(false), IsBraceStmtFromTopLevelDecl(true) {


This is not necessarily true. You'll want to ask isa<TopLevelCodeDecl>(DC).

Unfortunately I can not relay on isa<TopLevelCodeDecl>(DC) because in the following code snippet, it would return true when visiting the { that opens the ifs then block:

let x = 1 let y = 3 if (x > y) { defer { print("not so useful defer stmt.") } }

and I would like to diagnose this block.

vguerra · 2018-07-18T11:29:06Z

@xwu regarding using defer within initializers, I'll double check if that is still needed or not. Thank you for bringing that up.

rudkx · 2018-07-20T01:44:28Z

I'm not aware of other cases where we emit a note in isolation as opposed to being attached to a warning or error.

Do you know if there is precedence for this?

rudkx

Please add a test that the fix-it is applied correctly.

CodaFi · 2018-07-20T18:53:26Z

I'm not aware of other cases where we emit a note in isolation

Oh, good catch. No, that is never correct - and I thought the diagnostic engine would trap on that. This should be a warning.

CodaFi · 2018-07-20T18:53:39Z

include/swift/AST/DiagnosticsCommon.def

@@ -54,6 +54,10 @@ NOTE(previous_decldef,none,
 NOTE(brace_stmt_suggest_do,none,
     "did you mean to use a 'do' statement?", ())

+NOTE(defer_stmt_at_block_end,none,


This should be a warning. Notes are always attached to warnings or errors.

This is fixed in the next commit :)

vguerra · 2018-07-22T19:11:45Z

hi @rudkx! .. thank you for catching this. In my last commit I converted the NOTE into a WARNING and updated the test file to check for the fix-it!

rudkx · 2018-07-23T17:49:10Z

@swift-ci Please smoke test

rudkx · 2018-07-24T18:24:04Z

@swift-ci Please smoke test

vguerra · 2018-07-24T21:11:20Z

a test that has been added recently fails this time :( ( https://ci.swift.org/job/swift-PR-Linux-smoke-test/8474/testReport/junit/Swift(linux-x86_64)/stmt/yield_swift/ ). My bad, I did not rebase locally before running tests.

fix is coming :)

…nd block.

Updating test as well to check that the fix-it is applied correctly.

Usage of 'defer' now emits a warning in some places in the tests, so adding the corresponding 'expected-warning'.

xwu · 2018-07-25T02:22:03Z

include/swift/AST/DiagnosticsCommon.def

@@ -54,6 +54,10 @@ NOTE(previous_decldef,none,
 NOTE(brace_stmt_suggest_do,none,
     "did you mean to use a 'do' statement?", ())

+WARNING(defer_stmt_at_block_end,none,
+     "'defer' at the end of its scope is redundant and will execute immediately; "


@jrose-apple wrote an excellent guide on Swift diagnostic messages. Some points to consider here include using a terse style (known as reduced written register), phrasing as a rule, and matching the tone of other diagnostics. So perhaps:

'defer' statement immediately before end of scope always executes immediately; replace with 'do' statement to silence this warning

thank you for the pointer to the Diagnostics guide... useful stuff 👍 .. and yes the message you suggest definitely reads better. Will update it in next commit.

warning message improved in ffd1366

hamishknight · 2018-07-25T14:42:50Z

include/swift/AST/DiagnosticsCommon.def

@@ -54,6 +54,10 @@ NOTE(previous_decldef,none,
 NOTE(brace_stmt_suggest_do,none,
     "did you mean to use a 'do' statement?", ())

+WARNING(defer_stmt_at_block_end,none,
+     "'defer' at the end of its scope is redundant and will execute immediately; "


Nit: The diagnostic definition is better placed within DiagnosticsSema.def as it's only emitted from Sema. DiagnosticsCommon is for common diagnostics that are emitted from separate parts of the compiler (e.g Parse and Sema).

Ah, good catch. Yes, this belongs in DiagnosticsSema.def.

thank you for catching this ... fixed in ffd1366

@xwu

- moving WARNING definition from DiagnosticsCommon.def to DiagnosticsSema.def - improving WARNING message to better align with guidelines described in docs/Diagnostics.md as suggested by @xwu. - adapting expected warning message in tests.

rudkx · 2018-07-25T21:29:44Z

test/stmt/yield.swift

@@ -82,7 +82,7 @@ func call_yield() {
 struct YieldInDefer {
  var property: String {
    _read {
-      defer {
+      defer { // expectd-warning {{'defer' statement before end of scope always executes immediately}}{{7-12=do}}


typo: expectd

😢 ... fixed on ab513c8

rudkx · 2018-07-25T21:43:27Z

lib/Sema/TypeCheckStmt.cpp


  StmtChecker(TypeChecker &TC, DeclContext *DC)
-    : TC(TC), TheFunc(), DC(DC), IsREPL(false) {
+      : TC(TC), TheFunc(), DC(DC), IsREPL(false),
+        IsBraceStmtFromTopLevelDecl(true) {


I missed this earlier. Rather than setting this to true, can you instead check getContextKind() of the DeclContext that is passed in. That is less fragile, since as written this relies on the fact that the only place we currently use this constructor is when we're passing in a top level decl context.

I think this is kind of similar to what Robert suggested above about using isa<TopLevelCodeDecl>(DC), to what I responded:

Unfortunately I can not relay on isa<TopLevelCodeDecl>(DC) because in the following code snippet, it would return true when visiting the { that opens the ifs then block:

let x = 1 let y = 3 if (x > y) { defer { print("not so useful defer stmt.") } }

and I would like to diagnose this block.

when visiting the open brace of the if DC->getContextKind() returns TopLevelCodeDecl, so I can't rely on that 😞

For that you should have an outer BraceStmt, with an IfStmt nested within it, and a BraceStmt nested under it. So I would expect that when you hit the outermost BraceStmt you would set IsBraceStmtFromTopLevelDecl to false like you are now.

sorry.. I completely missed your point. I see what you mean now, I should base the initialization of the IsBraceStmtFromTopLevelDecl on DC->getContextKind(). Fixed it on da3149d

If DeclContext's kind is TopLevelCodeDecl we are sure to be visiting a BraceStmt that opens a TopLevelDecl.

rudkx · 2018-07-26T23:31:54Z

test/Sema/diag_defer_block_end.swift

@@ -0,0 +1,23 @@
+// RUN: %target-swift-frontend -typecheck -verify %s


You can take care of this in a future commit, but this could actually be // RUN: %target-typecheck-verify-swift without explicit options or %s.

rudkx

LGTM.

rudkx · 2018-07-26T23:32:46Z

@swift-ci Please smoke test

rudkx · 2018-07-27T06:16:44Z

@vguerra I opened #18277 to fix up a bunch of run lines that could be using %target-typecheck-verify-swift.

vguerra · 2018-07-27T07:30:31Z

thanks @rudkx !

vguerra force-pushed the SR7307 branch from 062a0fa to 193daad Compare July 15, 2018 20:37

xwu reviewed Jul 15, 2018

View reviewed changes

CodaFi reviewed Jul 16, 2018

View reviewed changes

vguerra force-pushed the SR7307 branch from 193daad to de021e5 Compare July 17, 2018 21:53

vguerra changed the title ~~[TypeChecker] SR-7307 : Warn when a block contains nothing but a defer statement~~ [TypeChecker] SR-7307 : Warn when a block contains defer as last statement Jul 17, 2018

rudkx requested changes Jul 20, 2018

View reviewed changes

CodaFi reviewed Jul 20, 2018

View reviewed changes

vguerra added 5 commits July 24, 2018 23:15

[TypeChecker] SR-7307 : Diagnose defer statments that appear at the e…

4ced94a

…nd block.

Fixing formating and a more clear explanation for diagnostic.

03fff5d

Adding a fix-it to diagnosting to replace 'defer' with a 'do' statement.

466ddcb

NOTE should actually be a WARNING.

38cf5fe

Updating test as well to check that the fix-it is applied correctly.

Fixing broken tests.

783ab96

Usage of 'defer' now emits a warning in some places in the tests, so adding the corresponding 'expected-warning'.

xwu reviewed Jul 25, 2018

View reviewed changes

hamishknight reviewed Jul 25, 2018

View reviewed changes

addressing comments on PR

ffd1366

- moving WARNING definition from DiagnosticsCommon.def to DiagnosticsSema.def - improving WARNING message to better align with guidelines described in docs/Diagnostics.md as suggested by @xwu. - adapting expected warning message in tests.

vguerra force-pushed the SR7307 branch from a12536a to ffd1366 Compare July 25, 2018 21:19

rudkx reviewed Jul 25, 2018

View reviewed changes

typo on expected-warning.

ab513c8

rudkx reviewed Jul 25, 2018

View reviewed changes

taking into account DeclContext's kind for initialization.

da3149d

If DeclContext's kind is TopLevelCodeDecl we are sure to be visiting a BraceStmt that opens a TopLevelDecl.

rudkx reviewed Jul 26, 2018

View reviewed changes

rudkx approved these changes Jul 26, 2018

View reviewed changes

rudkx merged commit b275edd into swiftlang:master Jul 27, 2018

vguerra deleted the SR7307 branch July 27, 2018 21:10

marcelofabri mentioned this pull request Jan 30, 2019

confirmation about usage of inert_defer realm/SwiftLint#2611

Closed

2 tasks

vguerra mentioned this pull request Jul 15, 2018

[SR-7307] Warn when a block contains nothing but a defer statement #49855

Closed

		@@ -0,0 +1,23 @@
		// RUN: %target-swift-frontend -typecheck -verify %s

[TypeChecker] SR-7307 : Warn when a block contains defer as last statement #17967

[TypeChecker] SR-7307 : Warn when a block contains defer as last statement #17967

Uh oh!

Conversation

vguerra commented Jul 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xwu 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!

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!

vguerra commented Jul 18, 2018

Uh oh!

rudkx commented Jul 20, 2018

Uh oh!

rudkx left a comment

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Jul 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vguerra commented Jul 22, 2018

Uh oh!

rudkx commented Jul 23, 2018

Uh oh!

rudkx commented Jul 24, 2018

Uh oh!

vguerra commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xwu Jul 25, 2018 • 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!

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!

vguerra Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

vguerra commented Jul 15, 2018 •

edited

Loading

xwu left a comment •

edited

Loading

vguerra commented Jul 24, 2018 •

edited

Loading

xwu Jul 25, 2018 •

edited

Loading

vguerra Jul 25, 2018 •

edited

Loading