[clang][dataflow] For bugprone-unchecked-optional-access report range (#131055)

Report the range in diagnostics, in addition to the location
in case the range helps disambiguate a little in chained `->`
expressions.
```
b->a->f->x = 1;
^~~~~~~
```
instead of just:
```
b->a->f->x = 1;
^
```
As a followup we should probably also report the location/range
of an `->` if that operator is used. Like:
```
b->a->f->x = 1;
       ^~
```

Author: jvoung

clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp

@@ -19,6 +19,7 @@
 namespace clang::tidy::bugprone {
 using ast_matchers::MatchFinder;
 using dataflow::UncheckedOptionalAccessDiagnoser;
+using dataflow::UncheckedOptionalAccessDiagnostic;
 using dataflow::UncheckedOptionalAccessModel;
 
 static constexpr llvm::StringLiteral FuncID("fun");
@@ -52,14 +53,16 @@ void UncheckedOptionalAccessCheck::check(
   UncheckedOptionalAccessDiagnoser Diagnoser(ModelOptions);
   // FIXME: Allow user to set the (defaulted) SAT iterations max for
   // `diagnoseFunction` with config options.
-  if (llvm::Expected<llvm::SmallVector<SourceLocation>> Locs =
-          dataflow::diagnoseFunction<UncheckedOptionalAccessModel,
-                                     SourceLocation>(*FuncDecl, *Result.Context,
-                                                     Diagnoser))
-    for (const SourceLocation &Loc : *Locs)
-      diag(Loc, "unchecked access to optional value");
+  if (llvm::Expected<llvm::SmallVector<UncheckedOptionalAccessDiagnostic>>
+          Diags = dataflow::diagnoseFunction<UncheckedOptionalAccessModel,
+                                             UncheckedOptionalAccessDiagnostic>(
+              *FuncDecl, *Result.Context, Diagnoser))
+    for (const UncheckedOptionalAccessDiagnostic &Diag : *Diags) {
+      diag(Diag.Range.getBegin(), "unchecked access to optional value")
+          << Diag.Range;
+    }
   else
-    llvm::consumeError(Locs.takeError());
+    llvm::consumeError(Diags.takeError());
 }
 
 } // namespace clang::tidy::bugprone

clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h

@@ -20,6 +20,7 @@
 #include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h"
 #include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
 #include "clang/Analysis/FlowSensitive/NoopLattice.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/SmallVector.h"
@@ -71,20 +72,26 @@ class UncheckedOptionalAccessModel
       TransferMatchSwitch;
 };
 
+/// Diagnostic information for an unchecked optional access.
+struct UncheckedOptionalAccessDiagnostic {
+  CharSourceRange Range;
+};
+
 class UncheckedOptionalAccessDiagnoser {
 public:
   UncheckedOptionalAccessDiagnoser(
       UncheckedOptionalAccessModelOptions Options = {});
 
-  llvm::SmallVector<SourceLocation>
+  llvm::SmallVector<UncheckedOptionalAccessDiagnostic>
   operator()(const CFGElement &Elt, ASTContext &Ctx,
              const TransferStateForDiagnostics<UncheckedOptionalAccessLattice>
                  &State) {
     return DiagnoseMatchSwitch(Elt, Ctx, State.Env);
   }
 
 private:
-  CFGMatchSwitch<const Environment, llvm::SmallVector<SourceLocation>>
+  CFGMatchSwitch<const Environment,
+                 llvm::SmallVector<UncheckedOptionalAccessDiagnostic>>
       DiagnoseMatchSwitch;
 };
 

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

@@ -1120,8 +1120,8 @@ auto buildTransferMatchSwitch() {
       .Build();
 }
 
