Make wide multi-character character literals ill-formed

[cor3ntin]

Summary

This implements P2362, which has not het been approved by the
C++ committee, but because wide-multi character literals are
implementation defined, clang might not have to wait for WG21.

While I think this change could be applied to all languages - notably
C, I do not know whether there would be breakage from it.

The other part of P2362, making non-representable character
literals ill-formed, is already implemented by clang

[cor3ntin]

Rebase

[aaron.ballman]

I think that C and C++ should behave the same here; at least, I don't see any reason why they should have different capabilities.

The paper said that there is no expected code breakage from this change, but have you tried building a diverse corpus of code (like a distro's worth of packages) under this patch to see if anything actually breaks in practice? (I don't expect breakage that isn't identifying an actual issue in the code, but having some verification would be appreciated.) This would also help to identify whether the change is appropriate for C as well.

clang/test/CodeGen/string-literal-short-wstring.c

1–5

No need to tell the file to be in C mode; it is by virtue of the extension.

19

I'd feel most comfortable if the top of the file did:

typedef __WCHAR_TYPE__ wchar_t;

as that's the definition of wchar_t from the stddef.h provided by Clang.

[cor3ntin]

In D106215#2943611, @aaron.ballman wrote:

I think that C and C++ should behave the same here; at least, I don't see any reason why they should have different capabilities.

I agree but as WG14 hasn't weighted in I didn't want to make that call.
What do you think?

The paper said that there is no expected code breakage from this change, but have you tried building a diverse corpus of code (like a distro's worth of packages) under this patch to see if anything actually breaks in practice? (I don't expect breakage that isn't identifying an actual issue in the code, but having some verification would be appreciated.) This would also help to identify whether the change is appropriate for C as well.

We have done regexes over various repositories (every vcpkg package) with no match. Not running a complete compiler

[aaron.ballman]

In D106215#2943631, @cor3ntin wrote:

In D106215#2943611, @aaron.ballman wrote:

I think that C and C++ should behave the same here; at least, I don't see any reason why they should have different capabilities.

I agree but as WG14 hasn't weighted in I didn't want to make that call.
What do you think?

My reading of C2x is that this is implementation-defined there as well.

6.4.4.4p13:

A wide character constant prefixed by the letter L has type wchar_t, an integer type defined in the
<stddef.h> header; a wide character constant prefixed by the letter u or U has type char16_t or
char32_t, respectively, unsigned integer types defined in the <uchar.h> header. The value of a
wide character constant containing a single multibyte character that maps to a single member of the
extended execution character set is the wide character corresponding to that multibyte character,
as defined by the mbtowc, mbrtoc16, or mbrtoc32 function as appropriate for its type, with an
implementation-defined current locale. The value of a wide character constant containing more
than one multibyte character or a single multibyte character that maps to multiple members of
the extended execution character set, or containing a multibyte character or escape sequence not
represented in the extended execution character set, is implementation-defined.

Do you agree?

The paper said that there is no expected code breakage from this change, but have you tried building a diverse corpus of code (like a distro's worth of packages) under this patch to see if anything actually breaks in practice? (I don't expect breakage that isn't identifying an actual issue in the code, but having some verification would be appreciated.) This would also help to identify whether the change is appropriate for C as well.

We have done regexes over various repositories (every vcpkg package) with no match. Not running a complete compiler

Regexes are a good start but they miss the goofy (and sometimes awful) stuff that people do with token pasting, line continuations, and other random tricks. Would you be willing to try this as an experiment, or am I asking too much? :-) My thinking is that if we don't see any breakage from compiling a diverse corpus of code, we've done enough due diligence to suggest this is safe for both C and C++, but if we see some breakage, we can either identify that there's some valid use for this that we've not considered (less likely) and would be informative for both WG21 and WG14, or we can identify that we helped find bugs in real world code (more likely) which is also good feedback for the committees.

[cor3ntin]

Improve tests

[cor3ntin]

clang/test/CodeGen/string-literal-short-wstring.c

19

Thanks, I was looking for exactly that

[cor3ntin]

In D106215#2943653, @aaron.ballman wrote:

In D106215#2943631, @cor3ntin wrote:

In D106215#2943611, @aaron.ballman wrote:

I think that C and C++ should behave the same here; at least, I don't see any reason why they should have different capabilities.

I agree but as WG14 hasn't weighted in I didn't want to make that call.
What do you think?

My reading of C2x is that this is implementation-defined there as well.

6.4.4.4p13:

A wide character constant prefixed by the letter L has type wchar_t, an integer type defined in the
<stddef.h> header; a wide character constant prefixed by the letter u or U has type char16_t or
char32_t, respectively, unsigned integer types defined in the <uchar.h> header. The value of a
wide character constant containing a single multibyte character that maps to a single member of the
extended execution character set is the wide character corresponding to that multibyte character,
as defined by the mbtowc, mbrtoc16, or mbrtoc32 function as appropriate for its type, with an
implementation-defined current locale. The value of a wide character constant containing more
than one multibyte character or a single multibyte character that maps to multiple members of
the extended execution character set, or containing a multibyte character or escape sequence not
represented in the extended execution character set, is implementation-defined.

Do you agree?

Yes, I agree.
I think clang could make it ill-formed if it wanted to!
If we want to do that we could probably remove some more code :)

The paper said that there is no expected code breakage from this change, but have you tried building a diverse corpus of code (like a distro's worth of packages) under this patch to see if anything actually breaks in practice? (I don't expect breakage that isn't identifying an actual issue in the code, but having some verification would be appreciated.) This would also help to identify whether the change is appropriate for C as well.

We have done regexes over various repositories (every vcpkg package) with no match. Not running a complete compiler

Regexes are a good start but they miss the goofy (and sometimes awful) stuff that people do with token pasting, line continuations, and other random tricks. Would you be willing to try this as an experiment, or am I asking too much? :-) My thinking is that if we don't see any breakage from compiling a diverse corpus of code, we've done enough due diligence to suggest this is safe for both C and C++, but if we see some breakage, we can either identify that there's some valid use for this that we've not considered (less likely) and would be informative for both WG21 and WG14, or we can identify that we helped find bugs in real world code (more likely) which is also good feedback for the committees.

Unless there is a script to do that easily, I'm not sure I'll be able to get to it any time soon.
But really, there is 0 use for these things! And you can't do much goofiness L ## 'ab' certainly - but that wouldn't be very useful either

[aaron.ballman]

In D106215#2943675, @cor3ntin wrote:

In D106215#2943653, @aaron.ballman wrote:

In D106215#2943631, @cor3ntin wrote:

In D106215#2943611, @aaron.ballman wrote:

I think that C and C++ should behave the same here; at least, I don't see any reason why they should have different capabilities.

I agree but as WG14 hasn't weighted in I didn't want to make that call.
What do you think?

My reading of C2x is that this is implementation-defined there as well.

6.4.4.4p13:

A wide character constant prefixed by the letter L has type wchar_t, an integer type defined in the
<stddef.h> header; a wide character constant prefixed by the letter u or U has type char16_t or
char32_t, respectively, unsigned integer types defined in the <uchar.h> header. The value of a
wide character constant containing a single multibyte character that maps to a single member of the
extended execution character set is the wide character corresponding to that multibyte character,
as defined by the mbtowc, mbrtoc16, or mbrtoc32 function as appropriate for its type, with an
implementation-defined current locale. The value of a wide character constant containing more
than one multibyte character or a single multibyte character that maps to multiple members of
the extended execution character set, or containing a multibyte character or escape sequence not
represented in the extended execution character set, is implementation-defined.

Do you agree?

Yes, I agree.
I think clang could make it ill-formed if it wanted to!
If we want to do that we could probably remove some more code :)

