[clang] [analyzer] `SValBuilder::evalBinOpLN`: try simplifying the RHS first (PR #161537)

[via cfe-commits]

================
@@ -1111,6 +1111,12 @@ SVal SimpleSValBuilder::evalBinOpLN(ProgramStateRef state,
   assert(!BinaryOperator::isComparisonOp(op) &&
          "arguments to comparison ops must be of the same type");
 
+  // Constraints may have changed since the creation of a bound SVal. Check if
+  // the value can be simplified based on those new constraints.
+  SVal simplifiedRhs = simplifySVal(state, rhs);
+  if (auto simplifiedRhsAsNonLoc = simplifiedRhs.getAs<NonLoc>())
+    rhs = *simplifiedRhsAsNonLoc;
+
----------------
guillem-bartrina-sonarsource wrote:

> The comment suggests some spooky action in the distance that "may have changed" the constraints. I think without explaining how and where that could happen this sentence only brings confusion. If that would be the case, I'd rather just have this code without a comment.

I agree that this comment is not very clear; I copied it verbatim from `evalBinOpNN` ([here](https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp#L430-L437)). At the same time, I don't think this is the best place to explain how and when the constraints may have changed, so I prefer to remove the comment.

> the outermost evalBinOp should simplify and then dispatch to the suffixed implementation variants and also potentially recurse. This structure should give us guarantees that every symbol simplified exactly once (except if we end up recursing of course).

Yes, that would be ideal, but as you say, that's not what the current implementation does. And, in fact, it doesn't help that the checkers directly use the `evalBinOpXX` operations (perhaps they shouldn't be exposed?). For now, this change will make the engine more complete.

https://github.com/llvm/llvm-project/pull/161537