[swift-dev] Advice on SR-580?

[Timothy Wood]

Looking through the ‘StarterBug’ tag, I started looking into <https://bugs.swift.org/browse/SR-580 <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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-dev/attachments/20160318/960da6ee/attachment.html>