Skip to content
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

Incorrect handling of incomplete types in std::derived_from #121278

Open
cor3ntin opened this issue Dec 28, 2024 · 2 comments · May be fixed by #121333
Open

Incorrect handling of incomplete types in std::derived_from #121278

cor3ntin opened this issue Dec 28, 2024 · 2 comments · May be fixed by #121333
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. rejects-valid

Comments

@cor3ntin
Copy link
Contributor

Per https://eel.is/c++draft/meta#lib:is_base_of,

Derived shall be a complete type

This is generally enforced by __is_base_of (the builtin used to implement is_base_of_v)

However, in libc++, derived_from is defined as

concept derived_from = is_base_of_v<_Bp, _Dp> && is_convertible_v<const volatile _Dp*, const volatile _Bp*>;

And because the constraints are only enforced in __is_base_of which is not in the immediate context of the is_base_of_v constraint, substituting an incomplete class in is_base_of_v does not result in a substitution failure in the immediate context (which should just cause derived_from to not be satisfied per https://eel.is/c++draft/temp.constr.atomic#3.sentence-2) but instead makes the program ill-formed, which i believe is non-conforming.

Note that GCC diverges in all cases. MSVC and EDG exposes what I believe to be the correct behavior.

I think there are 2 solutions:

  • Modify derived_from to use __is_base_of directly (which is what libstdc++ and MS STL do)
  • Add a require clause to std::is_base_of_v to check for completeness

https://godbolt.org/z/zGrKEKEao

@cor3ntin cor3ntin added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. rejects-valid labels Dec 28, 2024
@cor3ntin
Copy link
Contributor Author

cor3ntin commented Dec 28, 2024

Note that https://cplusplus.github.io/LWG/lwg-active.html#3929 and https://cplusplus.github.io/LWG/lwg-active.html#2939 are related

The fact that LWG suggest making it a Mandates (rather than a constraint) makes me question whether GCC is correct and this should always be a hard error (in which case it might be a clang bug)

@philnik777
Copy link
Contributor

IMO we should reject such code, since it can easily result in really hard-to-find bugs. Given that the LWG issues you linked seem to agree with me here, I'd rather have Clang reject both implementations of derived_from. I'm actually quite surprised that there is a difference between the two, which makes me question whether I should land #114840, or at least the <concepts> part of it.

cor3ntin added a commit to cor3ntin/llvm-project that referenced this issue Dec 30, 2024
…inae-friendly error

LWG3929 suggests that passing incomplete types to __is_base_of and
other builtins supporting [meta.unary] should result in a non-sfinaeable error.

This is consistent with GCC's behavior and avoid inconsistency when
using a builtin instead of a standard trait in a concept-definition.

Fixes llvm#121278
cor3ntin added a commit to cor3ntin/llvm-project that referenced this issue Dec 30, 2024
…inae-friendly error

LWG3929 suggests that passing incomplete types to __is_base_of and
other builtins supporting [meta.unary] should result in a non-sfinaeable error.

This is consistent with GCC's behavior and avoid inconsistency when
using a builtin instead of a standard trait in a concept-definition.

Fixes llvm#121278
cor3ntin added a commit to cor3ntin/llvm-project that referenced this issue Dec 30, 2024
…inae-friendly error

LWG3929 suggests that passing incomplete types to __is_base_of and
other builtins supporting [meta.unary] should result in a non-sfinaeable error.

This is consistent with GCC's behavior and avoid inconsistency when
using a builtin instead of a standard trait in a concept-definition.

Fixes llvm#121278
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. rejects-valid
Projects
None yet
2 participants