[analyzer] Remove barely used class 'KnownSVal' (NFC)

[NagyDonat]

The class KnownSVal was very magical abstract class within the SVal class hierarchy: with a hacky classof method it acted as if it was the common ancestor of the classes UndefinedSVal and DefinedSVal.

However, it was only used in two getAs<KnownSVal>() calls and the signatures of two methods, which does not "pay for" its weird behavior, so I created this commit that removes it and replaces its use with more straightforward solutions.

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: None (NagyDonat)

Changes

The class KnownSVal was very magical abstract class within the SVal class hierarchy: with a hacky classof method it acted as if it was the common ancestor of the classes UndefinedSVal and DefinedSVal.

However, it was only used in two getAs<KnownSVal>() calls and the signatures of two methods, which does not "pay for" its weird behavior, so I created this commit that removes it and replaces its use with more straightforward solutions.

---

Full diff: https://github.com/llvm/llvm-project/pull/86953.diff

4 Files Affected:

• (modified) clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h (+2-1)
• (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h (-8)
• (modified) clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp (+1-1)
• (modified) clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (+7-7)

diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
index d9b3d9352d3224..cc3d93aabafda4 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -374,6 +374,7 @@ bool trackExpressionValue(const ExplodedNode *N, const Expr *E,
 /// from.
 ///
 /// \param V We're searching for the store where \c R received this value.
+///        It may be either defined or undefined, but should not be unknown.
 /// \param R The region we're tracking.
 /// \param Opts Tracking options specifying how we want to track the value.
 /// \param Origin Only adds notes when the last store happened in a
@@ -383,7 +384,7 @@ bool trackExpressionValue(const ExplodedNode *N, const Expr *E,
 ///        changes to its value in a nested stackframe could be pruned, and
 ///        this visitor can prevent that without polluting the bugpath too
 ///        much.
-void trackStoredValue(KnownSVal V, const MemRegion *R,
+void trackStoredValue(SVal V, const MemRegion *R,
                       PathSensitiveBugReport &Report, TrackingOptions Opts = {},
                       const StackFrameContext *Origin = nullptr);
 
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
index c60528b7685fe8..3a4b0872571494 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
@@ -232,14 +232,6 @@ class DefinedSVal : public DefinedOrUnknownSVal {
       : DefinedOrUnknownSVal(Kind, Data) {}
 };
 
-/// Represents an SVal that is guaranteed to not be UnknownVal.
-class KnownSVal : public SVal {
-public:
-  /*implicit*/ KnownSVal(DefinedSVal V) : SVal(V) {}
-  /*implicit*/ KnownSVal(UndefinedVal V) : SVal(V) {}
-  static bool classof(SVal V) { return !V.isUnknown(); }
-};
-
 class NonLoc : public DefinedSVal {
 protected:
   NonLoc(SValKind Kind, const void *Data) : DefinedSVal(Kind, Data) {}
diff --git a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
index c3acb73ba7175b..086c3e5e49b770 100644
--- a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -977,7 +977,7 @@ void RefLeakReport::findBindingToReport(CheckerContext &Ctx,
     //       something like derived regions if we want to construct SVal from
     //       Sym. Instead, we take the value that is definitely stored in that
     //       region, thus guaranteeing that trackStoredValue will work.
-    bugreporter::trackStoredValue(AllVarBindings[0].second.castAs<KnownSVal>(),
+    bugreporter::trackStoredValue(AllVarBindings[0].second,
                                   AllocBindingToReport, *this);
   } else {
     AllocBindingToReport = AllocFirstBinding;
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index a0822513a6d02e..ce167d979d5761 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1238,12 +1238,12 @@ class StoreSiteFinder final : public TrackingBugReporterVisitor {
   ///        changes to its value in a nested stackframe could be pruned, and
   ///        this visitor can prevent that without polluting the bugpath too
   ///        much.
-  StoreSiteFinder(bugreporter::TrackerRef ParentTracker, KnownSVal V,
+  StoreSiteFinder(bugreporter::TrackerRef ParentTracker, SVal V,
                   const MemRegion *R, TrackingOptions Options,
                   const StackFrameContext *OriginSFC = nullptr)
       : TrackingBugReporterVisitor(ParentTracker), R(R), V(V), Options(Options),
         OriginSFC(OriginSFC) {
-    assert(R);
+    assert(!V.isUnknown() && R);
   }
 
   void Profile(llvm::FoldingSetNodeID &ID) const override;
@@ -2539,9 +2539,9 @@ class DefaultExpressionHandler final : public ExpressionHandler {
         Report.addVisitor<UndefOrNullArgVisitor>(L->getRegion());
         Result.FoundSomethingToTrack = true;
 
-        if (auto KV = RVal.getAs<KnownSVal>())
+        if (!RVal.isUnknown())
           Result.combineWith(
-              getParentTracker().track(*KV, L->getRegion(), Opts, SFC));
+              getParentTracker().track(RVal, L->getRegion(), Opts, SFC));
       }
 
       const MemRegion *RegionRVal = RVal.getAsRegion();
@@ -2663,8 +2663,8 @@ Tracker::Result Tracker::track(const Expr *E, const ExplodedNode *N,
 
 Tracker::Result Tracker::track(SVal V, const MemRegion *R, TrackingOptions Opts,
                                const StackFrameContext *Origin) {
-  if (auto KV = V.getAs<KnownSVal>()) {
-    Report.addVisitor<StoreSiteFinder>(this, *KV, R, Opts, Origin);
+  if (!V.isUnknown()) {
+    Report.addVisitor<StoreSiteFinder>(this, V, R, Opts, Origin);
     return {true};
   }
   return {};
@@ -2692,7 +2692,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
       .FoundSomethingToTrack;
 }
 
-void bugreporter::trackStoredValue(KnownSVal V, const MemRegion *R,
+void bugreporter::trackStoredValue(SVal V, const MemRegion *R,
                                    PathSensitiveBugReport &Report,
                                    TrackingOptions Opts,
                                    const StackFrameContext *Origin) {

[steakhal]

The simpler the better.
Thanks.

[steakhal]

Split these conditions to different assertions.

[NagyDonat]

Actually, I realized that it's better to avoid asserting that V is not unknown. This is a precondition that happens to be satisfied because this class is constructed in one single location and that is preceded by a !V.isUnknown() check -- but I'm fairly certain that the actual implementation of StoreSiteFinder can find a site where UnknownVal was stored in a region. Adding an assert like this could be confusing for the readers who would naturally assume that UnknownVal would cause problems in the logic of this handler.

[haoNoQ]

Yeah nuke it.