-
Notifications
You must be signed in to change notification settings - Fork 13.7k
[Clang] Added explanation why is_constructible
evaluated to false.
#143309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… it is pack; added is_constructible support
@llvm/pr-subscribers-clang Author: Shamshura Egor (egorshamshura) ChangesAlso fixed problem with Full diff: https://github.com/llvm/llvm-project/pull/143309.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 5f44d503580b9..b04537d2cac3c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1765,7 +1765,8 @@ def err_user_defined_msg_constexpr : Error<
// Type traits explanations
def note_unsatisfied_trait : Note<"%0 is not %enum_select<TraitName>{"
"%TriviallyRelocatable{trivially relocatable}|"
- "%TriviallyCopyable{trivially copyable}"
+ "%TriviallyCopyable{trivially copyable}|"
+ "%Constructible{constructible with provided types}"
"}1">;
def note_unsatisfied_trait_reason
@@ -1785,7 +1786,9 @@ def note_unsatisfied_trait_reason
"%UserProvidedAssign{has a user provided %select{copy|move}1 "
"assignment operator}|"
"%UnionWithUserDeclaredSMF{is a union with a user-declared "
- "%sub{select_special_member_kind}1}"
+ "%sub{select_special_member_kind}1}|"
+ "%FunctionType{is a function type}|"
+ "%CVVoidType{is a cv void type}"
"}0">;
def warn_consteval_if_always_true : Warning<
diff --git a/clang/lib/Sema/SemaTypeTraits.cpp b/clang/lib/Sema/SemaTypeTraits.cpp
index 330f2aa750a09..37fe965815269 100644
--- a/clang/lib/Sema/SemaTypeTraits.cpp
+++ b/clang/lib/Sema/SemaTypeTraits.cpp
@@ -21,6 +21,7 @@
#include "clang/Sema/Overload.h"
#include "clang/Sema/Sema.h"
#include "clang/Sema/SemaHLSL.h"
+#include "llvm/Support/raw_ostream.h"
using namespace clang;
@@ -1941,6 +1942,7 @@ static std::optional<TypeTrait> StdNameToTypeTrait(StringRef Name) {
.Case("is_trivially_relocatable",
TypeTrait::UTT_IsCppTriviallyRelocatable)
.Case("is_trivially_copyable", TypeTrait::UTT_IsTriviallyCopyable)
+ .Case("is_constructible", TypeTrait::TT_IsConstructible)
.Default(std::nullopt);
}
@@ -1977,8 +1979,14 @@ static ExtractedTypeTraitInfo ExtractTypeTraitFromExpression(const Expr *E) {
Trait = StdNameToTypeTrait(Name);
if (!Trait)
return std::nullopt;
- for (const auto &Arg : VD->getTemplateArgs().asArray())
- Args.push_back(Arg.getAsType());
+ for (const auto &Arg : VD->getTemplateArgs().asArray()) {
+ if (Arg.getKind() == TemplateArgument::ArgKind::Pack) {
+ for (const auto &InnerArg : Arg.pack_elements())
+ Args.push_back(InnerArg.getAsType());
+ }
+ if (Arg.getKind() == TemplateArgument::ArgKind::Type)
+ Args.push_back(Arg.getAsType());
+ }
return {{Trait.value(), std::move(Args)}};
}
@@ -2159,6 +2167,26 @@ static void DiagnoseNonTriviallyCopyableReason(Sema &SemaRef,
}
}
+static void DiagnoseNonConstructibleReason(Sema &SemaRef, SourceLocation Loc,
+ QualType T) {
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait)
+ << T << diag::TraitName::Constructible;
+
+ if (T->isFunctionType())
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::FunctionType;
+
+ if (T->isVoidType())
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::CVVoidType;
+
+ const CXXRecordDecl *D = T->getAsCXXRecordDecl();
+ if (!D || D->isInvalidDecl() || !D->hasDefinition())
+ return;
+
+ SemaRef.Diag(D->getLocation(), diag::note_defined_here) << D;
+}
+
static void DiagnoseNonTriviallyCopyableReason(Sema &SemaRef,
SourceLocation Loc, QualType T) {
SemaRef.Diag(Loc, diag::note_unsatisfied_trait)
@@ -2195,6 +2223,9 @@ void Sema::DiagnoseTypeTraitDetails(const Expr *E) {
case UTT_IsTriviallyCopyable:
DiagnoseNonTriviallyCopyableReason(*this, E->getBeginLoc(), Args[0]);
break;
+ case TT_IsConstructible:
+ DiagnoseNonConstructibleReason(*this, E->getBeginLoc(), Args[0]);
+ break;
default:
break;
}
diff --git a/clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp b/clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp
index 498e202e26265..b9ee677a1cffc 100644
--- a/clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp
+++ b/clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp
@@ -20,6 +20,14 @@ struct is_trivially_copyable {
template <typename T>
constexpr bool is_trivially_copyable_v = __is_trivially_copyable(T);
+
+template <typename... Args>
+struct is_constructible {
+ static constexpr bool value = __is_constructible(Args...);
+};
+
+template <typename... Args>
+constexpr bool is_constructible_v = __is_constructible(Args...);
#endif
#ifdef STD2
@@ -44,6 +52,17 @@ using is_trivially_copyable = __details_is_trivially_copyable<T>;
template <typename T>
constexpr bool is_trivially_copyable_v = __is_trivially_copyable(T);
+
+template <typename... Args>
+struct __details_is_constructible{
+ static constexpr bool value = __is_constructible(Args...);
+};
+
+template <typename... Args>
+using is_constructible = __details_is_constructible<Args...>;
+
+template <typename... Args>
+constexpr bool is_constructible_v = __is_constructible(Args...);
#endif
@@ -73,6 +92,15 @@ using is_trivially_copyable = __details_is_trivially_copyable<T>;
template <typename T>
constexpr bool is_trivially_copyable_v = is_trivially_copyable<T>::value;
+
+template <typename... Args>
+struct __details_is_constructible : bool_constant<__is_constructible(Args...)> {};
+
+template <typename... Args>
+using is_constructible = __details_is_constructible<Args...>;
+
+template <typename... Args>
+constexpr bool is_constructible_v = is_constructible<Args...>::value;
#endif
}
@@ -100,6 +128,17 @@ static_assert(std::is_trivially_copyable_v<int&>);
// expected-note@-1 {{because it is a reference type}}
+static_assert(std::is_constructible<int, int>::value);
+
+static_assert(std::is_constructible<void>::value);
+// expected-error-re@-1 {{static assertion failed due to requirement 'std::{{.*}}is_constructible<void>::value'}} \
+// expected-note@-1 {{'void' is not constructible with provided types}} \
+// expected-note@-1 {{because it is a cv void type}}
+static_assert(std::is_constructible_v<void>);
+// expected-error@-1 {{static assertion failed due to requirement 'std::is_constructible_v<void>'}} \
+// expected-note@-1 {{'void' is not constructible with provided types}} \
+// expected-note@-1 {{because it is a cv void type}}
+
namespace test_namespace {
using namespace std;
static_assert(is_trivially_relocatable<int&>::value);
@@ -119,6 +158,15 @@ namespace test_namespace {
// expected-error@-1 {{static assertion failed due to requirement 'is_trivially_copyable_v<int &>'}} \
// expected-note@-1 {{'int &' is not trivially copyable}} \
// expected-note@-1 {{because it is a reference type}}
+
+ static_assert(is_constructible<void>::value);
+ // expected-error-re@-1 {{static assertion failed due to requirement '{{.*}}is_constructible<void>::value'}} \
+ // expected-note@-1 {{'void' is not constructible with provided types}} \
+ // expected-note@-1 {{because it is a cv void type}}
+ static_assert(is_constructible_v<void>);
+ // expected-error@-1 {{static assertion failed due to requirement 'is_constructible_v<void>'}} \
+ // expected-note@-1 {{'void' is not constructible with provided types}} \
+ // expected-note@-1 {{because it is a cv void type}}
}
@@ -139,6 +187,15 @@ concept C2 = std::is_trivially_copyable_v<T>; // #concept4
template <C2 T> void g2(); // #cand4
+template <typename... Args>
+requires std::is_constructible<Args...>::value void f3(); // #cand5
+
+template <typename... Args>
+concept C3 = std::is_constructible_v<Args...>; // #concept6
+
+template <C3 T> void g3(); // #cand6
+
+
void test() {
f<int&>();
// expected-error@-1 {{no matching function for call to 'f'}} \
@@ -169,5 +226,20 @@ void test() {
// expected-note@#concept4 {{because 'std::is_trivially_copyable_v<int &>' evaluated to false}} \
// expected-note@#concept4 {{'int &' is not trivially copyable}} \
// expected-note@#concept4 {{because it is a reference type}}
+
+ f3<void>();
+ // expected-error@-1 {{no matching function for call to 'f3'}} \
+ // expected-note@#cand5 {{candidate template ignored: constraints not satisfied [with Args = <void>]}} \
+ // expected-note-re@#cand5 {{because '{{.*}}is_constructible<void>::value' evaluated to false}} \
+ // expected-note@#cand5 {{'void' is not constructible with provided types}} \
+ // expected-note@#cand5 {{because it is a cv void type}}
+
+ g3<void>();
+ // expected-error@-1 {{no matching function for call to 'g3'}} \
+ // expected-note@#cand6 {{candidate template ignored: constraints not satisfied [with T = void]}} \
+ // expected-note@#cand6 {{because 'void' does not satisfy 'C3'}} \
+ // expected-note@#concept6 {{because 'std::is_constructible_v<void>' evaluated to false}} \
+ // expected-note@#concept6 {{'void' is not constructible with provided types}} \
+ // expected-note@#concept6 {{because it is a cv void type}}
}
}
diff --git a/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp b/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
index 0256569fcca5f..9e668ef86b31a 100644
--- a/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
+++ b/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
@@ -290,3 +290,47 @@ static_assert(__is_trivially_copyable(S12));
// expected-note@-1 {{'S12' is not trivially copyable}} \
// expected-note@#tc-S12 {{'S12' defined here}}
}
+
+namespace constructible {
+
+struct S1 { // #c-S1
+ S1(int);
+};
+static_assert(__is_constructible(S1, int, float));
+// expected-error@-1 {{static assertion failed due to requirement '__is_constructible(constructible::S1, int, float)'}} \
+// expected-note@-1 {{'S1' is not constructible with provided types}} \
+// expected-note@#c-S1 {{'S1' defined here}}
+
+struct S2 { // #c-S2
+ S2(int, float, double);
+};
+static_assert(__is_constructible(S2, float));
+// expected-error@-1 {{static assertion failed due to requirement '__is_constructible(constructible::S2, float)'}} \
+// expected-note@-1 {{'S2' is not constructible with provided types}} \
+// expected-note@#c-S2 {{'S2' defined here}}
+
+static_assert(__is_constructible(void));
+// expected-error@-1 {{static assertion failed due to requirement '__is_constructible(void)'}} \
+// expected-note@-1 {{'void' is not constructible with provided types}} \
+// expected-note@-1 {{because it is a cv void type}}
+
+static_assert(__is_constructible(const void));
+// expected-error@-1 {{static assertion failed due to requirement '__is_constructible(const void)'}} \
+// expected-note@-1 {{'const void' is not constructible with provided types}} \
+// expected-note@-1 {{because it is a cv void type}}
+
+static_assert(__is_constructible(volatile void));
+// expected-error@-1 {{static assertion failed due to requirement '__is_constructible(volatile void)'}} \
+// expected-note@-1 {{'volatile void' is not constructible with provided types}} \
+// expected-note@-1 {{because it is a cv void type}}
+
+static_assert(__is_constructible(int ()));
+// expected-error@-1 {{static assertion failed due to requirement '__is_constructible(int ())'}} \
+// expected-note@-1 {{'int ()' is not constructible with provided types}} \
+// expected-note@-1 {{because it is a function type}}
+
+static_assert(__is_constructible(void (int, float)));
+// expected-error@-1 {{static assertion failed due to requirement '__is_constructible(void (int, float))'}} \
+// expected-note@-1 {{'void (int, float)' is not constructible with provided types}} \
+// expected-note@-1 {{because it is a function type}}
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for working on that
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
@@ -2159,6 +2167,26 @@ static void DiagnoseNonTriviallyCopyableReason(Sema &SemaRef, | |||
} | |||
} | |||
|
|||
static void DiagnoseNonConstructibleReason(Sema &SemaRef, SourceLocation Loc, | |||
QualType T) { | |||
SemaRef.Diag(Loc, diag::note_unsatisfied_trait) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what would be useful here is to try to run overload resolution again on all the arguments,
and show the errors - https://compiler-explorer.com/z/aMG1nEoz1
You can look at how static bool EvaluateBooleanTypeTrait
works (in the same file).
I think that by calling InitializationSequence::Diagnose
we would get super useful diagnostics.
We don't have a good way to turn the errors that would be produced into notes, though @AaronBallman @erichkeane @Sirraide - but maybe emitting errors is good enough as a first approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have a good way to turn the errors that would be produced into notes
I’m working on something diagnostics-related right now that would allow nesting errors but it’s not finished yet. Because of how that works though it should be straight-forward to add the nesting later, so just emitting the errors for now should be fine imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your advice @cor3ntin (and also can i ping you?). I have tried to use InitializationSequence::Diagnose
. However I'm dealing with some issues.
InitializationSequence::Diagnose
does not emit any diagnostic. It correctly returns true or false, but nothing appears in console. Although the FailureKind inInitializationSequence
is defined correctly.- After
InitializationSequence::Diagnose
I can not useSemaRef.Diag
. Nothing is being written to the console. However I can write inllvm::errs
manually.
So, can you please help me understand what I'm doing wrong? I have uploaded the latest update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I figured it out! SFINAETrap was the problem. So now it works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, except for a check that does not seem to be useful
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
if (ArgTy->isIncompleteType()) { | ||
SemaRef.Diag(Loc, diag::err_incomplete_type_used_in_type_trait_expr) | ||
<< ArgTy; | ||
CompleteTypes = false; | ||
} | ||
} | ||
if (!CompleteTypes) | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete types should have been diagnosed already, so the check for incomplete type is not useful (we do need checks for void and incomplete array types)
https://godbolt.org/z/hc5jrjz51
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it good now? According to my observations when Args contains cv void type it is always evaluated to false, however I can not say the same about incomplete array types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And now, for every cv void type, we show a note, but is this a good practice? Or do we want to show it once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I decided to show it once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments, but generally looks good to me.
for (const auto &Arg : VD->getTemplateArgs().asArray()) { | ||
if (Arg.getKind() == TemplateArgument::ArgKind::Pack) { | ||
for (const auto &InnerArg : Arg.pack_elements()) | ||
Args.push_back(InnerArg.getAsType()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IS this a valid assumption here (that this is a type?). Presumably this could be an NTTP pack as well...
Though getAsType
would assert for us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getAsType would assert for us
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
for (const auto &InnerArg : Arg.pack_elements()) | ||
Args.push_back(InnerArg.getAsType()); | ||
} | ||
if (Arg.getKind() == TemplateArgument::ArgKind::Type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (Arg.getKind() == TemplateArgument::ArgKind::Type) | |
else if (Arg.getKind() == TemplateArgument::ArgKind::Type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
} | ||
if (Arg.getKind() == TemplateArgument::ArgKind::Type) | ||
Args.push_back(Arg.getAsType()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the kind is neither, we should probably do SOMETHING about it, even if it is just an assert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added assert
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
static void DiagnoseNonConstructibleReason( | ||
Sema &SemaRef, SourceLocation Loc, | ||
const llvm::SmallVector<clang::QualType, 1> &Ts) { | ||
for (const auto &ArgTy : Ts) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (const auto &ArgTy : Ts) { | |
for (const QualType &ArgTy : Ts) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
<< diag::TraitNotSatisfiedReason::CVVoidType; | ||
} | ||
|
||
QualType T = Ts[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know that Ts
is >=1? in size? Maybe early-exit if Ts.empty
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added early-exit, thanks
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
if (Arg.getKind() == TemplateArgument::ArgKind::Pack) { | ||
for (const auto &InnerArg : Arg.pack_elements()) | ||
Args.push_back(InnerArg.getAsType()); | ||
} else if (Arg.getKind() == TemplateArgument::ArgKind::Type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This else if
still needs curley brackets, since its if
did.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, added curley brackets
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
Args.push_back(InnerArg.getAsType()); | ||
} else if (Arg.getKind() == TemplateArgument::ArgKind::Type) | ||
Args.push_back(Arg.getAsType()); | ||
assert((Arg.getKind() == TemplateArgument::ArgKind::Type || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest just putting an llvm_unreachable
in the else
branch instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed assert to llvm_unreachable
is_constructible
evaluated to false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks a lot.
Will you need us to merge that for you?
|
Hi @egorshamshura and @cor3ntin It seems like a bunch of libc++ testcases start failing with this patch:
The
failure happens also before this patch, but the other ones are new. One example of a failure printout for libcxx/utilities/expected/expected.expected/value.observers.verify.cpp:
Seen e.g. here: https://lab.llvm.org/buildbot/#/builders/55/builds/12732 |
Hi @egorshamshura and @cor3ntin, we are seeing the test failures mentioned in the previous comment in our downstream builders as well. If a forward-fix can't be produced quickly, could you please revert this change while investigating? |
…valuated to false. " (#144127) Reverts llvm/llvm-project#143309 Someone needs to go through the libc++ tests and update the diagnostics checks in those tests (ie, i don't believe there was anything wrong with the PR, but it impacts libc++ tests nonetheless
…lvm#143309) Added explanation why a is constructible evaluated to false. Also fixed problem with ```ExtractTypeTraitFromExpression```. In case ```std::is_xxx_v<>``` with variadic pack it tries to get template argument, but fails in expression ```Arg.getAsType()``` due to ```Arg.getKind() == TemplateArgument::ArgKind::Pack```, but not ```TemplateArgument::ArgKind::Type```.
… false. " (llvm#144127) Reverts llvm#143309 Someone needs to go through the libc++ tests and update the diagnostics checks in those tests (ie, i don't believe there was anything wrong with the PR, but it impacts libc++ tests nonetheless
Added explanation why a is constructible evaluated to false. Also fixed problem with
ExtractTypeTraitFromExpression
. In casestd::is_xxx_v<>
with variadic pack it tries to get template argument, but fails in expressionArg.getAsType()
due toArg.getKind() == TemplateArgument::ArgKind::Pack
, but notTemplateArgument::ArgKind::Type
.