[clang] [analyzer] Restore commutativity of pointer/null comparison for addresses of fields of symbolic regions (PR #209875)

[Balázs Benics via cfe-commits]

================
@@ -0,0 +1,43 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+// Regression test for #206798.
+//
+// &r->s is a field of a symbolic region, so it's symbol-based. A Yoda null
+// check (`nullptr == p`) has to build the same constraint as the normal
+// `p == nullptr`. If it doesn't, the analyzer disagrees with itself and
+// reports a null deref that can't actually happen.
+
+struct S {
+  int n;
+};
+struct T {
+  S s;
+};
+
+int viaFieldAddress(S *p) {
+  if (!p) {
+  }
+  if (nullptr == p) // Yoda-style null check
+    return 0;
+  return p->n; // no-warning: p can't be null here
+}
+
+void callViaFieldAddress(T *r) { viaFieldAddress(&r->s); }
+
+// The normal `p == nullptr` order should still work too.
+int viaFieldAddressCanonical(S *p) {
+  if (!p) {
+  }
+  if (p == nullptr)
+    return 0;
+  return p->n; // no-warning
+}
+
+void callViaFieldAddressCanonical(T *r) { viaFieldAddressCanonical(&r->s); }
+
+// A real null deref should still be flagged - the fix shouldn't hide bugs.
+int stillReportsRealBug(S *p) {
+  if (nullptr == p)
+    return p->n; // expected-warning{{Access to field 'n' results in a dereference of a null pointer (loaded from variable 'p')}}
+  return 0;
+}
----------------
steakhal wrote:

Can we also have this for the swapped variant?
The comment is also not too useful here either. The expectation already tells you this.

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