[Sema] List conversion validate character array

[Mordante]

Summary

The function TryListConversion didn't properly validate the following part of the standard:

Otherwise, if the parameter type is a character array [... ]
and the initializer list has a single element that is an
appropriately-typed string literal (8.5.2 [dcl.init.string]), the
implicit conversion sequence is the identity conversion.

This caused the following call to f() to be ambiguous.

void f(int(&&)[1]);
void f(unsigned(&&)[1]);

void g(unsigned i) {
  f({i});
}

This issue only occurrs when the initializer list had one element.

[rsmith]

clang/lib/Sema/SemaOverload.cpp

4986–4987

Phabricator thinks you converted spaces to tabs here. Please can you check and convert back if necessary?

4987

isCharType is too narrow a check here. We also need to check for all the other types that can be initialized from a string literal (wchar_t, char16_t, ... -- including unsigned short in some cases). These details are handled by [IsStringInit](https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaInit.cpp#L136). Maybe it's worth exposing that as a bool Sema::isStringInit function or similar to use from here?

4988

This is too narrow a check in two ways: we should allow parenthesized string literals here, and we should allow ObjCEncodeExpr.

[Mordante]

Thanks for the feedback!

clang/lib/Sema/SemaOverload.cpp

4986–4987

That's odd in my editor I see spaces. I ran git clang-format on an earlier version where I made some changes here. I'll revert the hunk.

4987

I think exposing this function sounds good.

[Mordante]

clang/lib/Sema/SemaOverload.cpp

4988

Actually it seems the code isn't behaving properly at all. It seems the conversion is done by
https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaOverload.cpp#L5174
resulting in an array-to-pointer conversion instead of an identity conversion.

It can solve it by manually removing the decay like:

if (const auto *DT = dyn_cast<DecayedType>(ToType))
  if (const auto *AT = S.Context.getAsArrayType(DT->getOriginalType()))
    if (S.IsStringInit(From->getInit(0), AT) {
       ...

This code works and results in an identity conversion. But it feels a bit odd to manually peel away the array-to-pointer decay. Is this the best solution or do you have a better suggestions?

[rsmith]

clang/lib/Sema/SemaOverload.cpp

4988

I think this is a bug in your testcases. I'll comment below.

clang/test/CXX/drs/dr14xx.cpp

411–414

These should presumably be references to arrays, rather than arrays, or the parameter type is as if you wrote (for example) void f(const char *), which shouldn't get the special treatment here.

[over.ics.list]p4 mentions this in its footnote:

"Otherwise, if the parameter type is a character array [Footnote: Since there are no parameters of array type, this will only occur as the referenced type of a reference parameter.] and the initializer list has a single element that is an appropriately-typed string-literal (9.4.3), the implicit conversion sequence is the identity conversion."

[Mordante]

clang/test/CXX/drs/dr14xx.cpp

411–414

Ah I missed that footnote. I reread the standard and can you confirm some cases?

namespace A { 
  void f(const char(&)[4]);
  void g() { f({"abc"}); }
}
namespace B { 
  void f(const char(&&)[4]);
  void g() { f({"abc"}); }
}

both should work and with P0388 the array without bounds will also work.

I ask since this is my interpretation of the standard, but it seems there's a difference between implementations and void f(const char(&&)[4]); fails for "abc" but works for "ab".
It seems ICC and consider "abc" an lvalue in this case and not when using "ab".

Here's a gotbolt link with the examples https://godbolt.org/z/r1TKfx

[rsmith]

clang/test/CXX/drs/dr14xx.cpp

411–414

That's a really interesting example :)

The initializer is a list containing an lvalue of type const char[4]. Per [dcl.init.list]/3.9 and /3.10, the behavior depends on whether the referenced type is reference-related to const char[4] -- if so, then the reference can only bind directly and a && reference will be invalid, because it's binding an rvalue reference to an lvalue, and if not, then we create an array temporary and the && binding is fine.

Per [dcl.init.ref]/4, const char[???] is reference-related to const char[4] if they are similar types, and per [conv.qual]/2, the types are similar if ??? is omitted or 4, and not similar otherwise.

So:

• const char (&&r)[4] = {"abc"} is ill-formed (types are the same, binds rvalue reference to lvalue)
• const char (&&)[] = {"abc"} is ill-formed (types are similar, binds rvalue reference to lvalue)
• const char (&&r)[5] = {"abc"} is OK (types are not similar, creates temporary)

That seems like a very surprising outcome to me. Perhaps we should probably ignore array bounds entirely when determining whether two types are reference-related. I'll take this to CWG for discussion.

[rsmith]

clang/test/CXX/drs/dr14xx.cpp

411–414

I think that's only an answer to half of your question. The other half is that [over.ics.list]p4 does not apply (directly) to either of your testcases, because the "parameter type" is a reference type, not an array type. For:

namespace A { 
  void f(const char(&)[4]);
  void g() { f({"abc"}); }
}

... we reach [over.ics.list]p9, which says to use the rules in [over.ics.ref]. Those rules say that if the reference binds directly to an argument expression (ignore the "expression" here; this is very old wording that predates braced initializers), then we form an identity conversion. So that's what happens in this case.

For

namespace B { 
  void f(const char(&&)[4]);
  void g() { f({"abc"}); }
}

the same thing happens, but now [over.ics.ref]p3 says "an implicit conversion sequence cannot be formed if it requires binding an lvalue reference other than a reference to a non-volatile const type to an rvalue or binding an rvalue reference to an lvalue other than a function lvalue", so the candidate is not viable.

If the array bound were omitted or were not 4, then the reference would not bind directly, and we would instead consider initializing a temporary. [over.ics.ref]p2 says "When a parameter of reference type is not bound directly to an argument expression, the conversion sequence is the one required to convert the argument expression to the referenced type according to 12.4.4.2.", which takes us back around to [over.ics.list] with the "parameter type" now being the array type. That's how we can reach [over.ics.list]p4 and consider string literal -> array initialization.

So I think that, according to the current rules, for

void f(const char (&&)[4]); // #1
void f(const char (&&)[5]); // #2

... a call to f({"abc"}) remarkably calls #2, because #1 is not viable (would bind rvalue reference to lvalue string literal), but #2 is, just like const char (&&r)[4] = {"abc"}; is ill-formed but const char (&&r)[5] = {"abc"}; is valid.

[Mordante]

clang/test/CXX/drs/dr14xx.cpp

411–414

That's a really interesting example :)

Thanks :-)
Also thanks for the detailed explanation of the rules applying to these cases.

So:

• const char (&&r)[4] = {"abc"} is ill-formed (types are the same, binds rvalue reference to lvalue)
• const char (&&)[] = {"abc"} is ill-formed (types are similar, binds rvalue reference to lvalue)

Interesting, wasn't P0388 trying to solve the issues with array of unknown bound?
Am I correct to assume it's intended that this code will become valid in the future?

• const char (&&r)[5] = {"abc"} is OK (types are not similar, creates temporary)

That seems like a very surprising outcome to me. Perhaps we should probably ignore array bounds entirely when determining whether two types are reference-related. I'll take this to CWG for discussion.

I agree this seems unexpected behaviour, thanks for discussing it with CWG.

So I think that, according to the current rules, for

void f(const char (&&)[4]); #1
void f(const char (&&)[5]); #2

... a call to f({"abc"}) remarkably calls #2, because #1 is not viable (would bind rvalue reference to lvalue string literal), but #2 is, just like const char (&&r)[4] = {"abc"}; is ill-formed but const char (&&r)[5] = {"abc"}; is valid.

MSVC agrees with your observation, GCC picks #1 and fails to create the call, and ICC aborts with an ICE
https://godbolt.org/z/s7nTPP

[Mordante]

Addresses the review comments.
Adds an extra test case to test whether the proper overload is called. The proper overload is a bit of a surprise so when the expected behaviour changes the overload test can be adjusted.

[rsmith]

Thanks, only nits here. Please feel free to submit after addressing them, or request another round of review if you prefer.

clang/lib/Sema/SemaInit.cpp

3115–3118

Consider moving this up to around line 144 (just after the definition of ::IsStringInit).

clang/lib/Sema/SemaOverload.cpp

4988

Nit: the if body is long, please add braces.

clang/test/CXX/drs/dr14xx.cpp

338–340

It would be useful to also test that the right overload is used here, and for g2.

411–414

Am I correct to assume it's intended that this code will become valid in the future?

I think it's more likely that the corresponding cases with other array bounds will become ill-formed. String literals are lvalues, so binding an rvalue reference to a string literal should not be valid based on our normal principles; allowing the wrong-array-bound case is then pretty surprising emergent behavior of the form that we usually disallow (on the basis that it "looks sufficiently like" the intention was for the reference to bind directly, so it's an error if the reference can't bind directly).

I think the likely outcomes are either that we ignore array bounds when determining reference-relatedness, or that we always create a temporary when binding an rvalue-reference-to-char-array to a string literal, regardless of whether the bound matches. Or maybe that we set the array bound of the string literal to that of the reference-to-array, allowing the const & cases but not the && cases with an incorrect bound. But I wouldn't want to guess which.

424–431

Please test that the correct overload is called in each of these cases (especially considering the unintuitive outcome of these calls).

clang/test/SemaCXX/overload-array-size.cpp

3–5

6–13

It's more common to test such things via diagnostics rather than by AST dumps. (AST dump tests tend to be more fragile in practice.) You could mark the [5] case as =delete and check that you get the "call to a deleted function" diagnostic, or you could change the [5] case to return int & and check that you can initialize an int & from a call to it, or something like that.

[Mordante]

clang/test/CXX/drs/dr14xx.cpp

411–414

Thanks for your insight.

clang/test/SemaCXX/overload-array-size.cpp

6–13

I'm also not too fond of the AST checks, but forgot I could use = delete to detect whether the wanted overload is tested. Since I adjusted the other tests as suggested this test has no additional value and I removed it.

clang/include/clang/Sema/Sema.h

ExprResult PerformMoveOrCopyInitialization(const InitializedEntity &Entity,
                                              const VarDecl *NRVOCandidate,
                                              QualType ResultType,
                                              Expr *Value,
                                              bool AllowNRVO = true);
 
   bool CanPerformAggregateInitializationForOverloadResolution(
       const InitializedEntity &Entity, InitListExpr *From);
 
+  bool IsStringInit(Expr *Init, const ArrayType *AT);
+
   bool CanPerformCopyInitialization(const InitializedEntity &Entity,
                                     ExprResult Init);
   ExprResult PerformCopyInitialization(const InitializedEntity &Entity,
                                        SourceLocation EqualLoc,
                                        ExprResult Init,
                                        bool TopLevelOfInitList = false,
                                        bool AllowExplicit = false);
   ExprResult PerformObjectArgumentInitialization(Expr *From,

clang/lib/Sema/SemaInit.cpp

bool Sema::CanPerformAggregateInitializationForOverloadResolution(
     const InitializedEntity &Entity, InitListExpr *From) {
   QualType Type = Entity.getType();
   InitListChecker Check(*this, Entity, From, Type, /*VerifyOnly=*/true,
                         /*TreatUnavailableAsInvalid=*/false,
                         /*InOverloadResolution=*/true);
   return !Check.HadError();
 }
 
+bool Sema::IsStringInit(Expr *Init, const ArrayType *AT) {
+  return ::IsStringInit(Init, AT, Context) == SIF_None;
+}
+
 /// Check that the given Index expression is a valid array designator
 /// value. This is essentially just a wrapper around
 /// VerifyIntegerConstantExpression that also checks for negative values
 /// and produces a reasonable diagnostic if there is a
 /// failure. Returns the index expression, possibly with an implicit cast
 /// added, on success.  If everything went okay, Value will receive the
 /// value of the constant expression.
 static ExprResult

clang/lib/Sema/SemaOverload.cpp

if (From->getNumInits() == 1) {
     if (ToType->isRecordType()) {
       QualType InitType = From->getInit(0)->getType();
       if (S.Context.hasSameUnqualifiedType(InitType, ToType) ||
           S.IsDerivedFrom(From->getBeginLoc(), InitType, ToType))
         return TryCopyInitialization(S, From->getInit(0), ToType,
                                      SuppressUserConversions,
                                      InOverloadResolution,
                                      AllowObjCWritebackConversion);
     }
-    // FIXME: Check the other conditions here: array of character type,
-    // initializer is a string literal.
-    if (ToType->isArrayType()) {
+
+    if (const auto *AT = S.Context.getAsArrayType(ToType))
+      if (S.IsStringInit(From->getInit(0), AT)) {
         InitializedEntity Entity =
           InitializedEntity::InitializeParameter(S.Context, ToType,
                                                  /*Consumed=*/false);
         if (S.CanPerformCopyInitialization(Entity, From)) {
           Result.setStandard();
           Result.Standard.setAsIdentityConversion();
           Result.Standard.setFromType(ToType);
           Result.Standard.setAllToTypes(ToType);
           return Result;
         }
       }
   }
 
   // C++14 [over.ics.list]p2: Otherwise, if the parameter type [...] (below).
   // C++11 [over.ics.list]p2:
   //   If the parameter type is std::initializer_list<X> or "array of X" and
   //   all the elements can be implicitly converted to X, the implicit
   //   conversion sequence is the worst conversion necessary to convert an
   //   element of the list to X.

clang/test/CXX/drs/dr14xx.cpp

namespace dr_example {
 
     struct X {
       X() = default;
       X(const X&) = default;
     };
 
     X x;
     X x2{x};
+
+    void f1(int);
+    void f1(std::initializer_list<long>);
+    void g1() { f1({42}); }
+
+    template <class T, class U>
+    struct Pair {
+      Pair(T, U);
+    };
+    struct String {
+      String(const char *);
+    };
+
+    void f2(Pair<const char *, const char *>);
+    void f2(std::initializer_list<String>);
+    void g2() { f2({"foo", "bar"}); }
   } // dr_example
 
   namespace nonaggregate {
     struct NonAggregate {
       NonAggregate() {}
     };
 
     struct WantsIt {
namespace dr1467 {  // dr1467: 3.7 c++11
   NestedInit ni[1] = {{NestedInit{1, 2, 3}}};
 
   namespace NestedInit2 {
     struct Pair { int a, b; };
     struct TwoPairs { TwoPairs(Pair, Pair); };
     struct Value { Value(Pair); Value(TwoPairs); };
     void f() { Value{{{1,2},{3,4}}}; }
   }
+  namespace NonAmbiguous {
+  // The original implementation made this case ambigious due to the special
+  // handling of one element initialization lists.
+  void f(int(&&)[1]);
+  void f(unsigned(&&)[1]);
+
+  void g(unsigned i) {
+    f({i});
+  }
+  } // namespace NonAmbiguous
+
+#if __cplusplus >= 201103L
+  namespace StringLiterals {
+  void f(const char(&&)[4]);
+  void f(const char(&&)[5]);
+  void f(const wchar_t(&&)[4]);
+  void f(const wchar_t(&&)[5]);
+#if __cplusplus >= 202002L
+  void f(const char8_t(&&)[4]);
+  void f(const char8_t(&&)[5]);
+#endif
+  void f(const char16_t(&&)[4]);
+  void f(const char16_t(&&)[5]);
+  void f(const char32_t(&&)[4]);
+  void f(const char32_t(&&)[5]);
+  void g() {
+    f({"abc"});
+    f({((("abc")))});
+    f({L"abc"});
+#if __cplusplus >= 202002L
+    f({u8"abc"});
+#endif
+    f({uR"(abc)"});
+    f({(UR"(abc)")});
+  }
+  } // namespace StringLiterals
+#endif
 } // dr1467
 
 namespace dr1490 {  // dr1490: 3.7 c++11
   // List-initialization from a string literal
 
   char s[4]{"abc"};                   // Ok
   std::initializer_list<char>{"abc"}; // expected-error {{expected unqualified-id}}}
 } // dr190

clang/test/SemaCXX/overload-array-size.cpp

+// RUN: %clang_cc1 %s -ast-dump | FileCheck %s
+
+// When the array size is 4 the call will attempt to bind an lvalue to an
+// rvalue and fail. Therfore #2 will be called. (rsmith will bring this issue
+// to CWG)
+void f(const char(&&)[4]); // #1
+void f(const char(&&)[5]); // #2
+void g() {
+  f({"abc"});
+  // CHECK: ExprWithCleanups {{.*}} <line:[[@LINE-1]]:{{.*}}> 'void'
+  // CHECK-NEXT: CallExpr
+  // CHECK-NEXT: ImplicitCastExpr {{.*}} 'void (*)(const char (&&)[5])'
+}

clang/test/SemaObjCXX/overload.mm

void test(NSDictionary *d1, NSDictionary<A *, A *> *d2, NSMutableDictionary<A *, A *> *m1) {
   overload(d1);
   overload(d2); // no warning
   overload(m1); // expected-warning {{'overload' is deprecated}}
   overload2(d2); // no warning
   overload2(m1); // expected-error {{call to 'overload2' is ambiguous}}
 }
 
 }
+
+namespace StringLiterals {
+void f(const char(&&)[5]);
+void f(const wchar_t(&&)[5]);
+void f(const char16_t(&&)[5]);
+void f(const char32_t(&&)[5]);
+void g() {
+  f({"abc"});
+  f({(((@encode(int))))});
+  f({L"abc"});
+  f({uR"(abc)"});
+  f({(UR"(abc)")});
+}
+} // namespace StringLiterals