[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

> There are already two way more sophisticated (forgive me my bias) 
> implementations in Clang that are for checking if two statements or decls are 
> the same.
>
> 1. ODRHash, used in modules to discover ODR violations
> 2. ASTStructuralEquivalenceContext, used in ASTImporter to discover if two 
> AST nodes are the same or not (as a side effect we diagnose ODR violations as 
> well).
>
> It is not the first time, when such a similarity check is needed (see 
> https://reviews.llvm.org/D75041). Of course reusing the before mentioned 
> components would require some architectural changes, but it might be 
> beneficial.

I do not quite see the overlap.  This patch addresses the structural 
equivalence of DeclRefExprs: as the `std::is_same` (or any type trait example) 
demonstrates, two declarations may be the "same" (e.g. they are both 
`std::false_type::value`), but two DeclRefExprs referring to those declarations 
should not necessarily be considered the "same": the qualifier, specifying the 
path that was taken to look them up, can matter to a user.  It's not a matter 
of the sophistication of the similarity check, it's a matter of what we mean by 
similarity.

I do not see DeclRefExprs handled in ODRHash or ASTStructuralEquivalence.  I do 
see NestedNameSpecifiers handled in both, but I don't think the implementation 
quite matches what is needed here (e.g. in ASTStructuralEquivalence check, if 
one NNS is a NamespaceAlias, the other one is assumed to be a NamespaceAlias: 
not what we want).  It's probably not worth the trouble to factor something 
common out of those; though they should certainly be used as a guide to make 
sure no cases have been missed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D114622#3200678 , @NoQ wrote:

> These checks are almost 2000 lines of code each and it looks like all they do 
> is figure out if two statements are the same and we have a very flexible 
> reusable solution for this sort of stuff - `CloneDetector`, but none of them 
> use it. Your patch demonstrates that they all have the same bugs that need to 
> be fixed in each of them separately, so reusability seems really valuable. If 
> I was to work on these checks, the first thing I'd try would be to throw out 
> their machines and plug in a reusable solution.

There are already two way more sophisticated (forgive me my bias) 
implementations in Clang that are for checking if two statements or decls are 
the same.

1. ODRHash, used in modules to discover ODR violations
2. ASTStructuralEquivalenceContext, used in ASTImporter to discover if two AST 
nodes are the same or not (as a side effect we diagnose ODR violations as well).

It is not the first time, when such a similarity check is needed (see 
https://reviews.llvm.org/D75041). Of course reusing the before mentioned 
components would require some architectural changes, but it might be beneficial.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp:372
+}
+
 /// Determines whether two statement trees are identical regarding

davrec wrote:
> I would strongly support turning these functions into methods of DeclRefExpr 
> and NestedNameSpecifier, not just to avoid the code duplication here but 
> because they sure seem to be of general utility to AST users.
> 
> You just need to clearly motivate these methods in the documentation, which 
> your description for this patch largely does, e.g. something like.:
> 
> ```
> class DeclRefExpr : ... {
>   ...
>   /// Returns whether this \c DeclRefExpr 
>   ///   a. refers to the same canonical declaration as \c other and
>   ///   b. if it has a qualifier, that qualifier refers to the same canonical 
>   ///   declarations as those of \c other .
>   ///
>   /// Consider these two DeclRefExprs:
>   /// \code
>   ///   std::is_same::value
>   ///   std::is_same::value;
>   /// \endcode
>   ///
>   /// We can see that the value static members are different, but in fact
>   /// the \c getDecl() of these two returns the same VarDecl. (...etc)
>   bool isSemanticallyEquivalentTo(const DeclRefExpr *Other) { ... }
> };
> ...
> class NestedNameSpecifier {
>   ...
>   /// Returns whether this refers to the same sequence of 
> identifiers/canonical declarations
>   /// as \c Other.  (Then I would repeat the std::is_same example here, since 
> that
>   /// really makes the issue clear.  And also describe what this returns when 
> other is nullptr or
>   /// when getPrefix() is nullptr while other->getPrefix() is non-null -- 
> maybe add a parameter if
>   /// its not clear in general what the behavior should be.)
>   bool isSemanticallyEquivalentTo(const NestedNameSpecifier *Other, bool 
> RecurseToPrefix = true) { ... }
> };
> 
> ```
Also if doing this it might be nice to add `isSyntacticallyEquivalentTo(other)` 
methods alongside the semantic versions, defined the same except they don't 
desugar the type nor get the underlying NamespaceDecl from a 
NamespaceAliasDecl.  This would be simple and, given that you found that two 
NestedNameSpecifiers representing the exact same syntax are nonetheless 
different pointers, it could be very useful for AST users concerned with syntax 
rather than semantics.

(Btw note the DeclRefExpr's syntactic method should still use 
getDecl()->getCanonicalDecl(), since, confusingly, getCanonicalDecl simply 
fetches the FirstDecl from the various redeclarations, rather than doing any 
kind of desugaring as getCanonicalType does.  And fortunately I don't think it 
is necessary to manually "desugar" the canonical decl in the semantic version, 
since a DeclRefExpr's getDecl is always a ValueDecl, which I don't think can 
ever be "sugar" for some underlying decl, as say a NamespaceAliasDecl is for a 
NamespaceDecl -- so that part should be fine.)



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[David Rector via Phabricator via cfe-commits]

davrec added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:59
+   RTy->getCanonicalTypeUnqualified();
+
+  // FIXME: We should probably check the other kinds of nested name specifiers.

steakhal wrote:
> davrec wrote:
> > It occurs to me this needs to be recursive, to check a sequence of 
> > qualifiers, i.e.
> > ```
> >   const Type *LTy = Left->getAsType();
> >   const Type *RTy = Right->getAsType();
> >   if (!LTy || !RTy) 
> > return true;
> >   if (LTy->getCanonicalTypeUnqualified() !=
> >RTy->getCanonicalTypeUnqualified())
> > return false;
> >   return areEquivalentNameSpecifier(Left->getPrefix(), Right->getPrefix());
> > ```
> > 
> > The reason is, if there is a prefix qualifier to this qualifier, we run 
> > into the original problem, e.g.:
> > ```
> > struct Base {
> >   struct Inner { 
> > static const bool value = true;
> >   };
> > };
> > struct Outer1 : Base {};
> > struct Outer2 : Base {};
> > 
> > // We do not want the following to warn, but without checking the prefix of 
> > Inner, 
> > // I believe these will be interpreted as equivalent DeclRefs and will warn:
> > auto sink = Outer1::Inner::value || Outer2::Inner::value;
> > ```
> > 
> You are right, but that would require me to implement all the possible kinds 
> of NNS, in addition to the types.
> And it's not entirely clear to me (yet) what the desired behavior should be 
> for implementing this.
I agree it's not entirely clear what should be the desired behavior in that 
rare case; I suppose it might be appropriate to introduce a `bool 
RecurseToPrefix` param that determines whether you do the while loop or finish 
after one iteration, mostly so that others using this function can ponder the 
decision for themselves.



Comment at: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp:372
+}
+
 /// Determines whether two statement trees are identical regarding

I would strongly support turning these functions into methods of DeclRefExpr 
and NestedNameSpecifier, not just to avoid the code duplication here but 
because they sure seem to be of general utility to AST users.

You just need to clearly motivate these methods in the documentation, which 
your description for this patch largely does, e.g. something like.:

```
class DeclRefExpr : ... {
  ...
  /// Returns whether this \c DeclRefExpr 
  ///   a. refers to the same canonical declaration as \c other and
  ///   b. if it has a qualifier, that qualifier refers to the same canonical 
  ///   declarations as those of \c other .
  ///
  /// Consider these two DeclRefExprs:
  /// \code
  ///   std::is_same::value
  ///   std::is_same::value;
  /// \endcode
  ///
  /// We can see that the value static members are different, but in fact
  /// the \c getDecl() of these two returns the same VarDecl. (...etc)
  bool isSemanticallyEquivalentTo(const DeclRefExpr *Other) { ... }
};
...
class NestedNameSpecifier {
  ...
  /// Returns whether this refers to the same sequence of identifiers/canonical 
declarations
  /// as \c Other.  (Then I would repeat the std::is_same example here, since 
that
  /// really makes the issue clear.  And also describe what this returns when 
other is nullptr or
  /// when getPrefix() is nullptr while other->getPrefix() is non-null -- maybe 
add a parameter if
  /// its not clear in general what the behavior should be.)
  bool isSemanticallyEquivalentTo(const NestedNameSpecifier *Other, bool 
RecurseToPrefix = true) { ... }
};

```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Balázs Benics via Phabricator via cfe-commits]

