[clang] Fix `static_cast` to array of unknown bound

[Fznamznon]

Summary

Per P1975R0 an expression like static_cast<U[]>(...) defines the type
of the expression as U[1].

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

[erichkeane]

clang/lib/Sema/SemaType.cpp

8859

I am not really sure this is the right way about this. You're supposed to be testing T, but this looks like it is checking the type of E, isn't it? I think you just need to check Cast->getType().

8861

Not supposed to have curleys here.

[Fznamznon]

clang/lib/Sema/SemaType.cpp

8859

For the case I'm trying to fix, T is array of unknown bound, it is already checked before calling completeExprArrayBound. Right now when you write something like

int (&&arr1)[1] = static_cast<int[]>(42);

Clang actually is able to realize that parenthesized initialization is made, so it actually generates CXXParenListInitExpr that has type int[1]. Here I'm just paranoidally double-checking that is the case before switching the type. Maybe I don't need this double-check at all then?

[erichkeane]

clang/lib/Sema/SemaType.cpp

8859

That makes me wonder if this is the correct place for this? Should when we generate this type when we do the CXXParenListInitExpr fixup?

Either way, I think making this depend on that behavior (which would possibly be fragile), we should just do it based on the type passed ot the static_cast.

Another question: is this something that needs to be done for other cast types? Does similar behavior exist for the other casts? Should it also happen with 'clobber' casts (C-style) that are effectively static?

[Fznamznon]

clang/lib/Sema/SemaType.cpp

8859

That makes me wonder if this is the correct place for this? Should when we generate this type when we do the CXXParenListInitExpr fixup?

The function called completeExprArrayBound seemed like an appropriate place for me.
Could you please elaborate what you mean under CXXParenListInitExpr fixup?

is this something that needs to be done for other cast types?

I'm not sure. https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1975r0.html seems to be extending only static_casts. And this was the original purpose of this bugfix.
gcc and msvc don't agree on that matter https://godbolt.org/z/1M3ahhsYr.

[erichkeane]

clang/lib/Sema/SemaType.cpp

8859

Could you please elaborate what you mean under CXXParenListInitExpr fixup?

You mentioned that at a different place we set the type of the CXXParenListInitExpr to be the 1-arity array. I was saying that if there is logic THERE based on whether its an incomplete array type it would be for the same purpose, and perhaps be the correct place to do this.

I'm not sure. https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1975r0.html seems to be extending only static_casts.

Yep, thats what I was hoping to see, thanks.

And this was the original purpose of this bugfix.

I was concerned that we increasing the inconsistency by only fixing 1 of the types of casts. It might seem like 'feature creep', but it is very important that we make sure a patch is reasonably complete, so that it doesn't make the compiler less consistent.

[Fznamznon]

clang/lib/Sema/SemaType.cpp

8859

I was saying that if there is logic THERE based on whether its an incomplete array type it would be for the same purpose, and perhaps be the correct place to do this.

Well, it seems to be happening inside of a TryStaticImplicitCast before the CXXStaticCastExpr created. There is initialization sequence performed, and the source expression comes out of as an array of 1 element. It is not very convenient to switch the type there since there is no cast expression itself yet and the destination type is passed as a copy through several functions. Will it be ok to check and switch the type of resulting static_cast expression just before/after it is created?

[Fznamznon]

Move changes to another place, check destination type

[Fznamznon]

Ping for review.

[aaron.ballman]

Thank you for working on this!

clang/lib/Sema/SemaCast.cpp

1269–1272 ↗

