Advice on SR-580?


(Timothy Wood) #1

Looking through the ‘StarterBug’ tag, I started looking into <https://bugs.swift.org/browse/SR-580>, in which:

func foo(x: Int) -> Int {
var result = x + 1
#if NOT_ENABLED
_ = result
#endif
return result
}

does not warn that “result" was never written to.

The immediate cause seems to be in lib/Sema/MiscDiagnostics.cpp, in VarDeclUsageChecker::handleIfConfig(). This code walks the inactive #if blocks and does:

     // If we see a bound reference to a decl in an inactive #if block, then
     // conservatively mark it read and written. This will silence "variable
     // unused" and "could be marked let" warnings for it.
     if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
       VDUC.addMark(DRE->getDecl(), RK_Read|RK_Written);
     }

In trying to replace that with something that really walks the Expr (by calling VDUC.walkToExprPre() instead or something of the like), I run into a bunch of checks like this:

// Sema leaves some subexpressions null, which seems really unfortunate. It
// should replace them with ErrorExpr.
if (E == nullptr || !E->getType() || E->getType()->is<ErrorType>()) {
   sawError = true;
   return;
}

And, if I dump the AST for the test case, it has this section with all sorts of nulls:

     (#if_stmt
       (#if:
         (unresolved_decl_ref_expr type='<null>' name=NOT_ENABLED specialized=no)
         (elements
           (sequence_expr type='<null>'
             (discard_assignment_expr type='<null>')
             (assign_expr
               (**NULL EXPRESSION**)
               (**NULL EXPRESSION**))
             (declref_expr type='<null>' decl=main.(file).func decl.qqq@/Users/bungi/Desktop/SR-580.swift:2:7 specialized=yes))))

Presumably VarDeclUsageChecker::handleIfConfig() could pass the right subset of RK_Read|RK_Written if the declref_expr had its accessType set, or maybe a more complete AST that could be walked.

So, then I started looking into places that look for IfConfig{Stmt,Decl} and bail, and found ASTWalker.cpp’s Traversal::visitIfConfigStmt(), and TypeCheckStmt.cpp’s visitIfConfigStmt(). I hacked in a walk() call in the Traversal case and broke all sorts of stuff (maybe since I didn’t do something similar in TypeCheckStmt.cpp). That was about the point that I thought I’d ask if I’m digging in the wrong spot =)

Presumably making more work get done for inactive #if branches will slow down the compiler some (hopefully the percentage of existing code inside a #if block at all is pretty low). But, worse, I can imaging there is a ton of other stuff in them that will cause errors if more of the compiler runs on them (whole missing types, selectors, …).

Is this direction even worth pursuing? Maybe I should look into whether there is a quicker way to get the access type of the declref_expr set when it is being created during the inactive #if parsing?

Thanks!

-tim


(Greg Titus) #2

Tim,

FWIW, I think that one got incorrectly marked as a starter bug. I don’t think the bug is fixable as written. You are right about the immediate cause, and I think that the only thing we _can_ do is mark it conservatively, because that part of the code isn’t type-checked at all, and probably can’t be, if it is #if’d out due to it being for some other version of Swift. That variable could be passed to mutable functions or operators which don’t actually exist in this version of Swift, so there’s no knowing for certain whether it’s actually read or written.

  - Greg

···

On Mar 18, 2016, at 2:18 PM, Timothy Wood via swift-dev <swift-dev@swift.org> wrote:

Looking through the ‘StarterBug’ tag, I started looking into <https://bugs.swift.org/browse/SR-580>, in which:

func foo(x: Int) -> Int {
var result = x + 1
#if NOT_ENABLED
_ = result
#endif
return result
}

does not warn that “result" was never written to.

The immediate cause seems to be in lib/Sema/MiscDiagnostics.cpp, in VarDeclUsageChecker::handleIfConfig(). This code walks the inactive #if blocks and does:

     // If we see a bound reference to a decl in an inactive #if block, then
     // conservatively mark it read and written. This will silence "variable
     // unused" and "could be marked let" warnings for it.
     if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
       VDUC.addMark(DRE->getDecl(), RK_Read|RK_Written);
     }

In trying to replace that with something that really walks the Expr (by calling VDUC.walkToExprPre() instead or something of the like), I run into a bunch of checks like this:

// Sema leaves some subexpressions null, which seems really unfortunate. It
// should replace them with ErrorExpr.
if (E == nullptr || !E->getType() || E->getType()->is<ErrorType>()) {
   sawError = true;
   return;
}

And, if I dump the AST for the test case, it has this section with all sorts of nulls:

     (#if_stmt
       (#if:
         (unresolved_decl_ref_expr type='<null>' name=NOT_ENABLED specialized=no)
         (elements
           (sequence_expr type='<null>'
             (discard_assignment_expr type='<null>')
             (assign_expr
               (**NULL EXPRESSION**)
               (**NULL EXPRESSION**))
             (declref_expr type='<null>' decl=main.(file).func decl.qqq@/Users/bungi/Desktop/SR-580.swift:2:7 specialized=yes))))

Presumably VarDeclUsageChecker::handleIfConfig() could pass the right subset of RK_Read|RK_Written if the declref_expr had its accessType set, or maybe a more complete AST that could be walked.

So, then I started looking into places that look for IfConfig{Stmt,Decl} and bail, and found ASTWalker.cpp’s Traversal::visitIfConfigStmt(), and TypeCheckStmt.cpp’s visitIfConfigStmt(). I hacked in a walk() call in the Traversal case and broke all sorts of stuff (maybe since I didn’t do something similar in TypeCheckStmt.cpp). That was about the point that I thought I’d ask if I’m digging in the wrong spot =)

Presumably making more work get done for inactive #if branches will slow down the compiler some (hopefully the percentage of existing code inside a #if block at all is pretty low). But, worse, I can imaging there is a ton of other stuff in them that will cause errors if more of the compiler runs on them (whole missing types, selectors, …).

Is this direction even worth pursuing? Maybe I should look into whether there is a quicker way to get the access type of the declref_expr set when it is being created during the inactive #if parsing?

Thanks!

-tim

_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev