Skip to content

Bogus error when using __reference_converts_from_temporary in default template argument #132044

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
aaronpuchert opened this issue Mar 19, 2025 · 3 comments · May be fixed by #135390
Open

Bogus error when using __reference_converts_from_temporary in default template argument #132044

aaronpuchert opened this issue Mar 19, 2025 · 3 comments · May be fixed by #135390
Assignees
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"

Comments

@aaronpuchert
Copy link
Member

We ran into this with an std::function (libstdc++ 13) returning an object with protected copy constructor. It boils down to this:

class X {
protected:
    X(const X&);
};

static_assert(!__reference_converts_from_temporary(X, X));

template<typename A, typename B,
         bool Dangle = __reference_converts_from_temporary(A, B)>
static void test();

using Result = decltype(test<X, X>());

produces:

bug.cpp:9:24: error: calling a protected constructor of class 'X'
    9 |          bool Dangle = __reference_converts_from_temporary(A, B)>
      |                        ^

Replacing either A or B by X doesn't make the error go away. Interestingly, if we define test, for example with an empty body, and do a full instantiation, then the error appears if the instantiation is implicit, but not if it's explicit:

template<typename A, typename B,
         bool Dangle = __reference_converts_from_temporary(A, B)>
static void test() {} // <-- Added an empty body.

template void test<X, X>(); // No error.

void f() {
    test<X, X>(); // Error.
}
@aaronpuchert aaronpuchert added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Mar 19, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 19, 2025

@llvm/issue-subscribers-clang-frontend

Author: Aaron Puchert (aaronpuchert)

We ran into this with an `std::function` (libstdc++ 13) returning an object with protected copy constructor. It boils down to this:
class X {
protected:
    X(const X&amp;);
};

static_assert(!__reference_converts_from_temporary(X, X));

template&lt;typename A, typename B,
         bool Dangle = __reference_converts_from_temporary(A, B)&gt;
static void test();

using Result = decltype(test&lt;X, X&gt;());

produces:

bug.cpp:9:24: error: calling a protected constructor of class 'X'
    9 |          bool Dangle = __reference_converts_from_temporary(A, B)&gt;
      |                        ^

Replacing either A or B by X doesn't make the error go away. Interestingly, if we define test, for example with an empty body, and do a full instantiation, then the error appears if the instantiation is implicit, but not if it's explicit:

template&lt;typename A, typename B,
         bool Dangle = __reference_converts_from_temporary(A, B)&gt;
static void test() {} // &lt;-- Added an empty body.

template void test&lt;X, X&gt;(); // No error.

void f() {
    test&lt;X, X&gt;(); // Error.
}

@aaronpuchert
Copy link
Member Author

@cor3ntin, if you want to have a look.

@aaronpuchert aaronpuchert self-assigned this Apr 10, 2025
@aaronpuchert
Copy link
Member Author

aaronpuchert commented Apr 10, 2025

The issue seems to affect other type traits as well:

static_assert(!__is_convertible(X, X));

template<typename A, typename B,
         bool = __is_convertible(A, B)> // error: calling a protected constructor of class 'X'
static void test_conv();

using ResultConv = decltype(test_conv<X, X>());

and this:

class Y {
protected:
  Y(const X&);
};

template<typename A, typename B> struct BaseTemp {};
template<typename T> struct HasTypeMember {};

__builtin_common_type<BaseTemp, HasTypeMember, void, X, Y> f() {
  return; // Checks that we select `void`.
}

template<typename A, typename B,
         typename Res = __builtin_common_type<BaseTemp, HasTypeMember, void, A, B>>
static Res test_common_type();

decltype(test_common_type<X, Y>()) g() { return; }

Also here:

error: calling a protected constructor of class 'Y'
         typename Res = __builtin_common_type<BaseTemp, HasTypeMember, void, A, B>>
                        ^

For all these type traits, we use an SFINAETrap with AccessCheckingSFINAE = true to silence errors. The problem: errors might still be added to SuppressedDiagnostics in the TemplateDeductionInfo returned by isSFINAEContext (see Sema::EmitDiagnostic). When the overload candidate has been selected, those errors are emitted.