steakhal marked an inline comment as done.
steakhal added a comment.

In D114622#3200678 , @NoQ wrote:

> These checks are almost 2000 lines of code each and it looks like all they do 
> is figure out if two statements are the same and we have a very flexible 
> reusable solution for this sort of stuff - `CloneDetector`, but none of them 
> use it. Your patch demonstrates that they all have the same bugs that need to 
> be fixed in each of them separately, so reusability seems really valuable. If 
> I was to work on these checks, the first thing I'd try would be to throw out 
> their machines and plug in a reusable solution.

Well, yes. Ideally, we should remove probably the ClangSA implementation, since 
it does basically the same thing. Right now I'm focusing on fixing this FP, and 
I don't really want to tip my toe into anything bigger than that.

About the `CloneDetector`, well in February this year we observed that it 
basically halted an analysis, due to some unfortunate situation. I saw 
`areSequencesClones()` stringifying QualTypes for hours if not days.
We had other priorities to focus, thus we could not get there reproducing and 
fixing the issue with that, but that could be a potential reason why people 
don't want to use that. We should really dig into that at some point.
For the record, at the time it was analyzing llvm itself, the 
`AMDGPUAsmParser.cpp` file to be precise.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

P.S. `bugprone-branch-clone` seems to have attempted to use `CloneDetector` in 
the past but now it's no more than a dead #include. I wonder what happened 
there.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

