[clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure

[hazohelet]

Summary

BEFORE this patch, when clang handles constraints like C1 || C2 where C1 evaluates to false and C2 evaluates to true, it emitted irrelevant diagnostics about the falsity of C1.
This patch removes the irrelevant diagnostic information generated during the evaluation of C1 if C2 evaluates to true.

Fixes https://github.com/llvm/llvm-project/issues/54678

[tbaeder]

Generally LGTM, but the erase call could do with a comment before. And I'll let someone with more concepts experience handle whether this should be accepted or not.

[hazohelet]

Ping

[erichkeane]

Sorry for the delay in review, I'm still on vacation. However, I don't like the idea of removing the satisfaction. I have to think about it more, but optimally we'd just not add the detail until we knew what the RHS value was, though our design doesn't particularly support that currently.

[hazohelet]

To generate only the necessary diagnostic information, we need to know the evaluation result of the entire constraint expression.
So, the ideal way I can think of would be first to evaluate the nodes and, simultaneously, cache the results using DenseMap or something, and then traverse the nodes again to generate diagnostic information.

I was going in this way, but (https://github.com/llvm/llvm-project/blob/6f8b17703da2fbabba974b82578530b152b79c26/clang/lib/Sema/SemaConcept.cpp#L365) needs to use TemplateDeductionInfo::takeSFINAEDiagnostic to generate the diagnostic information. Generating this TemplateDeductionInfo in the second traversal seems unreasonable or impossible because it involves template instantiation.

Also, I'm unsure whether this refactor is worth the additional traversal and the cache data cost.

I would like to hear opinions about the direction from reviewers.

Note: The current implementation adds diagnostic information in the following places:

• https://github.com/llvm/llvm-project/blob/6f8b17703da2fbabba974b82578530b152b79c26/clang/lib/Sema/SemaConcept.cpp#L258
• https://github.com/llvm/llvm-project/blob/6f8b17703da2fbabba974b82578530b152b79c26/clang/lib/Sema/SemaConcept.cpp#L288
• https://github.com/llvm/llvm-project/blob/6f8b17703da2fbabba974b82578530b152b79c26/clang/lib/Sema/SemaConcept.cpp#L378

[cor3ntin]

Given that we still need to check for substitution errors in untaken branches, this look reasonable (especially as i think you are right that alternative approaches are likely to be more complex) however i think it does require a comment, and I'd like @erichkeane to have the final say.

clang/lib/Sema/SemaConcept.cpp

222–226

This really needs a comment explaining that we are pruning details that do not matter for satisfaction, and a FIXME along the line of what Erich suggested.

[hazohelet]

Added comment and FIXME

[cor3ntin]

LGTM, but please wait a few days for @erichkeane

[erichkeane]

I think this is alright. Please see if you can figure out and fix what the pre-commit clang-format issue is.

[hazohelet]

When I first ran git clang-format locally, it somehow forced 4-space indent.
After starting to use clang-format from the trunk, I haven't seen the suspicious behavior locally, but the CI format check failure might be caused by that.
The format seems okay as-is, so I'll push this change without addressing the CI clang-format failure.

clang/docs/ReleaseNotes.rst

- Clang constexpr evaluator now diagnoses compound assignment operators against
   uninitialized variables as a read of uninitialized object.
   (`#51536 <https://github.com/llvm/llvm-project/issues/51536>`_)
 - Clang's ``-Wfortify-source`` now diagnoses ``snprintf`` call that is known to
   result in string truncation.
   (`#64871: <https://github.com/llvm/llvm-project/issues/64871>`_).
   Also clang no longer emits false positive warnings about the output length of
   ``%g`` format specifier and about ``%o, %x, %X`` with ``#`` flag.
 - Clang now emits ``-Wcast-qual`` for functional-style cast expressions.
+- Clang no longer emits irrelevant notes about unsatisfied constraint expressions
+  on the left-hand side of ``||`` when the right-hand side constraint is satisfied.
+  (`#54678: <https://github.com/llvm/llvm-project/issues/54678>`_).
 
 Bug Fixes in This Version
 -------------------------
 - Fixed an issue where a class template specialization whose declaration is
   instantiated in one module and whose definition is instantiated in another
   module may end up with members associated with the wrong declaration of the
   class, which can result in miscompiles in some cases.
 - Fix crash on use of a variadic overloaded operator.

clang/lib/Sema/SemaConcept.cpp

 template <typename AtomicEvaluator>
 static ExprResult
 calculateConstraintSatisfaction(Sema &S, const Expr *ConstraintExpr,
                                 ConstraintSatisfaction &Satisfaction,
                                 AtomicEvaluator &&Evaluator) {
   ConstraintExpr = ConstraintExpr->IgnoreParenImpCasts();
 
   if (LogicalBinOp BO = ConstraintExpr) {
+    auto EffectiveDetailEnd = Satisfaction.Details.end();
     ExprResult LHSRes = calculateConstraintSatisfaction(
         S, BO.getLHS(), Satisfaction, Evaluator);
 
     if (LHSRes.isInvalid())
       return ExprError();
 
     bool IsLHSSatisfied = Satisfaction.IsSatisfied;
 
if (BO.isAnd() && !IsLHSSatisfied)
       // LHS is instantiated while RHS is not. Skip creating invalid BinaryOp.
       return LHSRes;
 
     ExprResult RHSRes = calculateConstraintSatisfaction(
         S, BO.getRHS(), Satisfaction, std::forward<AtomicEvaluator>(Evaluator));
     if (RHSRes.isInvalid())
       return ExprError();
 
+    bool IsRHSSatisfied = Satisfaction.IsSatisfied;
+    // Current implementation adds diagnostic information about the falsity
+    // of each false atomic constraint expression when it evaluates them.
+    // When the evaluation results to `false || true`, the information
+    // generated during the evaluation of left-hand side is meaningless
+    // because the whole expression evaluates to true.
+    // The following code removes the irrelevant diagnostic information.
+    // FIXME: We should probably delay the addition of diagnostic information
+    // until we know the entire expression is false.
+    if (BO.isOr() && IsRHSSatisfied)
+      Satisfaction.Details.erase(EffectiveDetailEnd,
+                                 Satisfaction.Details.end());
+
     return BO.recreateBinOp(S, LHSRes, RHSRes);
   }
 
   if (auto *C = dyn_cast<ExprWithCleanups>(ConstraintExpr)) {
     // These aren't evaluated, so we don't care about cleanups, so we can just
     // evaluate these as if the cleanups didn't exist.
     return calculateConstraintSatisfaction(
         S, C->getSubExpr(), Satisfaction,

clang/test/SemaTemplate/concepts.cpp

 }
 
 void test_params() {
     params<int, true>();
     params<int, false>(); // expected-note {{in instantiation of function template specialization 'GH63181::params<int, false>' requested here}}
 }
 
 }
+
+namespace GH54678 {
+template<class>
+concept True = true;
+
+template<class>
+concept False = false; // expected-note 9 {{'false' evaluated to false}}
+
+template<class>
+concept Irrelevant = false;
+
+template <typename T>
+concept ErrorRequires = requires(ErrorRequires auto x) { x; }; // expected-error {{unknown type name 'ErrorRequires'}}
+
+template<class T> void aaa(T t) // expected-note {{candidate template ignored: constraints not satisfied}}
+requires (False<T> || False<T>) || False<T> {} // expected-note 3 {{'int' does not satisfy 'False'}}
+template<class T> void bbb(T t) // expected-note {{candidate template ignored: constraints not satisfied}}
+requires (False<T> || False<T>) && True<T> {} // expected-note 2 {{'long' does not satisfy 'False'}}
+template<class T> void ccc(T t) // expected-note {{candidate template ignored: constraints not satisfied}}
+requires (True<T> || Irrelevant<T>) && False<T> {} // expected-note {{'unsigned long' does not satisfy 'False'}}
+template<class T> void ddd(T t) // expected-note {{candidate template ignored: constraints not satisfied}}
+requires (Irrelevant<T> || True<T>) && False<T> {} // expected-note {{'int' does not satisfy 'False'}}
+template<class T> void eee(T t) // expected-note {{candidate template ignored: constraints not satisfied}}
+requires (Irrelevant<T> || Irrelevant<T> || True<T>) && False<T> {} // expected-note {{'long' does not satisfy 'False'}}
+
+template<class T> void fff(T t) // expected-note {{candidate template ignored: constraints not satisfied}}
+requires((ErrorRequires<T> || False<T> || True<T>) && False<T>) {} // expected-note {{'unsigned long' does not satisfy 'False'}}
+
+void test() {
+    aaa(42); // expected-error {{no matching function}}
+    bbb(42L); // expected-error{{no matching function}}
+    ccc(42UL); // expected-error {{no matching function}}
+    ddd(42); // expected-error {{no matching function}}
+    eee(42L); // expected-error {{no matching function}}
+    fff(42UL); // expected-error {{no matching function}}
+}
+}