I don't see a reason why we'd want C and C++ to diverge here for our implementation, so I'd say let's do C as well. @rsmith -- do you have any concerns with that?

The paper said that there is no expected code breakage from this change, but have you tried building a diverse corpus of code (like a distro's worth of packages) under this patch to see if anything actually breaks in practice? (I don't expect breakage that isn't identifying an actual issue in the code, but having some verification would be appreciated.) This would also help to identify whether the change is appropriate for C as well.

We have done regexes over various repositories (every vcpkg package) with no match. Not running a complete compiler

Regexes are a good start but they miss the goofy (and sometimes awful) stuff that people do with token pasting, line continuations, and other random tricks. Would you be willing to try this as an experiment, or am I asking too much? :-) My thinking is that if we don't see any breakage from compiling a diverse corpus of code, we've done enough due diligence to suggest this is safe for both C and C++, but if we see some breakage, we can either identify that there's some valid use for this that we've not considered (less likely) and would be informative for both WG21 and WG14, or we can identify that we helped find bugs in real world code (more likely) which is also good feedback for the committees.

Unless there is a script to do that easily, I'm not sure I'll be able to get to it any time soon.
But really, there is 0 use for these things! And you can't do much goofiness L ## 'ab' certainly - but that wouldn't be very useful either

Okay, I'm probably making too big of an ask here. I can't think of reasonable code that would be broken by this change. If that turns out to be incorrect, we can address it when we get a real world use case.