These checks are almost 2000 lines of code each and it looks like all they do 
is figure out if two statements are the same and we have a very flexible 
reusable solution for this sort of stuff - `CloneDetector`, but none of them 
use it. Your patch demonstrates that they all have the same bugs that need to 
be fixed in each of them separately, so reusability seems really valuable. If I 
was to work on these checks, the first thing I'd try would be to throw out 
their machines and plug in a reusable solution.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Balázs Benics via Phabricator via cfe-commits]

steakhal updated this revision to Diff 395150.
steakhal added a comment.

Sorry for the late update, but here we are:

Now, I properly handle all kinds of `NestedNameSpecifiers`.
Basically, I verify if the decls are matching, then I use the nested name 
specifiers for finding mismatches. I look through namespace aliases and I use 
the canonical types.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

Files:
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
  clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
  clang/test/Analysis/identical-expressions.cpp

Index: clang/test/Analysis/identical-expressions.cpp
===
--- clang/test/Analysis/identical-expressions.cpp
+++ clang/test/Analysis/identical-expressions.cpp
@@ -1562,3 +1562,62 @@
   ;
   }
 }
+
+template  struct boolean_value {
+  static constexpr bool value = V;
+};
+template  struct my_trait : boolean_value {};
+
+bool respect_nested_name_specifiers(bool sink) {
+  sink |= my_trait::value || my_trait::value; // no-warning
+
+  sink |= my_trait::value || my_trait::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  using my_char = char;
+  sink |= my_trait::value || my_trait::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  sink |= my_trait::value || ::my_trait::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  using my_trait_alias = my_trait;
+  sink |= my_trait::value || my_trait_alias::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  return sink;
+}
+
+static void static_function() {}
+namespace my {
+int fn(int = 1);
+}
+void respect_namespaces(bool coin) {
+  namespace other = my;
+  namespace other2 = other;
+  using namespace other;
+
+  coin ? my::fn(1) : my::fn(2);// no-warning
+  coin ? my::fn(1) : other::fn(2); // no-warning
+
+  // expected-warning@+5 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+5 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+5 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+5 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+5 {{identical expressions on both sides of ':' in conditional expression}}
+  coin ? my::fn(1) : my::fn(1);
+  coin ? my::fn(1) : other::fn(1);
+  coin ? my::fn(1) : fn(1);
+  coin ? my::fn(1) : other2::fn(1);
+  coin ? fn(1) : other2::fn(1);
+
+  coin ? my::fn(1) : fn(); // FIXME: We should have a warning for this: the default parameter is also 1.
+
+  my::fn(1) & my::fn(1);// no-warning
+  my::fn(1) & other::fn(1); // no-warning
+
+  // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+  coin ? ::static_function() : ::static_function();
+  coin ? ::static_function() : static_function();
+}
Index: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
@@ -295,6 +295,81 @@
   return true;
 }
 