Why does this not happen outside of a template context or in an explicit instantiation? Both are not SFINAE contexts, and in the SFINAETrap constructor we have this:

      if (!SemaRef.isSFINAEContext())
        SemaRef.InNonInstantiationSFINAEContext = true;

So we already set the flag in non-SFINAE contexts. Setting this flag makes isSFINAEContext return a context that makes EmitDiagnostic swallow all errors. So I think we have to set InNonInstantiationSFINAEContext to true even in SFINAE contexts, when we're trapping for these builtins.

It seems that actually all places with AccessCheckingSFINAE = true want this behavior, so I think we simply have to change the if condition to !SemaRef.isSFINAEContext() || AccessCheckingSFINAE. Currently this looks good, and we still have nested instantations emitting errors as they should.

aaronpuchert added a commit to aaronpuchert/llvm-project that referenced this issue Apr 11, 2025
There are several type traits that produce a boolean value or type based
on the well-formedness of some expression (more precisely, the immediate
context, i.e. for example excluding nested template instantiation):
* `__is_constructible` and variants,
* `__is_convertible` and variants,
* `__is_assignable` and variants,
* `__reference_{binds_to,{constructs,converts}_from}_temporary`,
* `__is_trivially_equality_comparable`,
* `__builtin_common_type`.
(It should be noted that the standard doesn't always base this on the
immediate context being well-formed: for `std::common_type` it's based
on whether some expression "denotes a valid type." But I assume that's
an editorial issue and means the same thing.)

Errors in the immediate context are suppressed, instead the type traits
return another value or produce a different type if the expression is
not well-formed. This is achieved using an `SFINAETrap` with
`AccessCheckingSFINAE` set to true. If the type trait is used outside of
an SFINAE context, errors are discarded because in that case the
`SFINAETrap` sets `InNonInstantiationSFINAEContext`, which makes
`isSFINAEContext` return an `optional(nullptr)`, which causes the errors
to be discarded in `EmitDiagnostic`. However, in an SFINAE context this
doesn't happen, and errors are added to `SuppressedDiagnostics` in the
`TemplateDeductionInfo` returned by `isSFINAEContext`. Once we're done
with deducing template arguments and have decided which template is
going to be instantiated, the errors corresponding to the chosen
template are then emitted. At this point we get errors from those type
traits that we wouldn't have seen if used with the same arguments
outside of an SFINAE context. That doesn't seem right.

So what we want to do is always set `InNonInstantiationSFINAEContext`
when evaluating these well-formed-testing type traits, regardless of
whether we're in an SFINAE context or not. This should only affect the
immediate context, as nested contexts add a new `CodeSynthesisContext`
that resets `InNonInstantiationSFINAEContext` for the time it's active.