[cor3ntin]

Make wide multi char ill-formed regardless of language mode

[aaron.ballman]

I'm okay with the changes, but I think we should call this out in the release notes as there's technically the potential for this to be a breaking change (in both C and C++).

[cor3ntin]

Rebasing

[cor3ntin]

Describe this change in the changelog.

[aaron.ballman]

LGTM, but going to wait a bit to land it in case @rsmith has opinions.

[aaron.ballman]

Thanks! I've landed in bdeda959abd74c88a6cfc34b10c1b665cb45cb8d

clang/include/clang/Basic/DiagnosticLexKinds.td

 def warn_cxx11_compat_digit_separator : Warning<
   "digit separators are incompatible with C++ standards before C++14">,
   InGroup<CXXPre14Compat>, DefaultIgnore;
 def warn_c2x_compat_digit_separator : Warning<
   "digit separators are incompatible with C standards before C2x">,
   InGroup<CPre2xCompat>, DefaultIgnore;
 def err_digit_separator_not_between_digits : Error<
   "digit separator cannot appear at %select{start|end}0 of digit sequence">;
-def warn_extraneous_char_constant : Warning<
-  "extraneous characters in character constant ignored">;
 def warn_char_constant_too_large : Warning<
   "character constant too long for its type">;
-def err_multichar_utf_character_literal : Error<
-  "Unicode character literals may not contain multiple characters">;
+def err_multichar_character_literal : Error<
+  "%select{wide|Unicode}0 character literals may not contain multiple characters">;
 def err_exponent_has_no_digits : Error<"exponent has no digits">;
 def err_hex_constant_requires : Error<
   "hexadecimal floating %select{constant|literal}0 requires "
   "%select{an exponent|a significand}1">;
 def ext_hex_constant_invalid : Extension<
   "hexadecimal floating constants are a C99 feature">, InGroup<C99>;
 def ext_hex_literal_invalid : Extension<
   "hexadecimal floating literals are a C++17 feature">, InGroup<CXX17>;

clang/lib/Lex/LiteralSupport.cpp

uint64_t result =
                         FullSourceLoc(Loc,PP.getSourceManager()),
                         CharWidth, &PP.getDiagnostics(), PP.getLangOpts());
     *buffer_begin++ = result;
   }
 
   unsigned NumCharsSoFar = buffer_begin - &codepoint_buffer.front();
 
   if (NumCharsSoFar > 1) {
-    if (isWide())
-      PP.Diag(Loc, diag::warn_extraneous_char_constant);
-    else if (isAscii() && NumCharsSoFar == 4)
+    if (isAscii() && NumCharsSoFar == 4)
       PP.Diag(Loc, diag::warn_four_char_character_literal);
     else if (isAscii())
       PP.Diag(Loc, diag::warn_multichar_character_literal);
-    else
-      PP.Diag(Loc, diag::err_multichar_utf_character_literal);
+    else {
+      PP.Diag(Loc, diag::err_multichar_character_literal) << (isWide() ? 0 : 1);
+      HadError = true;
+    }
     IsMultiChar = true;
   } else {
     IsMultiChar = false;
   }
 
   llvm::APInt LitVal(PP.getTargetInfo().getIntWidth(), 0);
 
   // Narrow character literals act as though their value is concatenated

clang/test/CodeGen/char-literal.c

 // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck -check-prefix=CHECK-C %s