+static const NamespaceDecl *lookingThroughAliases(const NamedDecl *D) {
+  while (const auto *Alias = dyn_cast(D))
+D = Alias->getAliasedNamespace();
+  return cast_or_null(D);
+}
+
+static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
+   const NestedNameSpecifier *Right) {
+  const auto TryCompareAsNamespaces =
+  [](const NestedNameSpecifier *Left,
+ const NestedNameSpecifier *Right) -> Optional {
+const NamespaceDecl *LeftAsNS = Left->getAsNamespace();
+const NamespaceDecl *RightAsNS = Right->getAsNamespace();
+if (const auto *LeftAlias = Left->getAsNamespaceAlias())
+  LeftAsNS = lookingThroughAliases(LeftAlias);
+if (const auto *RightAlias = Left->getAsNamespaceAlias())
+  RightAsNS = lookingThroughAliases(RightAlias);
+
+if (!LeftAsNS || !RightAsNS)
+  return None;
+return LeftAsNS == RightAsNS;
+  };
+
+  const auto TryCompareAsTypes =
+  [](const NestedNameSpecifier *Left,
+ const NestedNameSpecifier *Right) -> Optional {
+const Type *LeftAsTy = Left->getAsType();
+const Type *RightAsTy = Right->getAsType();
+
+if (!LeftAsTy || !RightAsTy)
+  return None;
+
+LeftAsTy = LeftAsTy->getCanonicalTypeUnqualified().getTypePtr();
+RightAsTy = 

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Sorry for my late response, I'm busy with other things right now. I plan to 
come back to this in a couple of weeks.




Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:52
 
 static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
const NestedNameSpecifier *Right) {

davrec wrote:
> This function should probably either be made into a lambda private to 
> `areEquivalentDeclRefs()`, or renamed to make clear it does not apply 
> generically to all NNS's.  
> 
> Part of the reason is that this is only implemented for type NNS's.  But the 
> more important reason is that, when either a) `!Left || !Right`, or b) 
> `!Left->getAsType() || !Right->getAsType()`, this returns true, since 
> (presumably*) this gives us the desired behavior within 
> areEquivalentDeclRefs(), despite that in general a null NNS should probably 
> not be considered the same as a nonnull NNS.  
> 
> (*Is this indeed the desired behavior? Probably should add some tests that 
> check qualified DeclRefExprs against unqualified DeclRefExprs, to be sure.)
> This function should probably either be made into a lambda private to 
> areEquivalentDeclRefs(), or renamed to make clear it does not apply 
> generically to all NNS's.
> Part of the reason is that this is only implemented for type NNS's. But the 
> more important reason is that, when either a) !Left || !Right, or b) 
> !Left->getAsType() || !Right->getAsType(), this returns true, since 
> (presumably*) this gives us the desired behavior within 
> areEquivalentDeclRefs(), despite that in general a null NNS should probably 
> not be considered the same as a nonnull NNS.
I agree. I just wanted to preserve as much code as I could and aim for a 
minimal change to implement my intention.
I'll update this accordingly.

> (*Is this indeed the desired behavior? Probably should add some tests that 
> check qualified DeclRefExprs against unqualified DeclRefExprs, to be sure.)
You are right, not perfect. But it's already closer than it previously was. 
I'll add the test anyway.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:59
+   RTy->getCanonicalTypeUnqualified();
+
+  // FIXME: We should probably check the other kinds of nested name specifiers.

davrec wrote:
> It occurs to me this needs to be recursive, to check a sequence of 
> qualifiers, i.e.
> ```
>   const Type *LTy = Left->getAsType();
>   const Type *RTy = Right->getAsType();
>   if (!LTy || !RTy) 
> return true;
>   if (LTy->getCanonicalTypeUnqualified() !=
>RTy->getCanonicalTypeUnqualified())
> return false;
>   return areEquivalentNameSpecifier(Left->getPrefix(), Right->getPrefix());
> ```
> 
> The reason is, if there is a prefix qualifier to this qualifier, we run into 
> the original problem, e.g.:
> ```
> struct Base {
>   struct Inner { 
> static const bool value = true;
>   };
> };
> struct Outer1 : Base {};
> struct Outer2 : Base {};
> 
> // We do not want the following to warn, but without checking the prefix of 
> Inner, 
> // I believe these will be interpreted as equivalent DeclRefs and will warn:
> auto sink = Outer1::Inner::value || Outer2::Inner::value;
> ```
> 
You are right, but that would require me to implement all the possible kinds of 
NNS, in addition to the types.
And it's not entirely clear to me (yet) what the desired behavior should be for 
implementing this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

A couple thoughts/cases to consider...




Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:52
 
 static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
const NestedNameSpecifier *Right) {

This function should probably either be made into a lambda private to 
`areEquivalentDeclRefs()`, or renamed to make clear it does not apply 
generically to all NNS's.  

Part of the reason is that this is only implemented for type NNS's.  But the 
more important reason is that, when either a) `!Left || !Right`, or b) 
`!Left->getAsType() || !Right->getAsType()`, this returns true, since 
(presumably*) this gives us the desired behavior within 
areEquivalentDeclRefs(), despite that in general a null NNS should probably not 
be considered the same as a nonnull NNS.  

(*Is this indeed the desired behavior? Probably should add some tests that 
check qualified DeclRefExprs against unqualified DeclRefExprs, to be sure.)



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:53
 static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
