[clang][dataflow] For bugprone-unchecked-optional-access report range

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,8 @@ 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()};
+  auto Range = CharSourceRange::getTokenRange(ObjectExpr->getSourceRange());
+  return {UncheckedOptionalAccessDiagnostic{Range}};
 }
 
 auto buildDiagnoseMatchSwitch(
@@ -1143,8 +1142,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

@@ -1336,7 +1336,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),
@@ -1369,17 +1369,17 @@ class UncheckedOptionalAccessTest
           }
           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}, {});
             }
           }