[SR-8867] no unused constant warning if local function follows

[mattneub]

Previous ID	SR-8867
Radar	None
Original Reporter	@mattneub
Type	Improvement

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Improvement, DiagnosticsQoI
Assignee	None
Priority	Medium

md5: 11e0a5dbc64b0db877509024dc5754de

Issue Description:

This yields a nice warning about the unused constant:

    func test() {
        let x = 1 // warning
    }

This doesn't:

    func test() {
        let x = 1 // no warning
        func inner() {
        }
    }

The local function seems to make the compiler's analysis-foo go away.

[belkadan]

cc ravikandhadai (JIRA User)

[swift-ci]

Comment by David Moore (JIRA)

Probably an issue with the parent pattern binding used by VarDeclUsageChecker:~VarDeclUsageChecker(), lines 2577-2588 in MiscDiagnostics.cpp.

// If the source of the VarDecl is a trivial PatternBinding with only a
// single binding, rewrite the whole thing into an assignment.
//    let x = foo()
//  ->
//    _ = foo()
if (auto *pbd = var->getParentPatternBinding())
    if (pbd->getSingleVar() == var && pbd->getInit(0) != nullptr &&
        !isa<TypedPattern>(pbd->getPatternList()[0].getPattern())) {
        unsigned varKind = var->isLet();
        SourceRange replaceRange(
            pbd->getStartLoc(),
            pbd->getPatternList()[0].getPattern()->getEndLoc());
        Diags.diagnose(var->getLoc(), diag::pbd_never_used,
                       var->getName(), varKind)
          .fixItReplace(replaceRange, "_");
        continue;
    }

[swift-ci]

Comment by Ravichandhran Kandhadai Madhavan (JIRA)

cc @xedin

It seems the problem is actually the lines 2404 to 2420 in the file MiscDiagnostics.cpp in the Sema phase

if (auto *afd = dyn_cast<AbstractFunctionDecl>(D)) {
      // If this is a nested function with a capture list, mark any captured
      // variables.
      if (afd->isBodyTypeChecked()) {
        for (const auto &capture : afd->getCaptureInfo().getCaptures())
          addMark(capture.getDecl(), RK_Read|RK_Written);
      } else {
        // If the body hasn't been type checked yet, be super-conservative and
        // mark all variables as used.  This can be improved later, e.g. by
        // walking the untype-checked body to look for things that could
        // possibly be used.
        VarDecls.clear();
      }

As suggested by the comments in the else branch, vardecls that are tracked for detecting unused variables are dropped (conservatively) if the body of an inner function is not type checked at that point. In fact, I am surprised to find that there is no dead-code diagnostics at the SIL level, even though there are SIL-level utility functions like `recursivelyDeleteTriviallyDeadInstructions`. I think a SIL level dead code diagnostics phase could be a lot more powerful. I am creating a radar to track this bug.

[xedin]

ravikandhadai (JIRA User) It sounds like it makes sense to re-run this analysis when the body of the function is completely type-checked, not sure if that's possible though...