const NestedNameSpecifier *Right) {
+  const Type *LTy = Left->getAsType();

Suggest you move the null check from `areEquivalentDeclRefs()` here, i.e.
```
if (!Left || !Right)
  return true;
```
mainly since this is needs to be done in recursive calls (see below), but also 
since you do the same logic on LTy and RTy subsequently.





Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:59
+   RTy->getCanonicalTypeUnqualified();
+
+  // FIXME: We should probably check the other kinds of nested name specifiers.

It occurs to me this needs to be recursive, to check a sequence of qualifiers, 
i.e.
```
  const Type *LTy = Left->getAsType();
  const Type *RTy = Right->getAsType();
  if (!LTy || !RTy) 
return true;
  if (LTy->getCanonicalTypeUnqualified() !=
   RTy->getCanonicalTypeUnqualified())
return false;
  return areEquivalentNameSpecifier(Left->getPrefix(), Right->getPrefix());
```

The reason is, if there is a prefix qualifier to this qualifier, we run into 
the original problem, e.g.:
```
struct Base {
  struct Inner { 
static const bool value = true;
  };
};
struct Outer1 : Base {};
struct Outer2 : Base {};

// We do not want the following to warn, but without checking the prefix of 
Inner, 
// I believe these will be interpreted as equivalent DeclRefs and will warn:
auto sink = Outer1::Inner::value || Outer2::Inner::value;
```




Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:72
+  // The decls were already matched, so just return true at any later point.
+  if (!Left->hasQualifier() || !Right->hasQualifier())
+return true;

Suggest you move this null check into areEquivalentNameSpecifier, see above


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:54-55
const NestedNameSpecifier *Right) {
-  llvm::FoldingSetNodeID LeftID, RightID;
-  Left->Profile(LeftID);
-  Right->Profile(RightID);
-  return LeftID == RightID;
+  if (const auto *L = Left->getAsType())
+if (const auto *R = Right->getAsType())
+  return L->getCanonicalTypeUnqualified() ==

whisperity wrote:
> whisperity wrote:
> > (Wrt. to the comment about not typing stuff out directly, I guess `L` and 
> > `R` here will be `X` and `X` and thus not the same.)
> (Style nit to lessen the indent depth, until C++17...)
> (Wrt. to the comment about not typing stuff out directly, I guess L and R 
> here will be X and X and thus not the same.)
I don't quite get what do you mean by this.



Comment at: 
clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:132-134
+const DeclRefExpr *DeclRef1 = cast(Left);
+const DeclRefExpr *DeclRef2 = cast(Right);
+return areEquivalentDeclRefs(DeclRef1, DeclRef2);

whisperity wrote:
> (Style nit. Or `LeftDR`, `RightDR`.)
`DeclRef1` -> `LeftDR`



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp:822
+
+  using my_char = char;
+  sink |= my_trait::value || my_trait::value;

whisperity wrote:
> Are there any more ways we could trick this check to cause false positives? 
> Something through which it doesn't look the same but still refers to the same 
> thing?
> 
> At the top of my head I'm thinking about something along the lines of this:
> 
> ```lang=cpp
> template 
> struct X {
>   template 
>   static constexpr bool same = std::is_same_v;
> };
> 
> X ch() { return X{}; }
> X i() { return X{}; }
> 
> void test() {
>   coin ? ch().same : i().same;
> }
> ```
> 
> So the "type name" where we want to look up isn't typed out directly.
> Or would these still be caught (and thus fixed) by looking at a 
> `NestedNameSpecifier`?
For the example:
```lang=C++
template  struct integral_constant {
static constexpr T value = V;
typedef T value_type;
typedef integral_constant type;
};
typedef integral_constant true_type;
typedef integral_constant false_type;
template  struct is_same : false_type {};
template  struct is_same : true_type {};

template 
struct X {
  template 
  static constexpr bool same = is_same::value;
};

X ch();
X i();

void test(bool coin) {
  coin ? ch().same : i().same;
}
```
We would have no warnings for this code, nor we had previously:
https://godbolt.org/z/9xo4adfbP

Should I demonstrate this in the test file?
I wouldn't pollute it TBH, since it requires some template machinery which is 
quite verbose.
However, if you insist I will add it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Balázs Benics via Phabricator via cfe-commits]

steakhal updated this revision to Diff 390032.
steakhal marked 3 inline comments as done.
steakhal added a comment.

Fixing nits: renaming variables, etc.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

Files:
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
  clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
  clang/test/Analysis/identical-expressions.cpp

Index: clang/test/Analysis/identical-expressions.cpp
===
--- clang/test/Analysis/identical-expressions.cpp
+++ clang/test/Analysis/identical-expressions.cpp
@@ -1562,3 +1562,50 @@
   ;
   }
 }