-llvm::SmallVector<SourceLocation> diagnoseUnwrapCall(const Expr *ObjectExpr,
-                                                     const Environment &Env) {
+llvm::SmallVector<UncheckedOptionalAccessDiagnostic>
+diagnoseUnwrapCall(const Expr *ObjectExpr, const Environment &Env) {
   if (auto *OptionalLoc = cast_or_null<RecordStorageLocation>(
           getLocBehindPossiblePointer(*ObjectExpr, Env))) {
     auto *Prop = Env.getValue(locForHasValue(*OptionalLoc));
@@ -1132,9 +1132,9 @@ llvm::SmallVector<SourceLocation> diagnoseUnwrapCall(const Expr *ObjectExpr,
   }
 
   // Record that this unwrap is *not* provably safe.
-  // FIXME: include either the name of the optional (if applicable) or a source
-  // range of the access for easier interpretation of the result.
-  return {ObjectExpr->getBeginLoc()};
+  // FIXME: include the name of the optional (if applicable).
+  auto Range = CharSourceRange::getTokenRange(ObjectExpr->getSourceRange());
+  return {UncheckedOptionalAccessDiagnostic{Range}};
 }
 
 auto buildDiagnoseMatchSwitch(
@@ -1143,8 +1143,9 @@ auto buildDiagnoseMatchSwitch(
   // lot of duplicated work (e.g. string comparisons), consider providing APIs
   // that avoid it through memoization.
   auto IgnorableOptional = ignorableOptional(Options);
-  return CFGMatchSwitchBuilder<const Environment,
-                               llvm::SmallVector<SourceLocation>>()
+  return CFGMatchSwitchBuilder<
+             const Environment,
+             llvm::SmallVector<UncheckedOptionalAccessDiagnostic>>()
       // optional::value
       .CaseOfCFGStmt<CXXMemberCallExpr>(
           valueCall(IgnorableOptional),

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

@@ -14,6 +14,7 @@
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Frontend/TextDiagnostic.h"
 #include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Error.h"
@@ -1282,6 +1283,14 @@ static raw_ostream &operator<<(raw_ostream &OS,
 class UncheckedOptionalAccessTest
     : public ::testing::TestWithParam<OptionalTypeIdentifier> {
 protected:
+  // Check that after running the analysis on SourceCode, it produces the
+  // expected diagnostics according to [[unsafe]] annotations.
+  // - No annotations => no diagnostics.
+  // - Given "// [[unsafe]]" annotations on a line, we expect a diagnostic on
+  //   that line.
+  // - Given "// [[unsafe:range_text]]" annotations on a line, we expect a
+  //   diagnostic on that line, and we expect the diagnostic Range (printed as
+  //   a string) to match the "range_text".
   void ExpectDiagnosticsFor(std::string SourceCode,
                             bool IgnoreSmartPointerDereference = true) {
     ExpectDiagnosticsFor(SourceCode, ast_matchers::hasName("target"),
@@ -1336,7 +1345,7 @@ class UncheckedOptionalAccessTest
       T Make();
     )");
     UncheckedOptionalAccessModelOptions Options{IgnoreSmartPointerDereference};
-    std::vector<SourceLocation> Diagnostics;
+    std::vector<UncheckedOptionalAccessDiagnostic> Diagnostics;
     llvm::Error Error = checkDataflow<UncheckedOptionalAccessModel>(
         AnalysisInputs<UncheckedOptionalAccessModel>(
             SourceCode, std::move(FuncMatcher),
@@ -1364,22 +1373,34 @@ class UncheckedOptionalAccessTest
                                   &Annotations,
                               const AnalysisOutputs &AO) {
           llvm::DenseSet<unsigned> AnnotationLines;
-          for (const auto &[Line, _] : Annotations) {
+          llvm::DenseMap<unsigned, std::string> AnnotationRangesInLines;
+          for (const auto &[Line, AnnotationWithMaybeRange] : Annotations) {
             AnnotationLines.insert(Line);
+            auto it = AnnotationWithMaybeRange.find(':');
+            if (it != std::string::npos) {
+              AnnotationRangesInLines[Line] =
+                  AnnotationWithMaybeRange.substr(it + 1);
+            }
           }
           auto &SrcMgr = AO.ASTCtx.getSourceManager();
           llvm::DenseSet<unsigned> DiagnosticLines;
-          for (SourceLocation &Loc : Diagnostics) {
-            unsigned Line = SrcMgr.getPresumedLineNumber(Loc);
+          for (const UncheckedOptionalAccessDiagnostic &Diag : Diagnostics) {
+            unsigned Line = SrcMgr.getPresumedLineNumber(Diag.Range.getBegin());
             DiagnosticLines.insert(Line);
             if (!AnnotationLines.contains(Line)) {
               IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(
                   new DiagnosticOptions());
               TextDiagnostic TD(llvm::errs(), AO.ASTCtx.getLangOpts(),
                                 DiagOpts.get());
-              TD.emitDiagnostic(FullSourceLoc(Loc, SrcMgr),
+              TD.emitDiagnostic(FullSourceLoc(Diag.Range.getBegin(), SrcMgr),
                                 DiagnosticsEngine::Error,
-                                "unexpected diagnostic", {}, {});
+                                "unexpected diagnostic", {Diag.Range}, {});
+            } else {
+              auto it = AnnotationRangesInLines.find(Line);
+              if (it != AnnotationRangesInLines.end()) {
+                EXPECT_EQ(Diag.Range.getAsRange().printToString(SrcMgr),
+                          it->second);
+              }
             }
           }
 
@@ -4085,6 +4106,31 @@ TEST_P(UncheckedOptionalAccessTest, ConstPointerRefAccessor) {
                        /*IgnoreSmartPointerDereference=*/false);
 }
 
+TEST_P(UncheckedOptionalAccessTest, DiagnosticsHaveRanges) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      $ns::$optional<int> fi;
+    };
+    struct B {
+      $ns::$optional<A> fa;
+    };
+
+    void target($ns::$optional<B> opt) {
+      opt.value();  // [[unsafe:<input.cc:12:7>]]
+      if (opt) {
+        opt  // [[unsafe:<input.cc:14:9, line:16:13>]]
+          ->
+            fa.value();
+        if (opt->fa) {
+          opt->fa->fi.value();  // [[unsafe:<input.cc:18:11, col:20>]]
+        }
+      }
+    }
+  )cc");
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)