Teach the "unused variable / value" diagnostics to use SwiftIfConfig

Rather than walking into the inactive regions of IfConfigDecls looking for
references to a declaration before we diagnose it, go to the syntax
tree and look through inactive *and unparsed* regions for identifier
tokens that match. If we find one, suppress the diagnostic.

This reduces our dependency on IfConfigDecl in the AST, and also makes
the same suppression work with code in unparsed regions that had no
representation in IfConfigDecl.

Author: DougGregor

lib/ASTGen/Sources/ASTGen/CompilerBuildConfiguration.swift

@@ -334,6 +334,83 @@ public func freeConfiguredRegions(
   UnsafeMutableBufferPointer(start: regions, count: numRegions).deallocate()
 }
 
+/// Cache used when checking for inactive code that might contain a reference
+/// to specific names.
+private struct InactiveCodeContainsReferenceCache {
+  let syntax: SourceFileSyntax
+  let configuredRegions: ConfiguredRegions
+}
+
+/// Determine whether the inactive code within the given search range
+/// contains a token referring to the given name.
+@_cdecl("swift_ASTGen_inactiveCodeContainsReference")
+public func inactiveCodeContainsReference(
+  astContext: BridgedASTContext,
+  sourceFileBuffer: BridgedStringRef,
+  searchRange: BridgedStringRef,
+  bridgedName: BridgedStringRef,
+  untypedCachePtr: UnsafeMutablePointer<UnsafeMutableRawPointer?>
+) -> Bool {
+  let syntax: SourceFileSyntax
+  let configuredRegions: ConfiguredRegions
+  if let untypedCachePtr = untypedCachePtr.pointee {
+    // Use the cache.
+    let cache = untypedCachePtr.assumingMemoryBound(to: InactiveCodeContainsReferenceCache.self)
+    syntax = cache.pointee.syntax
+    configuredRegions = cache.pointee.configuredRegions
+  } else {
+    // Parse the region.
+
+    // FIXME: Use 'ExportedSourceFile' when C++ parser is replaced.
+    let searchRangeBuffer = UnsafeBufferPointer<UInt8>(start: searchRange.data, count: searchRange.count)
+    syntax = Parser.parse(source: searchRangeBuffer)
+
+    // Compute the configured regions within the search range.
+    let configuration = CompilerBuildConfiguration(
+      ctx: astContext,
+      sourceBuffer: searchRangeBuffer
+    )
+    configuredRegions = syntax.configuredRegions(in: configuration)
+
+    let cache = UnsafeMutablePointer<InactiveCodeContainsReferenceCache>.allocate(capacity: 1)
+    cache.initialize(to: .init(syntax: syntax, configuredRegions: configuredRegions))
+
+    untypedCachePtr.pointee = .init(cache)
+  }
+
+  // If there are no regions, everything is active. This is the common case.
+  if configuredRegions.isEmpty {
+    return false
+  }
+
+  // Walk all of the tokens in the search range looking for one that references
+  // the given name.
+  let name = String(bridged: bridgedName)
+  for token in syntax.tokens(viewMode: .sourceAccurate) {
+    guard let identifier = token.identifier else {
+      continue
+    }
+
+    if identifier.name == name && configuredRegions.isActive(token) != .active {
+      // Found it in a non-active region.
+      return true
+    }
+  }
+
+  return false
+}
+
+@_cdecl("swift_ASTGen_freeInactiveCodeContainsReferenceCache")
+public func freeInactiveCodeContainsReferenceCache(pointer: UnsafeMutableRawPointer?) {
+  guard let pointer else {
+    return
+  }
+
+  let cache = pointer.assumingMemoryBound(to: InactiveCodeContainsReferenceCache.self)
+  cache.deinitialize(count: 1)
+  cache.deallocate()
+}
+
 /// Evaluate the #if condition at ifClauseLocationPtr.
 @_cdecl("swift_ASTGen_evaluatePoundIfCondition")
 public func evaluatePoundIfCondition(

lib/Sema/MiscDiagnostics.cpp

@@ -19,6 +19,7 @@
 #include "TypeCheckConcurrency.h"
 #include "TypeCheckInvertible.h"
 #include "TypeChecker.h"
+#include "swift/AST/ASTBridging.h"
 #include "swift/AST/ASTWalker.h"
 #include "swift/AST/ConformanceLookup.h"
 #include "swift/AST/DiagnosticsSema.h"
@@ -3160,6 +3161,9 @@ class VarDeclUsageChecker : public ASTWalker {
   DeclContext *DC;
 
   DiagnosticEngine &Diags;
+
+  SourceRange FullRange;
+
   // Keep track of some information about a variable.
   enum {
     RK_Defined     = 1,      ///< Whether it was ever defined in this scope.
@@ -3195,8 +3199,9 @@ class VarDeclUsageChecker : public ASTWalker {
   void operator=(const VarDeclUsageChecker &) = delete;
 
 public:
-  VarDeclUsageChecker(DeclContext *DC,
-                      DiagnosticEngine &Diags) : DC(DC), Diags(Diags) {}
+  VarDeclUsageChecker(DeclContext *DC, DiagnosticEngine &Diags,
+                      SourceRange range)
+      : DC(DC), Diags(Diags), FullRange(range) {}
 
   // After we have scanned the entire region, diagnose variables that could be
   // declared with a narrower usage kind.
@@ -3266,11 +3271,6 @@ class VarDeclUsageChecker : public ASTWalker {
     if (isa<TypeDecl>(D))
       return Action::SkipNode();
 
-    // The body of #if clauses are not walked into, we need custom processing
-    // for them.
-    if (auto *ICD = dyn_cast<IfConfigDecl>(D))
-      handleIfConfig(ICD);
-      
     // If this is a VarDecl, then add it to our list of things to track.
     if (auto *vd = dyn_cast<VarDecl>(D)) {
       if (shouldTrackVarDecl(vd)) {
@@ -3354,9 +3354,6 @@ class VarDeclUsageChecker : public ASTWalker {
   /// The heavy lifting happens when visiting expressions.
   PreWalkResult<Expr *> walkToExprPre(Expr *E) override;
 
-  /// handle #if directives.
-  void handleIfConfig(IfConfigDecl *ICD);
-
   /// Custom handling for statements.
   PreWalkResult<Stmt *> walkToStmtPre(Stmt *S) override {
     // Keep track of an association between vardecls and the StmtCondition that
@@ -3864,6 +3861,16 @@ SourceLoc swift::getFixItLocForVarToLet(VarDecl *var) {
   return SourceLoc();
 }
 
+extern "C" bool swift_ASTGen_inactiveCodeContainsReference(
+    BridgedASTContext ctx,
+    BridgedStringRef sourceFileBuffer,
+    BridgedStringRef searchRange,
+    BridgedStringRef name,
+    void **cache
+);
+
+extern "C" void swift_ASTGen_freeInactiveCodeContainsReferenceCache(void *cache);
+
 // After we have scanned the entire region, diagnose variables that could be
 // declared with a narrower usage kind.
 VarDeclUsageChecker::~VarDeclUsageChecker() {
@@ -3873,6 +3880,35 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
   if (sawError)
     return;
 
+  /// Check whether the given variable might have been referenced from an
+  /// inactive region of the source code.
+  void *usedInInactiveCache = nullptr;
+  auto isUsedInInactive = [&](VarDecl *var) -> bool {
+#if SWIFT_BUILD_SWIFT_SYNTAX
+    // Extract the full buffer containing the source range.
+    ASTContext &ctx = DC->getASTContext();
+    SourceManager &sourceMgr = ctx.SourceMgr;
+    auto bufferID = sourceMgr.findBufferContainingLoc(FullRange.Start);
+    StringRef sourceFileText = sourceMgr.getEntireTextForBuffer(bufferID);
+
+    // Extract the search text from that buffer.
+    auto searchTextCharRange = Lexer::getCharSourceRangeFromSourceRange(
+        sourceMgr, FullRange);
+    StringRef searchText = sourceMgr.extractText(searchTextCharRange, bufferID);
+
+    return swift_ASTGen_inactiveCodeContainsReference(
+        ctx, sourceFileText, searchText, var->getName().str(),
+        &usedInInactiveCache);
+#else
+    return false;
+#endif
+  };
+  SWIFT_DEFER {
+#if SWIFT_BUILD_SWIFT_SYNTAX
+    swift_ASTGen_freeInactiveCodeContainsReferenceCache(usedInInactiveCache);
+#endif
+  };
+
   for (auto p : VarDecls) {
     VarDecl *var;
     unsigned access;
@@ -3907,7 +3943,7 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
         auto VD = dyn_cast<VarDecl>(FD->getStorage());
         if ((access & RK_Read) == 0) {
           auto found = AssociatedGetterRefExpr.find(VD);
-          if (found != AssociatedGetterRefExpr.end()) {
+          if (found != AssociatedGetterRefExpr.end() && !isUsedInInactive(VD)) {
             auto *DRE = found->second;
             Diags.diagnose(DRE->getLoc(), diag::unused_setter_parameter,
                            var->getName());
@@ -3941,6 +3977,9 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
       // produce a fixit hint with a parent map, but this is a lot of effort for
       // a narrow case.
       if (access & RK_CaptureList) {
+        if (isUsedInInactive(var))
+          continue;
+
         Diags.diagnose(var->getLoc(), diag::capture_never_used,
                        var->getName());
         continue;
@@ -3954,6 +3993,9 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
       if (auto *pbd = var->getParentPatternBinding()) {
         if (pbd->getSingleVar() == var && pbd->getInit(0) != nullptr &&
             !isa<TypedPattern>(pbd->getPattern(0))) {
+          if (isUsedInInactive(var))
+            continue;
+
           unsigned varKind = var->isLet();
           SourceRange replaceRange(
               pbd->getStartLoc(),
@@ -3984,6 +4026,9 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
               if (isa<NamedPattern>(LP->getSubPattern())) {
                 auto initExpr = SC->getCond()[0].getInitializer();
                 if (initExpr->getStartLoc().isValid()) {
+                  if (isUsedInInactive(var))
+                    continue;
+
                   unsigned noParens = initExpr->canAppendPostfixExpression();
 
                   // If the subexpr is an "as?" cast, we can rewrite it to
@@ -4051,6 +4096,9 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
         });
 
         if (foundVP) {
+          if (isUsedInInactive(var))
+            continue;
+
           unsigned varKind = var->isLet();
           Diags
               .diagnose(var->getLoc(), diag::variable_never_used,
@@ -4062,6 +4110,9 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
 
       // Otherwise, this is something more complex, perhaps
       //    let (a,b) = foo()
+      if (isUsedInInactive(var))
+        continue;
+
       if (isWrittenLet) {
         Diags.diagnose(var->getLoc(),
                        diag::immutable_value_never_used_but_assigned,
@@ -4085,6 +4136,9 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
         !isVarDeclPartOfPBDThatHadSomeMutation(var)) {
       SourceLoc FixItLoc = getFixItLocForVarToLet(var);
 
+      if (isUsedInInactive(var))
+        continue;
+
       // If this is a parameter explicitly marked 'var', remove it.
       if (FixItLoc.isInvalid()) {
         Diags.diagnose(var->getLoc(), diag::variable_never_mutated,
@@ -4113,6 +4167,9 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
 
     // If this is a variable that was only written to, emit a warning.
     if ((access & RK_Read) == 0) {
+      if (isUsedInInactive(var))
+        continue;
+
       Diags.diagnose(var->getLoc(), diag::variable_never_read, var->getName());
       continue;
     }
@@ -4334,56 +4391,12 @@ ASTWalker::PreWalkResult<Expr *> VarDeclUsageChecker::walkToExprPre(Expr *E) {
   return Action::Continue(E);
 }
 
-/// handle #if directives.  All of the active clauses are already walked by the
-/// AST walker, but we also want to handle the inactive ones to avoid false
-/// positives.
-void VarDeclUsageChecker::handleIfConfig(IfConfigDecl *ICD) {
-  struct ConservativeDeclMarker : public ASTWalker {
-    VarDeclUsageChecker &VDUC;
-    SourceFile *SF;
-
-    ConservativeDeclMarker(VarDeclUsageChecker &VDUC)
-      : VDUC(VDUC), SF(VDUC.DC->getParentSourceFile()) {}
-
-    MacroWalking getMacroWalkingBehavior() const override {
-      return MacroWalking::Arguments;
-    }
-
-    PostWalkResult<Expr *> walkToExprPost(Expr *E) override {
-      // 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);
-      else if (auto *declRef = dyn_cast<UnresolvedDeclRefExpr>(E)) {
-        auto name = declRef->getName();
-        auto loc = declRef->getLoc();
-        if (name.isSimpleName() && loc.isValid()) {
-          auto *varDecl = dyn_cast_or_null<VarDecl>(
-            ASTScope::lookupSingleLocalDecl(SF, name.getFullName(), loc));
-          if (varDecl)
-            VDUC.addMark(varDecl, RK_Read|RK_Written);
-        }
-      }
-      return Action::Continue(E);
-    }
-  };
-
-  for (auto &clause : ICD->getClauses()) {
-    // Active clauses are handled by the normal AST walk.
-    if (clause.isActive) continue;
-
-    for (auto elt : clause.Elements)
-      elt.walk(ConservativeDeclMarker(*this));
-  }
-}
-
 /// Apply the warnings managed by VarDeclUsageChecker to the top level
 /// code declarations that haven't been checked yet.
 void swift::
 performTopLevelDeclDiagnostics(TopLevelCodeDecl *TLCD) {
   auto &ctx = TLCD->getDeclContext()->getASTContext();
-  VarDeclUsageChecker checker(TLCD, ctx.Diags);
+  VarDeclUsageChecker checker(TLCD, ctx.Diags, TLCD->getSourceRange());
   TLCD->walk(checker);
 }
 
@@ -4398,7 +4411,7 @@ void swift::performAbstractFuncDeclDiagnostics(AbstractFunctionDecl *AFD) {
     // declared as constants. Skip local functions though, since they will
     // be checked as part of their parent function or TopLevelCodeDecl.
     auto &ctx = AFD->getDeclContext()->getASTContext();
-    VarDeclUsageChecker checker(AFD, ctx.Diags);
+    VarDeclUsageChecker checker(AFD, ctx.Diags, AFD->getBody()->getSourceRange());
     AFD->walk(checker);
   }
 

test/decl/var/usage.swift

@@ -237,9 +237,14 @@ func testForceValueExpr() {
 func testBuildConfigs() {
   let abc = 42    // no warning.
   var mut = 18    // no warning.
+  let other = 15  // no warning?
+  var othermut = 15  // no warning
 #if false
   mut = abc    // These uses prevent abc/mut from being unused/unmutated.
 #endif
+#if compiler(>=10.0)
+  othermut = other    // This use prevents other/othermut from being unused/unmutated
+#endif
 }
 
 // same as above, but with a guard statement