+
+template  struct boolean_value {
+  static constexpr bool value = V;
+};
+template  struct my_trait : boolean_value {};
+
+bool respect_nested_name_specifiers(bool sink) {
+  sink |= my_trait::value || my_trait::value; // no-warning
+
+  sink |= my_trait::value || my_trait::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  using my_char = char;
+  sink |= my_trait::value || my_trait::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  return sink;
+}
+
+static void static_function() {}
+namespace my {
+int fn(int = 1);
+}
+void respect_namespaces(bool coin) {
+  namespace other = my;
+  using namespace other;
+
+  coin ? my::fn(1) : my::fn(2);// no-warning
+  coin ? my::fn(1) : other::fn(2); // no-warning
+
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  coin ? my::fn(1) : my::fn(1);
+  coin ? my::fn(1) : other::fn(1);
+  coin ? my::fn(1) : fn(1);
+
+  coin ? my::fn(1) : fn(); // FIXME: We should have a warning for this: the default parameter is also 1.
+
+  my::fn(1) & my::fn(1);// no-warning
+  my::fn(1) & other::fn(1); // no-warning
+
+  // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+  coin ? ::static_function() : ::static_function();
+  coin ? ::static_function() : static_function();
+}
Index: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
@@ -295,6 +295,32 @@
   return true;
 }
 
+static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
+   const NestedNameSpecifier *Right) {
+  const Type *LTy = Left->getAsType();
+  const Type *RTy = Right->getAsType();
+  if (LTy && RTy)
+return LTy->getCanonicalTypeUnqualified() ==
+   RTy->getCanonicalTypeUnqualified();
+
+  // FIXME: We should probably check the other kinds of nested name specifiers.
+  return true;
+}
+
+static bool areEquivalentDeclRefs(const DeclRefExpr *Left,
+  const DeclRefExpr *Right) {
+  if (Left->getDecl()->getCanonicalDecl() !=
+  Right->getDecl()->getCanonicalDecl()) {
+return false;
+  }
+
+  // The decls were already matched, so just return true at any later point.
+  if (!Left->hasQualifier() || !Right->hasQualifier())
+return true;
+  return areEquivalentNameSpecifier(Left->getQualifier(),
+Right->getQualifier());
+}
+
 /// Determines whether two statement trees are identical regarding
 /// operators and symbols.
 ///
@@ -458,11 +484,9 @@
 const CharacterLiteral *CharLit2 = cast(Stmt2);
 return CharLit1->getValue() == CharLit2->getValue();
   }
-  case Stmt::DeclRefExprClass: {
-const DeclRefExpr *DeclRef1 = cast(Stmt1);
-const DeclRefExpr *DeclRef2 = cast(Stmt2);
-return DeclRef1->getDecl() == DeclRef2->getDecl();
-  }
+  case Stmt::DeclRefExprClass:
+return areEquivalentDeclRefs(cast(Stmt1),
+ cast(Stmt2));
   case Stmt::IntegerLiteralClass: {
 const IntegerLiteral *IntLit1 = cast(Stmt1);
 const IntegerLiteral *IntLit2 = cast(Stmt2);
Index: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
@@ -807,3 +807,52 @@
 };
 
 } // namespace no_crash
+
+template  struct boolean_value {
+  static constexpr bool value = V;
+};
+template  struct my_trait : 

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Balázs Benics via Phabricator via cfe-commits]

steakhal updated this revision to Diff 390023.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

Files:
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
  clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
  clang/test/Analysis/identical-expressions.cpp

Index: clang/test/Analysis/identical-expressions.cpp
===
--- clang/test/Analysis/identical-expressions.cpp
+++ clang/test/Analysis/identical-expressions.cpp
@@ -1562,3 +1562,50 @@
   ;
   }
 }
+
+template  struct boolean_value {
+  static constexpr bool value = V;
+};
+template  struct my_trait : boolean_value {};
+
+bool respect_nested_name_specifiers(bool sink) {
+  sink |= my_trait::value || my_trait::value; // no-warning
+
+  sink |= my_trait::value || my_trait::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  using my_char = char;
+  sink |= my_trait::value || my_trait::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  return sink;
+}
+
+static void static_function() {}
+namespace my {
+int fn(int = 1);
+}
+void respect_namespaces(bool coin) {
+  namespace other = my;
+  using namespace other;
+
+  coin ? my::fn(1) : my::fn(2);// no-warning
+  coin ? my::fn(1) : other::fn(2); // no-warning
+
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  coin ? my::fn(1) : my::fn(1);
+  coin ? my::fn(1) : other::fn(1);
+  coin ? my::fn(1) : fn(1);
+
+  coin ? my::fn(1) : fn(); // FIXME: We should have a warning for this: the default parameter is also 1.
+
+  my::fn(1) & my::fn(1);// no-warning
+  my::fn(1) & other::fn(1); // no-warning
+
+  // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+  coin ? ::static_function() : ::static_function();
+  coin ? ::static_function() : static_function();
+}
Index: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
@@ -295,6 +295,31 @@
   return true;
 }
 
