[clang] Report narrowing conversions with const references #75332

Fznamznon · 2023-12-13T12:45:16Z

Fixes #63151

Fixes llvm#63151

llvmbot · 2023-12-13T12:45:46Z

@llvm/pr-subscribers-clang

Author: Mariya Podchishchaeva (Fznamznon)

Changes

Fixes #63151

Full diff: https://github.com/llvm/llvm-project/pull/75332.diff

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+3)
(modified) clang/lib/Sema/SemaInit.cpp (+18-12)
(added) clang/test/SemaCXX/GH63151.cpp (+12)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 05d59d0da264f3..2aba1740a6b610 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -521,6 +521,9 @@ Improvements to Clang's diagnostics
           |               ~~~~~~~^~~~~
 
 - Clang now diagnoses definitions of friend function specializations, e.g. ``friend void f<>(int) {}``.
+- Clang now diagnoses narrowing conversions involving const references.
+  (`#63151: <https://github.com/llvm/llvm-project/issues/63151>`_).
+
 
 Improvements to Clang's time-trace
 ----------------------------------
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 4028b2d642b212..7ff1b55d1fcba0 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -4432,7 +4432,8 @@ static void TryReferenceInitializationCore(Sema &S,
                                            Qualifiers T1Quals,
                                            QualType cv2T2, QualType T2,
                                            Qualifiers T2Quals,
-                                           InitializationSequence &Sequence);
+                                           InitializationSequence &Sequence,
+                                           bool TopLevelOfInitList);
 
 static void TryValueInitialization(Sema &S,
                                    const InitializedEntity &Entity,
@@ -4486,7 +4487,8 @@ static void TryReferenceListInitialization(Sema &S,
     if (RefRelationship >= Sema::Ref_Related) {
       // Try to bind the reference here.
       TryReferenceInitializationCore(S, Entity, Kind, Initializer, cv1T1, T1,
-                                     T1Quals, cv2T2, T2, T2Quals, Sequence);
+                                     T1Quals, cv2T2, T2, T2Quals, Sequence,
+                                     true);
       if (Sequence)
         Sequence.RewrapReferenceInitList(cv1T1, InitList);
       return;
@@ -4945,11 +4947,11 @@ static void CheckCXX98CompatAccessibleCopy(Sema &S,
                                            Expr *CurInitExpr);
 
 /// Attempt reference initialization (C++0x [dcl.init.ref])
-static void TryReferenceInitialization(Sema &S,
-                                       const InitializedEntity &Entity,
+static void TryReferenceInitialization(Sema &S, const InitializedEntity &Entity,
                                        const InitializationKind &Kind,
                                        Expr *Initializer,
-                                       InitializationSequence &Sequence) {
+                                       InitializationSequence &Sequence,
+                                       bool TopLevelOfInitList) {
   QualType DestType = Entity.getType();
   QualType cv1T1 = DestType->castAs<ReferenceType>()->getPointeeType();
   Qualifiers T1Quals;
@@ -4967,7 +4969,8 @@ static void TryReferenceInitialization(Sema &S,
 
   // Delegate everything else to a subfunction.
   TryReferenceInitializationCore(S, Entity, Kind, Initializer, cv1T1, T1,
-                                 T1Quals, cv2T2, T2, T2Quals, Sequence);
+                                 T1Quals, cv2T2, T2, T2Quals, Sequence,
+                                 TopLevelOfInitList);
 }
 
 /// Determine whether an expression is a non-referenceable glvalue (one to
@@ -4990,7 +4993,8 @@ static void TryReferenceInitializationCore(Sema &S,
                                            Qualifiers T1Quals,
                                            QualType cv2T2, QualType T2,
                                            Qualifiers T2Quals,
-                                           InitializationSequence &Sequence) {
+                                           InitializationSequence &Sequence,
+                                           bool TopLevelOfInitList) {
   QualType DestType = Entity.getType();
   SourceLocation DeclLoc = Initializer->getBeginLoc();
 
@@ -5264,7 +5268,8 @@ static void TryReferenceInitializationCore(Sema &S,
       Sequence.SetFailed(InitializationSequence::FK_ReferenceInitFailed);
     return;
   } else {
-    Sequence.AddConversionSequenceStep(ICS, TempEntity.getType());
+    Sequence.AddConversionSequenceStep(ICS, TempEntity.getType(),
+                                       TopLevelOfInitList);
   }
 
   //        [...] If T1 is reference-related to T2, cv1 must be the
@@ -6228,7 +6233,8 @@ void InitializationSequence::InitializeFrom(Sema &S,
     else if (isa<InitListExpr>(Args[0]))
       SetFailed(FK_ParenthesizedListInitForReference);
     else
-      TryReferenceInitialization(S, Entity, Kind, Args[0], *this);
+      TryReferenceInitialization(S, Entity, Kind, Args[0], *this,
+                                 TopLevelOfInitList);
     return;
   }
 
@@ -10431,7 +10437,7 @@ static void DiagnoseNarrowingInInitList(Sema &S,
                                         : diag::warn_init_list_type_narrowing)
         << PostInit->getSourceRange()
         << PreNarrowingType.getLocalUnqualifiedType()
-        << EntityType.getLocalUnqualifiedType();
+        << EntityType.getLocalUnqualifiedType().getNonReferenceType();
     break;
 
   case NK_Constant_Narrowing:
@@ -10442,7 +10448,7 @@ static void DiagnoseNarrowingInInitList(Sema &S,
                : diag::warn_init_list_constant_narrowing)
         << PostInit->getSourceRange()
         << ConstantValue.getAsString(S.getASTContext(), ConstantType)
-        << EntityType.getLocalUnqualifiedType();
+        << EntityType.getLocalUnqualifiedType().getNonReferenceType();
     break;
 
   case NK_Variable_Narrowing:
@@ -10453,7 +10459,7 @@ static void DiagnoseNarrowingInInitList(Sema &S,
                : diag::warn_init_list_variable_narrowing)
         << PostInit->getSourceRange()
         << PreNarrowingType.getLocalUnqualifiedType()
-        << EntityType.getLocalUnqualifiedType();
+        << EntityType.getLocalUnqualifiedType().getNonReferenceType();
     break;
   }
 
diff --git a/clang/test/SemaCXX/GH63151.cpp b/clang/test/SemaCXX/GH63151.cpp
new file mode 100644
index 00000000000000..3d5c3cc84a56db
--- /dev/null
+++ b/clang/test/SemaCXX/GH63151.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+
+struct A { A(const unsigned &x) {} };
+
+void foo(int p) {
+  A a { -1 }; // expected-error {{constant expression evaluates to -1 which cannot be narrowed to type 'const unsigned int'}}
+  A b { 0 };
+  A c { p }; // expected-error {{non-constant-expression cannot be narrowed from type 'int' to 'const unsigned int' in initializer list}}
+  A d { 0.5 }; // expected-error {{type 'double' cannot be narrowed to 'const unsigned int' in initializer list}}
+               // expected-warning@-1 {{implicit conversion from 'double' to 'unsigned int' changes value from 0.5 to 0}}
+}

github-actions · 2023-12-13T12:47:51Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff e418988175c2dee9d7c7976cf822b41aaf321c26 29eede77f5afbab726bf61f6cf93800d162a648a -- clang/test/SemaCXX/GH63151.cpp clang/lib/Sema/SemaInit.cpp

View the diff from clang-format here.

diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index de0d92edb5..aa3616e63a 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -4424,16 +4424,11 @@ ResolveOverloadedFunctionForReferenceBinding(Sema &S,
   return false;
 }
 
-static void TryReferenceInitializationCore(Sema &S,
-                                           const InitializedEntity &Entity,
-                                           const InitializationKind &Kind,
-                                           Expr *Initializer,
-                                           QualType cv1T1, QualType T1,
-                                           Qualifiers T1Quals,
-                                           QualType cv2T2, QualType T2,
-                                           Qualifiers T2Quals,
-                                           InitializationSequence &Sequence,
-                                           bool TopLevelOfInitList);
+static void TryReferenceInitializationCore(
+    Sema &S, const InitializedEntity &Entity, const InitializationKind &Kind,
+    Expr *Initializer, QualType cv1T1, QualType T1, Qualifiers T1Quals,
+    QualType cv2T2, QualType T2, Qualifiers T2Quals,
+    InitializationSequence &Sequence, bool TopLevelOfInitList);
 
 static void TryValueInitialization(Sema &S,
                                    const InitializedEntity &Entity,
@@ -4985,16 +4980,11 @@ static bool isNonReferenceableGLValue(Expr *E) {
 ///
 /// We also can get here in C if we call a builtin which is declared as
 /// a function with a parameter of reference type (such as __builtin_va_end()).
-static void TryReferenceInitializationCore(Sema &S,
-                                           const InitializedEntity &Entity,
-                                           const InitializationKind &Kind,
-                                           Expr *Initializer,
-                                           QualType cv1T1, QualType T1,
-                                           Qualifiers T1Quals,
-                                           QualType cv2T2, QualType T2,
-                                           Qualifiers T2Quals,
-                                           InitializationSequence &Sequence,
-                                           bool TopLevelOfInitList) {
+static void TryReferenceInitializationCore(
+    Sema &S, const InitializedEntity &Entity, const InitializationKind &Kind,
+    Expr *Initializer, QualType cv1T1, QualType T1, Qualifiers T1Quals,
+    QualType cv2T2, QualType T2, Qualifiers T2Quals,
+    InitializationSequence &Sequence, bool TopLevelOfInitList) {
   QualType DestType = Entity.getType();
   SourceLocation DeclLoc = Initializer->getBeginLoc();

Fznamznon · 2023-12-13T12:49:36Z

Formatting trouble is intentional.

erichkeane

1 nit, otherwise LGTM

clang/lib/Sema/SemaInit.cpp

Co-authored-by: Erich Keane <[email protected]>

erichkeane · 2023-12-13T20:50:11Z

clang/lib/Sema/SemaInit.cpp

@@ -10431,7 +10437,7 @@ static void DiagnoseNarrowingInInitList(Sema &S,
                                        : diag::warn_init_list_type_narrowing)
        << PostInit->getSourceRange()
        << PreNarrowingType.getLocalUnqualifiedType()
-        << EntityType.getLocalUnqualifiedType();
+        << EntityType.getLocalUnqualifiedType().getNonReferenceType();


Since this is supposed to remove local qualifiers, should the reference be removed first? What happens with a const int & as is?

ALSO, does the pre-narrowing-type also need to have references removed?

Since this is supposed to remove local qualifiers, should the reference be removed first? What happens with a const int & as is?

Perhaps makes sense. I only removed reference since it is already removed from the pre-narrowing-type and I was a bit inspired by gcc messages https://godbolt.org/z/Tcv17Wxhc . I could leave reference type it as is, then the messages will look like:

struct A { A(const unsigned &x) {} }; int foo(const int &aba) { A a { -1 }; // constant expression evaluates to -1 which cannot be narrowed to type 'const unsigned int&' A b { aba }; // non-constant-expression cannot be narrowed from type 'int' to 'const unsigned int&' in initializer list return 0; }

Why does leaving the reference type alone (which seems to be removed 'last' cause the const to stick around, and not the reference?

My guess is getLocalUnqualifiedType doesn't strip cv qualifiers from reference's pointee type, so we first should remove reference and then qualifiers to see plain int in the diagnostic.

I think we're talking past eachother then. I was suggesting to remove the reference first, which would have us match GCC's behavior, right? I think that is better than leaving it 'as is', but I was confused as to what is going on with the change on 10440 (removing the reference AFTER the qualifiers instead of before);

erichkeane

Looks like you got what I meant anyway, and we WERE just passionately agreeing with eachother :) LGTM.

Fznamznon · 2023-12-14T14:19:06Z

Looks like you got what I meant anyway, and we WERE just passionately agreeing with eachother :) LGTM.

Thanks!
Probably I was being a bit slow, lol

alexfh · 2023-12-20T16:12:39Z

The change in the diagnostic happens to make it much noisier than it was before. Our code was clean w.r.t. -Wc++11-narrowing, but now we see hundreds if not thousands of compilation breakages (we use -Werror, which turns out to be the only robust way to avoid backsliding). I think, the new behavior (though useful) needs to be guarded by a separate warning flag.

llvm#75332 diagnosed narrowing involving const reference. Our depot has hundreds if not thousands of breakages (llvm#75332 (comment)). Add a subgroup of -Wc++11-narrowing to help users gradually fix their issues without regressing the existing -Wc++11-narrowing diagnostics.

This patch llvm/llvm-project#75332 would report narrowing conversions with const references, which could lead to "error: constant expression evaluates to 2 which cannot be narrowed to type 'bool'".

llvm/llvm-project#75332 diagnoses narrowing conversions involving const references

#75332 diagnosed narrowing involving const reference. Our depot has hundreds if not thousands of breakages (#75332 (comment)). Add a subgroup of -Wc++11-narrowing to help users gradually fix their issues without regressing the existing -Wc++11-narrowing diagnostics.

llvm/llvm-project#75332 diagnoses narrowing conversions involving const references Also fix warning in 211 ../sycl/test-e2e/Basic/swizzle_op.cpp:211:24: warning: equality comparison with extraneous parentheses [-Wparentheses-equality]

erichkeane · 2024-01-03T15:01:54Z

The change in the diagnostic happens to make it much noisier than it was before. Our code was clean w.r.t. -Wc++11-narrowing, but now we see hundreds if not thousands of compilation breakages (we use -Werror, which turns out to be the only robust way to avoid backsliding). I think, the new behavior (though useful) needs to be guarded by a separate warning flag.

Looks like that was done here; #76094

…ttest Clang's diagnostics for narrowing casts should now catch more cases as of llvm/llvm-project#75332. Toolchain CI found this case, which should be addressed by adding a static cast to the unsigned type. Reported in https://issues.fuchsia.dev/issues/316594875. Fixed: 316594875 Change-Id: I1ac5118c84534532341c12abab0020bd206f2d69 Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/961451 Reviewed-by: Alice Neels <[email protected]> Commit-Queue: Auto-Submit <[email protected]> Fuchsia-Auto-Submit: Paul Kirth <[email protected]>

[clang] Report narrowing conversions with const references

d0a7276

Fixes llvm#63151

Fznamznon requested review from cor3ntin, shafik and erichkeane December 13, 2023 12:45

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 13, 2023

erichkeane approved these changes Dec 13, 2023

View reviewed changes

clang/lib/Sema/SemaInit.cpp Outdated Show resolved Hide resolved

Update clang/lib/Sema/SemaInit.cpp

3d6d16a

Co-authored-by: Erich Keane <[email protected]>

erichkeane reviewed Dec 13, 2023

View reviewed changes

Swap getLocalUnqualifiedType and getNonReferenceType

29eede7

erichkeane approved these changes Dec 14, 2023

View reviewed changes

Fznamznon merged commit cd09f21 into llvm:main Dec 15, 2023

XinWang10 mentioned this pull request Dec 18, 2023

Avoid sema error in test case intel/llvm#12197

Merged

MaskRay mentioned this pull request Dec 20, 2023

[Sema] Add -Wc++11-narrowing-const-reference #76094

Merged

jsji pushed a commit to intel/llvm that referenced this pull request Dec 21, 2023

[SYCL][E2E] Fix narrowing conversions warning

06fb99b

llvm/llvm-project#75332 diagnoses narrowing conversions involving const references

jsji mentioned this pull request Dec 21, 2023

LLVM and SPIRV-LLVM-Translator pulldown (WW51) intel/llvm#12226

Merged

frederick-vs-ja mentioned this pull request Jan 3, 2025

Narrowing conversion from double to const int& wrongly permitted within braces #53351

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang] Report narrowing conversions with const references #75332

[clang] Report narrowing conversions with const references #75332

Uh oh!

Fznamznon commented Dec 13, 2023

Uh oh!

llvmbot commented Dec 13, 2023

Uh oh!

github-actions bot commented Dec 13, 2023 •

edited

Loading

Uh oh!

Fznamznon commented Dec 13, 2023

Uh oh!

erichkeane left a comment

Uh oh!

Uh oh!

erichkeane Dec 13, 2023

Uh oh!

Fznamznon Dec 14, 2023 •

edited

Loading

Uh oh!

erichkeane Dec 14, 2023

Uh oh!

Fznamznon Dec 14, 2023

Uh oh!

erichkeane Dec 14, 2023

Uh oh!

erichkeane left a comment

Uh oh!

Fznamznon commented Dec 14, 2023

Uh oh!

alexfh commented Dec 20, 2023

Uh oh!

erichkeane commented Jan 3, 2024

Uh oh!

Uh oh!

[clang] Report narrowing conversions with const references #75332

[clang] Report narrowing conversions with const references #75332

Uh oh!

Conversation

Fznamznon commented Dec 13, 2023

Uh oh!

llvmbot commented Dec 13, 2023

Uh oh!

github-actions bot commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fznamznon commented Dec 13, 2023

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erichkeane Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

Fznamznon Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichkeane Dec 14, 2023

Choose a reason for hiding this comment

Uh oh!

Fznamznon Dec 14, 2023

Choose a reason for hiding this comment

Uh oh!

erichkeane Dec 14, 2023

Choose a reason for hiding this comment

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Fznamznon commented Dec 14, 2023

Uh oh!

alexfh commented Dec 20, 2023

Uh oh!

erichkeane commented Jan 3, 2024

Uh oh!

Uh oh!

github-actions bot commented Dec 13, 2023 •

edited

Loading

Fznamznon Dec 14, 2023 •

edited

Loading