-// RUN: %clang_cc1 -x c++ -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck -check-prefix=CHECK-C %s
 // RUN: %clang_cc1 -x c++ -std=c++11 -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck -check-prefix=CHECK-CPP0X %s
 
 #include <stddef.h>
 
 int main() {
   // CHECK-C: store i8 97
   // CHECK-CPP0X: store i8 97
   char a = 'a';
int main() {
   // CHECK-C: store i32 1919512167
   // CHECK-CPP0X: store i32 1919512167
   int b3 = 'somesillylongstring';
 
   // CHECK-C: store i32 97
   // CHECK-CPP0X: store i32 97
   wchar_t wa = L'a';
 
-  // Should pick second character.
-  // CHECK-C: store i32 98
-  // CHECK-CPP0X: store i32 98
-  wchar_t wb = L'ab';
-
 #if __cplusplus >= 201103L
   // CHECK-CPP0X: store i16 97
   char16_t ua = u'a';
 
   // CHECK-CPP0X: store i32 97
   char32_t Ua = U'a';
 
   // CHECK-CPP0X: store i16 1047
#endif
   // CHECK-CPP0X: store i32 1110027
   wchar_t wd = L'\U0010F00B';
 
 #if __cplusplus >= 201103L
   // CHECK-CPP0X: store i32 1110027
   char32_t Ud = U'\U0010F00B';
 #endif
 
-  // Should pick second character.
-  // CHECK-C: store i32 1110027
-  // CHECK-CPP0X: store i32 1110027
-  wchar_t we = L'\u1234\U0010F00B';
 }

clang/test/CodeGen/string-literal-short-wstring.c

-// RUN: %clang_cc1 -x c++ -triple %itanium_abi_triple -emit-llvm -fwchar-type=short -fno-signed-wchar %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=ITANIUM
-// RUN: %clang_cc1 -x c++ -triple %ms_abi_triple -emit-llvm -fwchar-type=short -fno-signed-wchar %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=MSABI
-// Runs in c++ mode so that wchar_t is available.
+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm -fwchar-type=short -fno-signed-wchar %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=ITANIUM
+// RUN: %clang_cc1 -triple %ms_abi_triple -emit-llvm -fwchar-type=short -fno-signed-wchar %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=MSABI
+
+// Run in C mode as wide multichar literals are not valid in C++
 
 // XFAIL: hexagon
 // Hexagon aligns arrays of size 8+ bytes to a 64-bit boundary, which fails
 // the first check line with "align 1".
 
+typedef __WCHAR_TYPE__ wchar_t;
+
 int main() {
   // This should convert to utf8.
   // CHECK: private unnamed_addr constant [10 x i8] c"\E1\84\A0\C8\A0\F4\82\80\B0\00", align 1
   char b[10] = "\u1120\u0220\U00102030";
 
   // ITANIUM: private unnamed_addr constant [3 x i16] [i16 65, i16 66, i16 0]
   // MSABI: linkonce_odr dso_local unnamed_addr constant [3 x i16] [i16 65, i16 66, i16 0]
   const wchar_t *foo = L"AB";
 
   // This should convert to utf16.
   // ITANIUM: private unnamed_addr constant [5 x i16] [i16 4384, i16 544, i16 -9272, i16 -9168, i16 0]
   // MSABI: linkonce_odr dso_local unnamed_addr constant [5 x i16] [i16 4384, i16 544, i16 -9272, i16 -9168, i16 0]
   const wchar_t *bar = L"\u1120\u0220\U00102030";
 
-
-
   // Should pick second character.
   // CHECK: store i8 98
   char c = 'ab';
 
   // CHECK: store i16 97
   wchar_t wa = L'a';
 
-  // Should pick second character.
-  // CHECK: store i16 98
-  wchar_t wb = L'ab';
-
   // -4085 == 0xf00b
   // CHECK: store i16 -4085
   wchar_t wc = L'\uF00B';
 }

clang/test/Lexer/char-literal.cpp

 
 #ifdef __cplusplus
 auto f = '\xE2\x8C\x98'; // expected-warning {{multi-character character constant}}
 #endif
 
 char16_t g = u'ab'; // expected-error {{Unicode character literals may not contain multiple characters}}
 char16_t h = u'\U0010FFFD'; // expected-error {{character too large for enclosing character literal type}}
 
-wchar_t i = L'ab'; // expected-warning {{extraneous characters in character constant ignored}}
+wchar_t i = L'ab'; // expected-error {{wide character literals may not contain multiple characters}}
+
 wchar_t j = L'\U0010FFFD';
 
 char32_t k = U'\U0010FFFD';
 
 char l = 'Ø'; // expected-error {{character too large for enclosing character literal type}}
 char m = '👿'; // expected-error {{character too large for enclosing character literal type}}
 
 char32_t n = U'ab'; // expected-error {{Unicode character literals may not contain multiple characters}}
 char16_t o = '👽'; // expected-error {{character too large for enclosing character literal type}}
 
 char16_t p[2] = u"\U0000FFFF";
 char16_t q[2] = u"\U00010000";
 #ifdef __cplusplus
 // expected-error@-2 {{too long}}
 #endif

clang/test/Lexer/wchar.c

 // RUN: %clang_cc1 -fsyntax-only -fwchar-type=short -fno-signed-wchar -verify %s
 
 void f() {
   (void)L"\U00010000"; // unicode escape produces UTF-16 sequence, so no warning
 
-  (void)L'\U00010000'; // expected-error {{character too large for enclosing character literal type}}
+  (void)L'ab';  // expected-error {{wide character literals may not contain multiple characters}}
 
-  (void)L'ab';  // expected-warning {{extraneous characters in character constant ignored}}
-
-  (void)L'a\u1000';  // expected-warning {{extraneous characters in character constant ignored}}
+  (void)L'a\u1000';  // expected-error {{wide character literals may not contain multiple characters}}
 }
 

clang/test/Misc/warning-flags.c

(2) It prevents us adding new warnings to Clang that have no -W flag.  All
     new warnings should have -W flags.
 
     If you add a new warning without a flag, this test will fail.  To fix
     this test, simply add a warning group to that warning.
 
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (68):
+CHECK: Warnings without flags (67):
 
 CHECK-NEXT:   ext_expected_semi_decl_list
 CHECK-NEXT:   ext_explicit_specialization_storage_class
 CHECK-NEXT:   ext_missing_declspec
 CHECK-NEXT:   ext_missing_whitespace_after_macro_name
 CHECK-NEXT:   ext_new_paren_array_nonconst
 CHECK-NEXT:   ext_plain_complex
 CHECK-NEXT:   ext_template_arg_extra_parens

 CHECK-NEXT:   warn_delete_array_type
 CHECK-NEXT:   warn_double_const_requires_fp64
 CHECK-NEXT:   warn_drv_assuming_mfloat_abi_is
 CHECK-NEXT:   warn_drv_clang_unsupported
 CHECK-NEXT:   warn_drv_pch_not_first_include
 CHECK-NEXT:   warn_dup_category_def
 CHECK-NEXT:   warn_enum_value_overflow
 CHECK-NEXT:   warn_expected_qualified_after_typename
-CHECK-NEXT:   warn_extraneous_char_constant
 CHECK-NEXT:   warn_fe_backend_unsupported
 CHECK-NEXT:   warn_fe_cc_log_diagnostics_failure
 CHECK-NEXT:   warn_fe_cc_print_header_failure
 CHECK-NEXT:   warn_fe_macro_contains_embedded_newline
 CHECK-NEXT:   warn_ignoring_ftabstop_value
 CHECK-NEXT:   warn_implements_nscopying
 CHECK-NEXT:   warn_incompatible_qualified_id
 CHECK-NEXT:   warn_invalid_asm_cast_lvalue

clang/test/Preprocessor/Weverything_pragma.c

 // RUN: %clang_cc1 -Weverything   -fsyntax-only -verify %s
 
 // Test that the pragma overrides command line option -Weverythings,
 
 // a diagnostic with DefaultIgnore. This is part of a group 'unused-macro'
 // but -Weverything forces it
 #define UNUSED_MACRO1 1 // expected-warning{{macro is not used}}
 
 void foo(void) // expected-warning {{no previous prototype for function}}
 // expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
 {
  // A diagnostic without DefaultIgnore, and not part of a group.
- (void) L'ab'; // expected-warning {{extraneous characters in character constant ignored}}
+ (void) 'ab'; // expected-warning {{multi-character character constant}}
 
 #pragma clang diagnostic warning "-Weverything" // Should not change anyhting.
 #define UNUSED_MACRO2 1 // expected-warning{{macro is not used}}
- (void) L'cd'; // expected-warning {{extraneous characters in character constant ignored}}
+ (void) 'cd'; // expected-warning {{multi-character character constant}}
 
 #pragma clang diagnostic ignored "-Weverything" // Ignore warnings now.
 #define UNUSED_MACRO2 1 // no warning
- (void) L'ef'; // no warning here
+ (void) 'ef'; // no warning here
 
 #pragma clang diagnostic warning "-Weverything" // Revert back to warnings.
 #define UNUSED_MACRO3 1 // expected-warning{{macro is not used}}
- (void) L'gh'; // expected-warning {{extraneous characters in character constant ignored}}
+ (void) 'gh'; // expected-warning {{multi-character character constant}}
 
 #pragma clang diagnostic error "-Weverything"  // Give errors now.
 #define UNUSED_MACRO4 1 // expected-error{{macro is not used}}
- (void) L'ij'; // expected-error {{extraneous characters in character constant ignored}}
+ (void) 'ij'; // expected-error {{multi-character character constant}}
 }