+static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
+   const NestedNameSpecifier *Right) {
+  if (const auto *L = Left->getAsType())
+if (const auto *R = Right->getAsType())
+  return L->getCanonicalTypeUnqualified() ==
+ R->getCanonicalTypeUnqualified();
+
+  // FIXME: We should probably check the other kinds of nested name specifiers.
+  return true;
+}
+
+static bool areEquivalentDeclRefs(const DeclRefExpr *Left,
+  const DeclRefExpr *Right) {
+  if (Left->getDecl()->getCanonicalDecl() !=
+  Right->getDecl()->getCanonicalDecl()) {
+return false;
+  }
+
+  // The decls were already matched, so just return true at any later point.
+  if (!Left->hasQualifier() || !Right->hasQualifier())
+return true;
+  return areEquivalentNameSpecifier(Left->getQualifier(),
+Right->getQualifier());
+}
+
 /// Determines whether two statement trees are identical regarding
 /// operators and symbols.
 ///
@@ -461,7 +486,7 @@
   case Stmt::DeclRefExprClass: {
 const DeclRefExpr *DeclRef1 = cast(Stmt1);
 const DeclRefExpr *DeclRef2 = cast(Stmt2);
-return DeclRef1->getDecl() == DeclRef2->getDecl();
+return areEquivalentDeclRefs(DeclRef1, DeclRef2);
   }
   case Stmt::IntegerLiteralClass: {
 const IntegerLiteral *IntLit1 = cast(Stmt1);
Index: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
@@ -807,3 +807,52 @@
 };
 
 } // namespace no_crash
+
+template  struct boolean_value {
+  static constexpr bool value = V;
+};
+template  struct my_trait : boolean_value {};
+
+bool respect_nested_name_specifiers(bool sink) {
+  sink |= my_trait::value || my_trait::value; // no-warning
+
+  sink |= my_trait::value || my_trait::value;
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: both sides of operator are equivalent
+
+  using my_char = char;
+  sink |= my_trait::value || my_trait::value;
+  // CHECK-MESSAGES: 

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D114622#3155605 , @whisperity 
wrote:

> I haven't looked at the patch in detail yet, but I know I've run into the 
> issue this aims to fix when developing a Tidy check!
>
> There is a `// NOLINTNEXTLINE(misc-redundant-expression)` directive in the 
> source file 
> `clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp` 
> right before a `static_assert` that checks some `type_traits`-y stuff. Should 
> be at line **510** -- according to my local checkout. Could you please remove 
> that line and have that change be part of this patch too? I did a grep on the 
> project and that's the only "user-side" mention to the misc-redundant-... 
> check. Running Tidy on Tidy itself afterwards would turn out to be a nice 
> real-world test that the fix indeed works. 

Yup, it works. I removed your `NOLINT` comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:54-55
const NestedNameSpecifier *Right) {
-  llvm::FoldingSetNodeID LeftID, RightID;
-  Left->Profile(LeftID);
-  Right->Profile(RightID);
-  return LeftID == RightID;
+  if (const auto *L = Left->getAsType())
+if (const auto *R = Right->getAsType())
+  return L->getCanonicalTypeUnqualified() ==

(Wrt. to the comment about not typing stuff out directly, I guess `L` and `R` 
here will be `X` and `X` and thus not the same.)



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:54-57
+  if (const auto *L = Left->getAsType())
+if (const auto *R = Right->getAsType())
+  return L->getCanonicalTypeUnqualified() ==
+ R->getCanonicalTypeUnqualified();

whisperity wrote:
> (Wrt. to the comment about not typing stuff out directly, I guess `L` and `R` 
> here will be `X` and `X` and thus not the same.)
(Style nit to lessen the indent depth, until C++17...)



Comment at: 
clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:132-134
+const DeclRefExpr *DeclRef1 = cast(Left);
+const DeclRefExpr *DeclRef2 = cast(Right);
+return areEquivalentDeclRefs(DeclRef1, DeclRef2);

(Style nit. Or `LeftDR`, `RightDR`.)



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp:822
+
+  using my_char = char;
+  sink |= my_trait::value || my_trait::value;

Are there any more ways we could trick this check to cause false positives? 
Something through which it doesn't look the same but still refers to the same 
thing?

At the top of my head I'm thinking about something along the lines of this:

```lang=cpp
template 
struct X {
  template 
  static constexpr bool same = std::is_same_v;
};

X ch() { return X{}; }
X i() { return X{}; }

void test() {
  coin ? ch().same : i().same;
}
```

So the "type name" where we want to look up isn't typed out directly.
Or would these still be caught (and thus fixed) by looking at a 
`NestedNameSpecifier`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

I haven't looked at the patch in detail yet, but I know I've run into the issue 
this aims to fix when developing a Tidy check!

There is a `// NOLINTNEXTLINE(misc-redundant-expression)` directive in the 
source file 
`clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp` 
right before a `static_assert` that checks some `type_traits`-y stuff. Should 
be at line 510 -- according to my local checkout. Could you please remove that 
line and have that change be part of this patch too? I did a grep on the 
project and that's the only "user-side" mention to the misc-redundant-... 
check. Running Tidy on Tidy itself afterwards would turn out to be a nice 
real-world test that the fix indeed works. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114622/new/

https://reviews.llvm.org/D114622

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Balázs Benics via Phabricator via cfe-commits]

