Comment by Piyush Verma (JIRA)

Hi All,
Is the scope of this bug:
1.) To warn when `defer` is the only statement in the block.
OR
2.) To warn in all cases when `defer` is at the end of a block.

I had only though of (1), but (2) seems generally useful too. Good idea piverma (JIRA User)!

Comment by Piyush Verma (JIRA)

@jckarter - I am interested in working on this Bug. Can I pick this as my starter bug?
P.S.: I am a beginner and this will be my first bug in the community.

Sure! Let us know if you need any help getting started.

Comment by Piyush Verma (JIRA)

@jckarter - I am facing an issue. How do i get the reference of the immediate parent closure of defer block. Similar to `Optional<AnyFunctionRef> TheFunc`

for the time being this bug is not assigned. I'll assign it to me and tackle it ( piverma (JIRA User), tell me if you still would like to work on this 😉 ).

Comment by Piyush Verma (JIRA)

Hi @vguerra ,
I needed some help regarding my above query.

hello piverma (JIRA User) and @jckarter

I have been trying to tackle this bug during the last weeks and the best I came up with, so far, is adding a check during the type checking phase. More specifically, when the BraceStmt is being type checked ( https://github.com/apple/swift/blob/master/lib/Sema/TypeCheckStmt.cpp#L1334 ).

My approach is very simple, emit a diagnostic message when a BrackeStmt encloses 1 element and this element is a DeferStmt. I.e.:

Stmt *StmtChecker::visitBraceStmt(BraceStmt *BS) {
  const SourceManager &SM = TC.Context.SourceMgr;

  if (BS->getNumElements() == 1) {
    if (auto stmt = BS->getElement(0).dyn_cast<Stmt*>()) {
      if (auto deferStmt = dyn_cast<DeferStmt>(stmt)) {
        TC.diagnose(deferStmt->getStartLoc(), diag::defer_stmt_suggest_useless);
      }
    }
  }
...

this seems to do the job, except in the case when defer statements appear at the top level of the file: E.g.:

using the following code:

defer {
    print("end of program.")
}

let x = 1
let y = 3

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

would generate the following note messages:

$> ./swiftc ../../../../examples/defer.swift
../../../../examples/defer.swift:1:1: note: defer statement is useless here
defer {
^
../../../../examples/defer.swift:9:5: note: defer statement is useless here
    defer {
    ^

and this is due to the fact that all Stmt's at the top level of the file are enclosed within a BraceStmt. The AST of the code above looks as follows ( https://gist.github.com/vguerra/5cfe45b2dda873deefd3cc444fc8d507 ):

$> /swiftc -dump-ast ../../../../examples/defer.swift &> /tmp/ast.txt
(source_file
  (top_level_code_decl
    (brace_stmt
      (defer_stmt
        (func_decl implicit "$defer()" interface type='() -> ()' access=fileprivate
        ...
  (top_level_code_decl
    (brace_stmt
      (pattern_binding_decl
        (pattern_named type='Int' 'x')
        ...
  (top_level_code_decl
    (brace_stmt
      (pattern_binding_decl
        (pattern_named type='Int' 'y')
  (top_level_code_decl
    (brace_stmt
      (if_stmt
      ...

the first DeferStmt in the AST would trigger the check i added due to the fact that it is the only statement within the first BraceStmt

So what I am now trying to figure out is if there is a smarter way to check those BraceStmt at the top level of the file.

I thought of taking advantage of the fact that StmtChecker has a reference to the TypeChecker and DeclContext to differentiate the BraceStmt at the top level but did not succeed. Is there a way i could achieve that?

Or maybe I took the wrong approach to solve this and there is another easier or more straight forward way to implement this ❓ 🙂.

Any comments, suggestions and pointers are more than appreciated 😃

Comment by Piyush Verma (JIRA)

Hi @vguerra ,
Great idea of implementing the check in visitBraceStmt. I find it better than the approach i was trying to implement. This approach solves condition (1). Can we have something like:

if (BS->getNumElements() > 0) {
    if (auto stmt = BS->getElement(BS->getNumElements() - 1).dyn_cast<Stmt*>()) {
      if (auto deferStmt = dyn_cast<DeferStmt>(stmt)) {
        TC.diagnose(deferStmt->getStartLoc(), diag::defer_stmt_suggest_useless);
      }
    }
 }

to tackle condition (1) and (2) both.

Comment by Piyush Verma (JIRA)

@vguerra, @jckarter,

TheFunc.hasValue()

Can this check not help in the case mentioned by @vguerra ? Since its null for top level code.

I mean:

if (TheFunc.hasValue() && BS->getNumElements() > 0) {
    if (auto stmt = BS->getElement(BS->getNumElements() - 1).dyn_cast<Stmt*>()) {
      if (auto deferStmt = dyn_cast<DeferStmt>(stmt)) {
        TC.diagnose(deferStmt->getStartLoc(), diag::defer_stmt_suggest_useless);
      }
    }
  }

hi! piverma (JIRA User)

indeed checking for the last element under the BraceStmt is much better and covers (1) and (2). Will do that 🙂.

Still, I am not sure how to solve the issue about DeferStmt being a top level decl. :/.

Oh nice! .. I did not notice before that {{TheFunc}} is null for those cases 🙂. @jckarter is it safe to base the check on that assumption?

hi piverma (JIRA User),

unfortunately I dont think checking TheFunc would help. I just checked locally and for both BraceStmts with a defer statements in my toy example

TheFunc.hasValue()

is False 🙁

Comment by Piyush Verma (JIRA)

@vguerra - Yeah, my apologies. It doesn't solve the issue. Also it might tend to ignore any defer at the end of the file. 🙁

piverma (JIRA User) no problem at all... thanks for pointing it out anyways 😉.

It would probably be better to have the walker track whether you're in a `TopLevelCodeDecl`, by setting a flag while walking the `BraceStmt` of a `TopLevelCodeDecl`, than to try to check other incidental state.

thank you @jckarter, will give that a try!

hello all!

I just opened a PR for this #17967

PR for this issue has been merged

Awesome, thanks @vguerra!