[PATCH] D157526: [clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb2cd9db58933: [clang][Sema] Remove irrelevant diagnostics
from constraint satisfaction failure (authored by hazohelet).
Changed prior to commit:
https://reviews.llvm.org/D157526?vs=556227&id=556936#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157526/new/
https://reviews.llvm.org/D157526
Files:
clang/docs/ReleaseNotes.rst
clang/lib/Sema/SemaConcept.cpp
clang/test/SemaTemplate/concepts.cpp
Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -994,3 +994,40 @@
}
}
+
+namespace GH54678 {
+template
+concept True = true;
+
+template
+concept False = false; // expected-note 9 {{'false' evaluated to false}}
+
+template
+concept Irrelevant = false;
+
+template
+concept ErrorRequires = requires(ErrorRequires auto x) { x; }; //
expected-error {{unknown type name 'ErrorRequires'}}
+
+template void aaa(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires (False || False) || False {} // expected-note 3 {{'int' does
not satisfy 'False'}}
+template void bbb(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires (False || False) && True {} // expected-note 2 {{'long' does
not satisfy 'False'}}
+template void ccc(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires (True || Irrelevant) && False {} // expected-note
{{'unsigned long' does not satisfy 'False'}}
+template void ddd(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires (Irrelevant || True) && False {} // expected-note {{'int'
does not satisfy 'False'}}
+template void eee(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires (Irrelevant || Irrelevant || True) && False {} //
expected-note {{'long' does not satisfy 'False'}}
+
+template void fff(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires((ErrorRequires || False || True) && False) {} //
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}}
+}
+}
Index: clang/lib/Sema/SemaConcept.cpp
===
--- clang/lib/Sema/SemaConcept.cpp
+++ clang/lib/Sema/SemaConcept.cpp
@@ -185,6 +185,7 @@
ConstraintExpr = ConstraintExpr->IgnoreParenImpCasts();
if (LogicalBinOp BO = ConstraintExpr) {
+auto EffectiveDetailEnd = Satisfaction.Details.end();
ExprResult LHSRes = calculateConstraintSatisfaction(
S, BO.getLHS(), Satisfaction, Evaluator);
@@ -218,6 +219,19 @@
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);
}
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -169,6 +169,9 @@
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: `_).
Bug Fixes in This Version
-
Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemp
[PATCH] D157526: [clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure
hazohelet added a comment. 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157526/new/ https://reviews.llvm.org/D157526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157526: [clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure
erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. I think this is alright. Please see if you can figure out and fix what the pre-commit clang-format issue is. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157526/new/ https://reviews.llvm.org/D157526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157526: [clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure
cor3ntin added a comment. LGTM, but please wait a few days for @erichkeane CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157526/new/ https://reviews.llvm.org/D157526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157526: [clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure
hazohelet updated this revision to Diff 556227.
hazohelet marked an inline comment as done.
hazohelet added a comment.
Added comment and FIXME
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157526/new/
https://reviews.llvm.org/D157526
Files:
clang/docs/ReleaseNotes.rst
clang/lib/Sema/SemaConcept.cpp
clang/test/SemaTemplate/concepts.cpp
Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -994,3 +994,40 @@
}
}
+
+namespace GH54678 {
+template
+concept True = true;
+
+template
+concept False = false; // expected-note 9 {{'false' evaluated to false}}
+
+template
+concept Irrelevant = false;
+
+template
+concept ErrorRequires = requires(ErrorRequires auto x) { x; }; //
expected-error {{unknown type name 'ErrorRequires'}}
+
+template void aaa(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires (False || False) || False {} // expected-note 3 {{'int' does
not satisfy 'False'}}
+template void bbb(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires (False || False) && True {} // expected-note 2 {{'long' does
not satisfy 'False'}}
+template void ccc(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires (True || Irrelevant) && False {} // expected-note
{{'unsigned long' does not satisfy 'False'}}
+template void ddd(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires (Irrelevant || True) && False {} // expected-note {{'int'
does not satisfy 'False'}}
+template void eee(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires (Irrelevant || Irrelevant || True) && False {} //
expected-note {{'long' does not satisfy 'False'}}
+
+template void fff(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires((ErrorRequires || False || True) && False) {} //
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}}
+}
+}
Index: clang/lib/Sema/SemaConcept.cpp
===
--- clang/lib/Sema/SemaConcept.cpp
+++ clang/lib/Sema/SemaConcept.cpp
@@ -185,6 +185,7 @@
ConstraintExpr = ConstraintExpr->IgnoreParenImpCasts();
if (LogicalBinOp BO = ConstraintExpr) {
+auto EffectiveDetailEnd = Satisfaction.Details.end();
ExprResult LHSRes = calculateConstraintSatisfaction(
S, BO.getLHS(), Satisfaction, Evaluator);
@@ -218,6 +219,19 @@
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);
}
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -166,6 +166,9 @@
Also clang no longer emits false positive warnings about the output length of
``%g`` format specifier.
- 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: `_).
Bug Fixes in This Version
-
Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -994,3 +994,40 @@
}
}
+
+namespace GH54678 {
+template
+concept True = true;
+
+template
+concept False = false; // expected-note 9 {{'false' evaluated to false}}
+
+template
+concept Irrelevant = false;
+
+template
+concept ErrorRequires = requires(ErrorRequires
[PATCH] D157526: [clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure
cor3ntin added a comment. 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. Comment at: clang/lib/Sema/SemaConcept.cpp:220-224 +bool IsRHSSatisfied = Satisfaction.IsSatisfied; +if (BO.isOr() && IsRHSSatisfied) + Satisfaction.Details.erase(EffectiveDetailEnd, + Satisfaction.Details.end()); + 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157526/new/ https://reviews.llvm.org/D157526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157526: [clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure
hazohelet added a comment. 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 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157526/new/ https://reviews.llvm.org/D157526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157526: [clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure
erichkeane added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157526/new/ https://reviews.llvm.org/D157526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157526: [clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure
hazohelet added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157526/new/ https://reviews.llvm.org/D157526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157526: [clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure
tbaeder added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157526/new/ https://reviews.llvm.org/D157526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157526: [clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure
hazohelet created this revision.
hazohelet added reviewers: aaron.ballman, erichkeane, tbaeder, shafik.
Herald added a project: All.
hazohelet requested review of this revision.
Herald added a project: clang.
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
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D157526
Files:
clang/docs/ReleaseNotes.rst
clang/lib/Sema/SemaConcept.cpp
clang/test/SemaTemplate/concepts.cpp
Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -994,3 +994,40 @@
}
}
+
+namespace GH54678 {
+template
+concept True = true;
+
+template
+concept False = false; // expected-note 9 {{'false' evaluated to false}}
+
+template
+concept Irrelevant = false;
+
+template
+concept ErrorRequires = requires(ErrorRequires auto x) { x; }; //
expected-error {{unknown type name 'ErrorRequires'}}
+
+template void aaa(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires (False || False) || False {} // expected-note 3 {{'int' does
not satisfy 'False'}}
+template void bbb(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires (False || False) && True {} // expected-note 2 {{'long' does
not satisfy 'False'}}
+template void ccc(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires (True || Irrelevant) && False {} // expected-note
{{'unsigned long' does not satisfy 'False'}}
+template void ddd(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires (Irrelevant || True) && False {} // expected-note {{'int'
does not satisfy 'False'}}
+template void eee(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires (Irrelevant || Irrelevant || True) && False {} //
expected-note {{'long' does not satisfy 'False'}}
+
+template void fff(T t) // expected-note {{candidate template ignored:
constraints not satisfied}}
+requires((ErrorRequires || False || True) && False) {} //
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}}
+}
+}
Index: clang/lib/Sema/SemaConcept.cpp
===
--- clang/lib/Sema/SemaConcept.cpp
+++ clang/lib/Sema/SemaConcept.cpp
@@ -183,6 +183,7 @@
ConstraintExpr = ConstraintExpr->IgnoreParenImpCasts();
if (LogicalBinOp BO = ConstraintExpr) {
+auto EffectiveDetailEnd = Satisfaction.Details.end();
ExprResult LHSRes = calculateConstraintSatisfaction(
S, BO.getLHS(), Satisfaction, Evaluator);
@@ -216,6 +217,11 @@
if (RHSRes.isInvalid())
return ExprError();
+bool IsRHSSatisfied = Satisfaction.IsSatisfied;
+if (BO.isOr() && IsRHSSatisfied)
+ Satisfaction.Details.erase(EffectiveDetailEnd,
+ Satisfaction.Details.end());
+
return BO.recreateBinOp(S, LHSRes, RHSRes);
}
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -125,6 +125,8 @@
of a base class is not called in the constructor of its derived class.
- Clang no longer emits ``-Wmissing-variable-declarations`` for variables
declared
with the ``register`` storage class.
+- Clang no longer emits irrelevant notes about unsatisfied constraint
expressions
+ on the left-hand side of ``||`` when the right-hand side constraint is
satisfied.
Bug Fixes in This Version
-
Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -994,3 +994,40 @@
}
}
+
+namespace GH54678 {
+template
+concept True = true;
+
+template
+concept False = false; // expected-note 9 {{'false' evaluated to false}}
+
+template
+concept Irrelevant = false;
+
+template
+concept ErrorRequires = requires(ErrorRequires auto x) { x; }; // expected-error {{unknown type name 'ErrorRequires'}}
+
+template void aaa(T t) // expected-note {{candidate template ignored: constraints not satisfied}}
+requires (False || False) || Fal