(On Diff #536172)

This seems incorrect to me -- I think the CastOperation should be created with the correct result type to begin with. What you are citing is a note, but the actual wording that specifies that note is: https://eel.is/c++draft/dcl.init#general-16.5 so I think the fix approach is somewhat correct but the code lives in the wrong place, this should be closer to the initialization code I think.

clang/test/SemaCXX/paren-list-agg-init.cpp

276–285

I'd like to see test coverage for:

int (&&arr)[] = (int[])(42);
int (&&arr1)[1] = (int[])(42);
int (&&arrr2)[2] = (int[])(42);
int (&&arr3)[3] = (int[3])(42);

where we're using a C-style cast, because: http://eel.is/c++draft/expr.cast#4

[Fznamznon]

clang/test/SemaCXX/paren-list-agg-init.cpp

276–285

Thank you for the review!

Just to double check, so it says:

The conversions performed by ... *all named casts* can be performed using the cast notation of explicit type conversion.

Does that mean the c-style cast should produce the same thing? And, https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1975r0.html doesn't say anything about c-style casts because it is assumed that it should be able to do anything that static_cast can do?

gcc doesn't agree https://godbolt.org/z/Pfq8frdn9 . The funny thing is that the original bug report seems to be using some kind of gcc test.

[Fznamznon]

Rebase, move the logic, modify c-style casts as well

[Fznamznon]

Ping.

[Fznamznon]

Rebase

[aaron.ballman]

clang/test/SemaCXX/paren-list-agg-init.cpp

276–285

Does that mean the c-style cast should produce the same thing? And, https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1975r0.html doesn't say anything about c-style casts because it is assumed that it should be able to do anything that static_cast can do?

Correct

gcc doesn't agree https://godbolt.org/z/Pfq8frdn9 . The funny thing is that the original bug report seems to be using some kind of gcc test.

MSVC and ICC both agree though, so I lean towards us being on the right path in allowing those casts. @hubert.reinterpretcast -- do you have thoughts?

[Fznamznon]

Rebase

[cor3ntin]

This looks reasonable to me know. should we try to land that today?
Sorry i could not get to it earlier!

[Fznamznon]

should we try to land that today?

I'm not sure. It causes failures in libc++ testing:

Failed Tests (3):
  llvm-libc++-shared.cfg.in :: std/ranges/range.access/end.pass.cpp
  llvm-libc++-shared.cfg.in :: std/ranges/range.access/rbegin.pass.cpp
  llvm-libc++-shared.cfg.in :: std/ranges/range.access/rend.pass.cpp

I haven't figured out why. Trying to compile:

#include <ranges>

using RangeCREndT = decltype(std::ranges::crend);
static_assert(!std::is_invocable_v<RangeCREndT, int (&)[]>);

fails with this patch only using libc++, libstdc++ is fine. I'm not sure it is my misunderstanding, bug in the patch or bug in libc++.

[aaron.ballman]

In D152003#4531404, @Fznamznon wrote:

should we try to land that today?

I'm not sure. It causes failures in libc++ testing:

Failed Tests (3):
  llvm-libc++-shared.cfg.in :: std/ranges/range.access/end.pass.cpp
  llvm-libc++-shared.cfg.in :: std/ranges/range.access/rbegin.pass.cpp
  llvm-libc++-shared.cfg.in :: std/ranges/range.access/rend.pass.cpp

I haven't figured out why. Trying to compile:

#include <ranges>

using RangeCREndT = decltype(std::ranges::crend);
static_assert(!std::is_invocable_v<RangeCREndT, int (&)[]>);

fails with this patch only using libc++, libstdc++ is fine. I'm not sure it is my misunderstanding, bug in the patch or bug in libc++.

CC @ldionne @philnik @Mordante for libc++ opinions

[philnik]

In D152003#4531732, @aaron.ballman wrote:

In D152003#4531404, @Fznamznon wrote:

should we try to land that today?

I'm not sure. It causes failures in libc++ testing:

Failed Tests (3):
  llvm-libc++-shared.cfg.in :: std/ranges/range.access/end.pass.cpp
  llvm-libc++-shared.cfg.in :: std/ranges/range.access/rbegin.pass.cpp
  llvm-libc++-shared.cfg.in :: std/ranges/range.access/rend.pass.cpp

I haven't figured out why. Trying to compile:

#include <ranges>

using RangeCREndT = decltype(std::ranges::crend);
static_assert(!std::is_invocable_v<RangeCREndT, int (&)[]>);

fails with this patch only using libc++, libstdc++ is fine. I'm not sure it is my misunderstanding, bug in the patch or bug in libc++.

CC @ldionne @philnik @Mordante for libc++ opinions

Thanks for the heads-up! From what I can tell, this could be a problem in the compiler, in is_invocable, in cend or in end. Unfortunately, I don't have the time right now to make a smaller reproducer and Louis isn't available. @Mordante do you have time to take a look at this? I will probably only have time to get to it by the end of the week or early next week.

[cor3ntin]

I think getting a smaller reproducer would be useful indeed before talking about conformance, even though i think there is indeed something wrong there - an incomplete parameter should not be able to produce a valid call.

[Mordante]

In D152003#4531404, @Fznamznon wrote:

should we try to land that today?

I'm not sure. It causes failures in libc++ testing:

Failed Tests (3):
  llvm-libc++-shared.cfg.in :: std/ranges/range.access/end.pass.cpp
  llvm-libc++-shared.cfg.in :: std/ranges/range.access/rbegin.pass.cpp
  llvm-libc++-shared.cfg.in :: std/ranges/range.access/rend.pass.cpp

I haven't figured out why. Trying to compile:

#include <ranges>

using RangeCREndT = decltype(std::ranges::crend);
static_assert(!std::is_invocable_v<RangeCREndT, int (&)[]>);

fails with this patch only using libc++, libstdc++ is fine. I'm not sure it is my misunderstanding, bug in the patch or bug in libc++.

What do you mean with libstdc++ is fine?
The reduced tests passed with libcstd++ https://godbolt.org/z/nh9v8cedr
The change seems to break libc++.

Based on the bug and this patch I am correct that "transformation" of an array of unknown bounds to anarray of known bounds only happens with an explicit cast and not implicitly?

I have a strong suspicion that this patch changes the test to

#include <ranges>

using RangeCREndT = decltype(std::ranges::crend);
static_assert(!std::is_invocable_v<RangeCREndT, int (&)[1]>); // no longer an array of unknown bound so the assert fails

Can you look at the AST output of the failed example to verify my hypothesis? When you think libc++ is wrong can you then post this AST?

[Fznamznon]

What do you mean with libstdc++ is fine?

What I mean is when I do (current patch is applied to clang):

source/llvm-project/build/bin/clang++ -std=c++20 t.cpp -c // compilation succeeds

But when I do (libc++ is built together with clang)

source/llvm-project/build/bin/clang++ -std=c++20 t.cpp -stdlib=libc++ -c
t.cpp:4:15: error: static assertion failed due to requirement '!std::is_invocable_v<const std::ranges::__crend::__fn, int (&)[]>'
    4 | static_assert(!std::is_invocable_v<RangeCREndT, int (&)[]>);
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

Based on the bug and this patch I am correct that "transformation" of an array of unknown bounds to anarray of known bounds only happens with an explicit cast and not implicitly?

Yes, type is changed only for explicit casts.

I have a strong suspicion that this patch changes the test to

Well technically it only changes explicit casts, so no. With this patch this static assertion still fails

static_assert(std::is_same<int (&)[1], int(&)[]>::value);

What I'm seeing is that in libc++ there is a bunch of explicit static casts in ranges::__crend::__fn that endup transformed:

namespace ranges {
namespace __crend {                                                                    
struct __fn {
  template <class _Tp>
    requires is_lvalue_reference_v<_Tp&&>                                              
  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
  constexpr auto operator()(_Tp&& __t) const
    noexcept(noexcept(ranges::rend(static_cast<const remove_reference_t<_Tp>&>(__t))))
    -> decltype(      ranges::rend(static_cast<const remove_reference_t<_Tp>&>(__t)))  
    { return          ranges::rend(static_cast<const remove_reference_t<_Tp>&>(__t)); }
                                                                                       
  template <class _Tp>                                                                 
    requires is_rvalue_reference_v<_Tp&&>                                              
  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
  constexpr auto operator()(_Tp&& __t) const
    noexcept(noexcept(ranges::rend(static_cast<const _Tp&&>(__t))))                    
    -> decltype(      ranges::rend(static_cast<const _Tp&&>(__t)))
    { return          ranges::rend(static_cast<const _Tp&&>(__t)); }
};
} // namespace __crend

Is that expected?

Please note that other implementations would do the same transformation and that is the point of this bugfix:
https://godbolt.org/z/ff8f3YEbz

[Mordante]

In D152003#4534963, @Fznamznon wrote:

What do you mean with libstdc++ is fine?

What I mean is when I do (current patch is applied to clang):

source/llvm-project/build/bin/clang++ -std=c++20 t.cpp -c // compilation succeeds

I see what you mean now.

What I'm seeing is that in libc++ there is a bunch of explicit static casts in ranges::__crend::__fn that endup transformed:

namespace ranges {
namespace __crend {                                                                    
struct __fn {
  template <class _Tp>
    requires is_lvalue_reference_v<_Tp&&>                                              
  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
  constexpr auto operator()(_Tp&& __t) const
    noexcept(noexcept(ranges::rend(static_cast<const remove_reference_t<_Tp>&>(__t))))
    -> decltype(      ranges::rend(static_cast<const remove_reference_t<_Tp>&>(__t)))  
    { return          ranges::rend(static_cast<const remove_reference_t<_Tp>&>(__t)); }
                                                                                       
  template <class _Tp>                                                                 
    requires is_rvalue_reference_v<_Tp&&>                                              
  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
  constexpr auto operator()(_Tp&& __t) const
    noexcept(noexcept(ranges::rend(static_cast<const _Tp&&>(__t))))                    
    -> decltype(      ranges::rend(static_cast<const _Tp&&>(__t)))
    { return          ranges::rend(static_cast<const _Tp&&>(__t)); }
};
} // namespace __crend

Is that expected?

I'm not very familiar with ranges but looking at the standard the static_cast is not required. I don't think the original author intended this behaviour which you implement in this patch. (To be honest I'm a bit surprised by this change in the language. I wonder how many other (library) developers it will catch off-guard.)

I agree this is a bug in libc++ and we should fix it. As mentioned I'm not too familiar with ranges. Maybe @var-const has time, otherwise
we need to wait for @philnik.

[Fznamznon]

I agree this is a bug in libc++ and we should fix it. As mentioned I'm not too familiar with ranges.

Thanks! I submitted https://github.com/llvm/llvm-project/issues/64250 so we don't forget about it.

clang/docs/ReleaseNotes.rst

- Fix the contains-errors bit not being set for DeclRefExpr that refers to a
   (`#48636 <https://github.com/llvm/llvm-project/issues/48636>`_),
   (`#50320 <https://github.com/llvm/llvm-project/issues/50320>`_).
 - Fix an assertion when using ``\u0024`` (``$``) as an identifier, by disallowing
   that construct (`#62133 <https://github.com/llvm/llvm-project/issues/38717>_`).
 - Fix crash caused by PseudoObjectExprBitfields: NumSubExprs overflow.
   (`#63169 <https://github.com/llvm/llvm-project/issues/63169>_`)
 - Fix crash when casting an object to an array type.
   (`#63758 <https://github.com/llvm/llvm-project/issues/63758>_`)
+- Fixed `static_cast` to array of unknown bound.
+  (`#62863 <https://github.com/llvm/llvm-project/issues/62863>`_).
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed a bug where attribute annotations on type specifiers (enums, classes,
   structs, unions, and scoped enums) were not properly ignored, resulting in

clang/lib/Sema/SemaType.cpp

 //===--- SemaType.cpp - Semantic Analysis for Types -----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
if (VarDecl *Var = dyn_cast<VarDecl>(DRE->getDecl())) {
         }
 
         // We still go on to try to complete the type independently, as it
         // may also require instantiations or diagnostics if it remains
         // incomplete.
       }
     }
   }
+  if (isa<ExplicitCastExpr>(E)) {
+    QualType DestType = E->getType();
+    if (const auto *IAT = Context.getAsIncompleteArrayType(DestType)) {
+      // C++20 [expr.static.cast]p.4: ... If T is “array of unknown bound of U”,
+      // this direct-initialization defines the type of the expression as U[1]
+      QualType ResultType = Context.getConstantArrayType(
+          IAT->getElementType(),
+          llvm::APInt(Context.getTypeSize(Context.getSizeType()), 1),
+          /*SizeExpr=*/nullptr, ArrayType::Normal, /*IndexTypeQuals=*/0);
+      E->setType(ResultType);
+    }
+  }
 }
 
 QualType Sema::getCompletedType(Expr *E) {
   // Incomplete array types may be completed by the initializer attached to
   // their definitions. For static data members of class templates and for
   // variable templates, we need to instantiate the definition to get this
   // initializer and complete the type.
   if (E->getType()->isIncompleteArrayType())

clang/test/SemaCXX/paren-list-agg-init.cpp

 }
 
 namespace gh63008 {
 auto a = new A('a', {1.1});
 // expected-warning@-1 {{braces around scalar init}}
 // beforecxx20-warning@-2 {{aggregate initialization of type 'A' from a parenthesized list of values is a C++20 extension}}
 }
 
 
 namespace GH63278 {
 struct S {
   int a = 0;
   int b {0};
   auto x = 1; // expected-error {{'auto' not allowed in non-static struct member}}
   static const auto y = 1;
 };
 
 int test() {
   // used to crash
   S a(0, 1);
   S b(0);
   S c(0, 0, 1); // beforecxx20-warning {{aggregate initialization of type 'S' from a parenthesized list of values is a C++20 extension}}
 
   S d {0, 1};
   S e {0};
   S f {0, 0, 1};
namespace GH63903 {
   struct S {
     int a = 0, b = f(); // expected-note {{undefined function 'f' cannot be used in a constant expression}}
   };
 
   // Test that errors produced by default members are produced at the location of the initialization
   constexpr S s(0); // beforecxx20-warning {{aggregate initialization of type 'const S' from a parenthesized list of values is a C++20 extension}} \
                     // expected-error {{constexpr variable 's' must be initialized by a constant expression}}
 }
+
+namespace gh62863 {
+int (&&arr)[] = static_cast<int[]>(42);
+// beforecxx20-warning@-1 {{aggregate initialization of type 'int[1]' from a parenthesized list of values is a C++20 extension}}
+int (&&arr1)[1] = static_cast<int[]>(42);
+// beforecxx20-warning@-1 {{aggregate initialization of type 'int[1]' from a parenthesized list of values is a C++20 extension}}
+int (&&arr2)[2] = static_cast<int[]>(42); // expected-error {{reference to type 'int[2]' could not bind to an rvalue of type 'int[1]'}}
+// beforecxx20-warning@-1 {{aggregate initialization of type 'int[1]' from a parenthesized list of values is a C++20 extension}}
+int (&&arr3)[3] = static_cast<int[3]>(42);
+// beforecxx20-warning@-1 {{aggregate initialization of type 'int[3]' from a parenthesized list of values is a C++20 extension}}
+
+int (&&arr4)[] = (int[])(42);
+// beforecxx20-warning@-1 {{aggregate initialization of type 'int[1]' from a parenthesized list of values is a C++20 extension}}
+int (&&arr5)[1] = (int[])(42);
+// beforecxx20-warning@-1 {{aggregate initialization of type 'int[1]' from a parenthesized list of values is a C++20 extension}}
+int (&&arr6)[2] = (int[])(42); // expected-error {{reference to type 'int[2]' could not bind to an rvalue of type 'int[1]'}}
+// beforecxx20-warning@-1 {{aggregate initialization of type 'int[1]' from a parenthesized list of values is a C++20 extension}}
+int (&&arr7)[3] = (int[3])(42);
+// beforecxx20-warning@-1 {{aggregate initialization of type 'int[3]' from a parenthesized list of values is a C++20 extension}}
+}