steakhal created this revision.
steakhal added reviewers: aaron.ballman, alexfh, hokein, courbet, NoQ, 
xazax.hun, martong, whisperity, davrec.
Herald added subscribers: carlosgalvezp, manas, ASDenysPetrov, dkrupp, 
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, 
baloghadamsoftware, dylanmckay.
Herald added a reviewer: Szelethus.
steakhal requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Consider this example:

  #include 
  using std::is_same;
  bool top() {
return is_same::value || is_same::value;
  }

We can see that the `value` static members are different, but in fact,
they refer to the same VarDecl entity. It is because both `is_same`
class instances inherit from the common `false_type` class, which
actually owns the `value` member.

The `alpha.core.IdenticalExpr` Static Analyzer checker actually warns
for this, since it only checked if the referred `Decls` are the same.
Interestingly, the clang-tidy `misc-redundant-expression` also reports
this is for the exact same reason, so I'm fixing both of them at the same
time.

This patch fixes this by checking if the `Decls` are the same and if they
are it will try to acquire the nested namespace specifier as a type and
compare them as well. If they are inequal, that means that the
//spelling// of the nested namespace specifiers actually differed, this
it's likely to be a false-positive.

I'd like to note that the checker/check was actually right about that
the expressions were semantically equal in that given context, however,
we still don't want these reports in general.

We could introduce checker options to finetune this behavior if needed.

PS: According to the code coverage of the test, all touched parts are
completely covered.

Thanks David Rector (@davrec) for the idea on the cfe-dev forum:
https://lists.llvm.org/pipermail/cfe-dev/2021-November/069389.html


https://reviews.llvm.org/D114622

Files:
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
  clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
  clang/test/Analysis/identical-expressions.cpp

Index: clang/test/Analysis/identical-expressions.cpp
===
--- clang/test/Analysis/identical-expressions.cpp
+++ clang/test/Analysis/identical-expressions.cpp
@@ -1562,3 +1562,50 @@
   ;
   }
 }
+
+template  struct boolean_value {
+  static constexpr bool value = V;
+};
+template  struct my_trait : boolean_value {};
+
+bool respect_nested_name_specifiers(bool sink) {
+  sink |= my_trait::value || my_trait::value; // no-warning
+
+  sink |= my_trait::value || my_trait::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  using my_char = char;
+  sink |= my_trait::value || my_trait::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  return sink;
+}
+
+static void static_function() {}
+namespace my {
+int fn(int = 1);
+}
+void respect_namespaces(bool coin) {
+  namespace other = my;
+  using namespace other;
+
+  coin ? my::fn(1) : my::fn(2);// no-warning
+  coin ? my::fn(1) : other::fn(2); // no-warning
+
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  coin ? my::fn(1) : my::fn(1);
+  coin ? my::fn(1) : other::fn(1);
+  coin ? my::fn(1) : fn(1);
+
+  coin ? my::fn(1) : fn(); // FIXME: We should have a warning for this: the default parameter is also 1.
+
+  my::fn(1) & my::fn(1);// no-warning
+  my::fn(1) & other::fn(1); // no-warning
+
+  // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+  coin ? ::static_function() : ::static_function();
+  coin ? ::static_function() : static_function();
+}
Index: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
@@ -295,6 +295,31 @@
   return true;
 }
 
+static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
+   const NestedNameSpecifier *Right) {
+  if (const auto *L = Left->getAsType())
+if (const auto *R = Right->getAsType())
+  return L->getCanonicalTypeUnqualified() ==
+ R->getCanonicalTypeUnqualified();
+
+  // FIXME: We should probably check the other kinds of nested name specifiers.
+  return true;
+}
+
+static bool areEquivalentDeclRefs(const