Going through uses of `SFINAETrap` with `AccessCheckingSFINAE` = `true`,
it occurred to me that all of them want this behavior and we can just
use this parameter to decide whether to use a non-instantiation context.
The uses are precisely the type traits mentioned above plus the
`TentativeAnalysisScope`, where I think it is also fine. (Though I think
we don't do tentative analysis in SFINAE contexts anyway.)

Because the parameter no longer just sets `AccessCheckingSFINAE` in Sema
but also `InNonInstantiationSFINAEContext`, I think it should be renamed
(along with uses, which also point the reviewer to the affected places).
Since we're testing for well-formedness of some expression, I think
`WellFormedSFINAE` is a good new name.

The added tests should more or less correspond to the users of
`SFINAETrap` with `AccessCheckingSFINAE` = `true`. I added a test for
errors outside of the immediate context for only one type trait, because
it requires some setup and is relatively noisy.

We put the `WellFormedSFINAE` condition first because it's constant in
all uses and would allow the compiler to prune the call to
`isSFINAEContext` when true.

Fixes llvm#132044.
aaronpuchert added a commit to aaronpuchert/llvm-project that referenced this issue Apr 11, 2025
There are several type traits that produce a boolean value or type based
on the well-formedness of some expression (more precisely, the immediate
context, i.e. for example excluding nested template instantiation):
* `__is_constructible` and variants,
* `__is_convertible` and variants,
* `__is_assignable` and variants,
* `__reference_{binds_to,{constructs,converts}_from}_temporary`,
* `__is_trivially_equality_comparable`,
* `__builtin_common_type`.

(It should be noted that the standard doesn't always base this on the
immediate context being well-formed: for `std::common_type` it's based
on whether some expression "denotes a valid type." But I assume that's
an editorial issue and means the same thing.)

Errors in the immediate context are suppressed, instead the type traits
return another value or produce a different type if the expression is
not well-formed. This is achieved using an `SFINAETrap` with
`AccessCheckingSFINAE` set to true. If the type trait is used outside of
an SFINAE context, errors are discarded because in that case the
`SFINAETrap` sets `InNonInstantiationSFINAEContext`, which makes
`isSFINAEContext` return an `optional(nullptr)`, which causes the errors
to be discarded in `EmitDiagnostic`. However, in an SFINAE context this
doesn't happen, and errors are added to `SuppressedDiagnostics` in the
`TemplateDeductionInfo` returned by `isSFINAEContext`. Once we're done
with deducing template arguments and have decided which template is
going to be instantiated, the errors corresponding to the chosen
template are then emitted. At this point we get errors from those type
traits that we wouldn't have seen if used with the same arguments
outside of an SFINAE context. That doesn't seem right.

So what we want to do is always set `InNonInstantiationSFINAEContext`
when evaluating these well-formed-testing type traits, regardless of
whether we're in an SFINAE context or not. This should only affect the
immediate context, as nested contexts add a new `CodeSynthesisContext`
that resets `InNonInstantiationSFINAEContext` for the time it's active.

Going through uses of `SFINAETrap` with `AccessCheckingSFINAE` = `true`,
it occurred to me that all of them want this behavior and we can just
use this parameter to decide whether to use a non-instantiation context.
The uses are precisely the type traits mentioned above plus the
`TentativeAnalysisScope`, where I think it is also fine. (Though I think
we don't do tentative analysis in SFINAE contexts anyway.)

Because the parameter no longer just sets `AccessCheckingSFINAE` in Sema
but also `InNonInstantiationSFINAEContext`, I think it should be renamed
(along with uses, which also point the reviewer to the affected places).
Since we're testing for well-formedness of some expression, I think
`WellFormedSFINAE` is a good new name.

The added tests should more or less correspond to the users of
`SFINAETrap` with `AccessCheckingSFINAE` = `true`. I added a test for
errors outside of the immediate context for only one type trait, because
it requires some setup and is relatively noisy.

We put the `WellFormedSFINAE` condition first because it's constant in
all uses and would allow the compiler to prune the call to
`isSFINAEContext` when true.

Fixes llvm#132044.
aaronpuchert added a commit to aaronpuchert/llvm-project that referenced this issue Apr 12, 2025
There are several type traits that produce a boolean value or type based
on the well-formedness of some expression (more precisely, the immediate
context, i.e. for example excluding nested template instantiation):
* `__is_constructible` and variants,
* `__is_convertible` and variants,
* `__is_assignable` and variants,
* `__reference_{binds_to,{constructs,converts}_from}_temporary`,
* `__is_trivially_equality_comparable`,
* `__builtin_common_type`.

(It should be noted that the standard doesn't always base this on the
immediate context being well-formed: for `std::common_type` it's based
on whether some expression "denotes a valid type." But I assume that's
an editorial issue and means the same thing.)

Errors in the immediate context are suppressed, instead the type traits
return another value or produce a different type if the expression is
not well-formed. This is achieved using an `SFINAETrap` with
`AccessCheckingSFINAE` set to true. If the type trait is used outside of
an SFINAE context, errors are discarded because in that case the
`SFINAETrap` sets `InNonInstantiationSFINAEContext`, which makes
`isSFINAEContext` return an `optional(nullptr)`, which causes the errors
to be discarded in `EmitDiagnostic`. However, in an SFINAE context this
doesn't happen, and errors are added to `SuppressedDiagnostics` in the
`TemplateDeductionInfo` returned by `isSFINAEContext`. Once we're done
with deducing template arguments and have decided which template is
going to be instantiated, the errors corresponding to the chosen
template are then emitted. At this point we get errors from those type
traits that we wouldn't have seen if used with the same arguments
outside of an SFINAE context. That doesn't seem right.

So what we want to do is always set `InNonInstantiationSFINAEContext`
when evaluating these well-formed-testing type traits, regardless of
whether we're in an SFINAE context or not. This should only affect the
immediate context, as nested contexts add a new `CodeSynthesisContext`
that resets `InNonInstantiationSFINAEContext` for the time it's active.

Going through uses of `SFINAETrap` with `AccessCheckingSFINAE` = `true`,
it occurred to me that all of them want this behavior and we can just
use this parameter to decide whether to use a non-instantiation context.
The uses are precisely the type traits mentioned above plus the
`TentativeAnalysisScope`, where I think it is also fine. (Though I think
we don't do tentative analysis in SFINAE contexts anyway.)

Because the parameter no longer just sets `AccessCheckingSFINAE` in Sema
but also `InNonInstantiationSFINAEContext`, I think it should be renamed
(along with uses, which also point the reviewer to the affected places).
Since we're testing for well-formedness of some expression, I think
`WellFormedSFINAE` is a good new name.

The added tests should more or less correspond to the users of
`SFINAETrap` with `AccessCheckingSFINAE` = `true`. I added a test for
errors outside of the immediate context for only one type trait, because
it requires some setup and is relatively noisy.

We put the `WellFormedSFINAE` condition first because it's constant in
all uses and would allow the compiler to prune the call to
`isSFINAEContext` when true.

Fixes llvm#132044.
aaronpuchert added a commit to aaronpuchert/llvm-project that referenced this issue Apr 12, 2025
There are several type traits that produce a boolean value or type based
on the well-formedness of some expression (more precisely, the immediate
context, i.e. for example excluding nested template instantiation):
* `__is_constructible` and variants,
* `__is_convertible` and variants,
* `__is_assignable` and variants,
* `__reference_{binds_to,{constructs,converts}_from}_temporary`,
* `__is_trivially_equality_comparable`,
* `__builtin_common_type`.

(It should be noted that the standard doesn't always base this on the
immediate context being well-formed: for `std::common_type` it's based
on whether some expression "denotes a valid type." But I assume that's
an editorial issue and means the same thing.)

Errors in the immediate context are suppressed, instead the type traits
return another value or produce a different type if the expression is
not well-formed. This is achieved using an `SFINAETrap` with
`AccessCheckingSFINAE` set to true. If the type trait is used outside of
an SFINAE context, errors are discarded because in that case the
`SFINAETrap` sets `InNonInstantiationSFINAEContext`, which makes
`isSFINAEContext` return an `optional(nullptr)`, which causes the errors
to be discarded in `EmitDiagnostic`. However, in an SFINAE context this
doesn't happen, and errors are added to `SuppressedDiagnostics` in the
`TemplateDeductionInfo` returned by `isSFINAEContext`. Once we're done
with deducing template arguments and have decided which template is
going to be instantiated, the errors corresponding to the chosen
template are then emitted. At this point we get errors from those type
traits that we wouldn't have seen if used with the same arguments
outside of an SFINAE context. That doesn't seem right.

So what we want to do is always set `InNonInstantiationSFINAEContext`
when evaluating these well-formed-testing type traits, regardless of
whether we're in an SFINAE context or not. This should only affect the
immediate context, as nested contexts add a new `CodeSynthesisContext`
that resets `InNonInstantiationSFINAEContext` for the time it's active.

Going through uses of `SFINAETrap` with `AccessCheckingSFINAE` = `true`,
it occurred to me that all of them want this behavior and we can just
use this parameter to decide whether to use a non-instantiation context.
The uses are precisely the type traits mentioned above plus the
`TentativeAnalysisScope`, where I think it is also fine. (Though I think
we don't do tentative analysis in SFINAE contexts anyway.)

Because the parameter no longer just sets `AccessCheckingSFINAE` in Sema
but also `InNonInstantiationSFINAEContext`, I think it should be renamed
(along with uses, which also point the reviewer to the affected places).
Since we're testing for well-formedness of some expression, I think
`WellFormedSFINAE` is a good new name.

The added tests should more or less correspond to the users of
`SFINAETrap` with `AccessCheckingSFINAE` = `true`. I added a test for
errors outside of the immediate context for only one type trait, because
it requires some setup and is relatively noisy.

We put the `WellFormedSFINAE` condition first because it's constant in
all uses and would allow the compiler to prune the call to
`isSFINAEContext` when true.

Fixes llvm#132044.
aaronpuchert added a commit to aaronpuchert/llvm-project that referenced this issue Apr 12, 2025
There are several type traits that produce a boolean value or type based
on the well-formedness of some expression (more precisely, the immediate
context, i.e. for example excluding nested template instantiation):
* `__is_constructible` and variants,
* `__is_convertible` and variants,
* `__is_assignable` and variants,
* `__reference_{binds_to,{constructs,converts}_from}_temporary`,
* `__is_trivially_equality_comparable`,
* `__builtin_common_type`.

(It should be noted that the standard doesn't always base this on the
immediate context being well-formed: for `std::common_type` it's based
on whether some expression "denotes a valid type." But I assume that's
an editorial issue and means the same thing.)

Errors in the immediate context are suppressed, instead the type traits
return another value or produce a different type if the expression is
not well-formed. This is achieved using an `SFINAETrap` with
`AccessCheckingSFINAE` set to true. If the type trait is used outside of
an SFINAE context, errors are discarded because in that case the
`SFINAETrap` sets `InNonInstantiationSFINAEContext`, which makes
`isSFINAEContext` return an `optional(nullptr)`, which causes the errors
to be discarded in `EmitDiagnostic`. However, in an SFINAE context this
doesn't happen, and errors are added to `SuppressedDiagnostics` in the
`TemplateDeductionInfo` returned by `isSFINAEContext`. Once we're done
with deducing template arguments and have decided which template is
going to be instantiated, the errors corresponding to the chosen
template are then emitted. At this point we get errors from those type
traits that we wouldn't have seen if used with the same arguments
outside of an SFINAE context. That doesn't seem right.

So what we want to do is always set `InNonInstantiationSFINAEContext`
when evaluating these well-formed-testing type traits, regardless of
whether we're in an SFINAE context or not. This should only affect the
immediate context, as nested contexts add a new `CodeSynthesisContext`
that resets `InNonInstantiationSFINAEContext` for the time it's active.

Going through uses of `SFINAETrap` with `AccessCheckingSFINAE` = `true`,
it occurred to me that all of them want this behavior and we can just
use this parameter to decide whether to use a non-instantiation context.
The uses are precisely the type traits mentioned above plus the
`TentativeAnalysisScope`, where I think it is also fine. (Though I think
we don't do tentative analysis in SFINAE contexts anyway.)

Because the parameter no longer just sets `AccessCheckingSFINAE` in Sema
but also `InNonInstantiationSFINAEContext`, I think it should be renamed
(along with uses, which also point the reviewer to the affected places).
Since we're testing for well-formedness of some expression, I think
`WellFormedSFINAE` is a good new name.

The added tests should more or less correspond to the users of
`SFINAETrap` with `AccessCheckingSFINAE` = `true`. I added a test for
errors outside of the immediate context for only one type trait, because
it requires some setup and is relatively noisy.

We put the `WellFormedSFINAE` condition first because it's constant in
all uses and would allow the compiler to prune the call to
`isSFINAEContext` when true.

Fixes llvm#132044.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants