[clang] Fix implicit integer conversion for opaque enums declared in class templates #121039
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (thebrandre) ChangesThis commit fixes implicit integer conversions for opaque-enum-declarations inside a class template with a specified enum-base (see [dcl.enum]). Previously, the promotion type of the instantiated enum was set only in Full diff: https://github.com/llvm/llvm-project/pull/121039.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index e058afe81da589..4d39fd409795b6 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1620,6 +1620,9 @@ Decl *TemplateDeclInstantiator::VisitEnumDecl(EnumDecl *D) {
if (isDeclWithinFunction(D) ? D == Def : Def && !Enum->isScoped()) {
SemaRef.CurrentInstantiationScope->InstantiatedLocal(D, Enum);
InstantiateEnumDefinition(Enum, Def);
+ } else {
+ if (D->isFixed() && !Def)
+ Enum->setPromotionType(Enum->getIntegerType());
}
return Enum;
diff --git a/clang/test/SemaCXX/enum-base-in-class-template.cpp b/clang/test/SemaCXX/enum-base-in-class-template.cpp
new file mode 100644
index 00000000000000..012bef9785cbaf
--- /dev/null
+++ b/clang/test/SemaCXX/enum-base-in-class-template.cpp
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only %s -verify
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only %s -verify
+
+// This program causes clang 19 and earlier to crash because
+// EnumDecl::PromotionType has not been set on the instantiated enum.
+// See GitHub Issue #117960.
+namespace Issue117960 {
+template <typename T>
+struct A {
+ enum E : T;
+};
+
+int b = A<int>::E{} + 0;
+}
+
+
+namespace test {
+template <typename T1, typename T2>
+struct IsSame {
+ static constexpr bool check() { return false; }
+};
+
+template <typename T>
+struct IsSame<T, T> {
+ static constexpr bool check() { return true; }
+};
+} // namespace test
+
+
+template <typename T>
+struct S1 {
+ enum E : T;
+};
+// checks if EnumDecl::PromotionType is set
+int X1 = S1<int>::E{} + 0;
+int Y1 = S1<unsigned>::E{} + 0;
+static_assert(test::IsSame<decltype(S1<int>::E{}+0), int>::check(), "");
+static_assert(test::IsSame<decltype(S1<unsigned>::E{}+0), unsigned>::check(), "");
+char Z1 = S1<unsigned>::E(-1) + 0; // expected-warning{{implicit conversion from 'unsigned int' to 'char'}}
+
+template <typename Traits>
+struct S2 {
+ enum E : typename Traits::IntegerType;
+};
+
+template <typename T>
+struct Traits {
+ typedef T IntegerType;
+};
+
+int X2 = S2<Traits<int>>::E{} + 0;
+int Y2 = S2<Traits<unsigned>>::E{} + 0;
+static_assert(test::IsSame<decltype(S2<Traits<int>>::E{}+0), int>::check(), "");
+static_assert(test::IsSame<decltype(S2<Traits<unsigned>>::E{}+0), unsigned>::check(), "");
+
+
+template <typename T>
+struct S3 {
+ enum E : unsigned;
+};
+
+int X3 = S3<float>::E{} + 0;
+
+// fails in clang 19 and earlier (see the discussion on GitHub Issue #117960):
+static_assert(test::IsSame<decltype(S3<float>::E{}+0), unsigned>::check(), "");
+
|
|
Thanks for the PR. Can you update the commit message to be more descriptive? I'm not sure the fix is sufficient. Maybe we should instead
That would not only fix the crash but also the fact that we are seemingly missing a lot of diagnostics |
|
Thanks for the feedback @cor3ntin! 🙂 I will change the title of the PR and of the commit message to:
adopting the terminology from [dcl.enum] if that isn't too verbose? The other potential issues you mentioned should actually conform to the specification in [temp.spec.general] if I understand it correctly. My changes should not affect the behavior if In any case, I'll try if I can find any other related scenarios where the fix isn't sufficient! |
thebrandre
changed the title
thebrandre
force-pushed
the
fix-issue-117960
branch
from
38eea8d to
551290d
Compare
|
I added regression tests for the scoped enumeration types to make sure that these are not affected. |
thebrandre
force-pushed
the
fix-issue-117960
branch
from
551290d to
7db274f
Compare
thebrandre
changed the title
|
@cor3ntin Good news! I figured it out on my own! And I now have a fix that I am confident in. 🙂 The attributes have already been handled correctly previously. But to make sure of it, I also added tests. Putting the few lines for the fix there, seems fairly consistent to me. It repeats the logic in As you suggested, I stepped through the code to check if anything else from Thanks again for your feedback @cor3ntin - it helped me to get a much cleaner solution and a better understanding of the issue even though my initial analysis might have been a little confusing ... I rebased my branch on the current main and also "rebased away" my prior fix to make the diff more concise. P.S.: Of course I also ran all the regression tests on my machine. And I didn't break any. 😉 |
@erichkeane wdyt? |
|
@cor3ntin I hope there is no misunderstanding here. If so, I apologize, I strongly agree with you that some refactoring should be applied because, as I mentioned, the diagnostics get duplicated in certain cases as shown here on Compiler Explorer: https://godbolt.org/z/473Eezhjn. If there is still something wrong with the commit message, I would gladly give it another try if you could be more specific. |
I agree, it seems that the instantiation of the enum is missing quite a bit when it comes to diagnostics. We should definitely do some sort of analysis to ensure that we're getting the same diagnostics at both points. That said, I am ok with doing this NOW, as long as the author promises to do the rest in a followup patch ASAP. |
|
@erichkeane I am not sure what you mean. As I pointed out earlier, the examples @cor3ntin brought up actually conform to the standard as in [temp.spec.general]:
The issues are correctly diagnosed by clang if the struct is explicitly instantiated as demonstrated here in Compiler Explorer: https://godbolt.org/z/GzYq9jzjo. Nevertheless, I investigated further and managed to discover some real bugs with diagnostics on template instantiation. I plan to keep working on it but I can't promise much given the facts that I only have the weekends to spend time on it and my limited familiarity with the codebase (this would be my very first commit to LLVM). But, of course, I promise to try my best! 🙂 |
My quick look through the ActOnEnum and this function looked like it showed some misses. If Corentin is comfortable with our checking here, then I'm happy too.
Thats great, thanks for finding that and working on it! Those look like some pretty unfortunate misses.
Definitely appreciate it! I'm happy when Corentin is here, I have no concerns. |
|
We usually try to diagnose issues as soon as possible, so i still believe there are improvements to be made here. That being said, I'm happy to approve the PR as it is now. That being said, it is missing a changelog entry in |
|
The issue number here seems wrong, can you update the description? |
…ass templates Repeat the steps in `Sema::ActOnTag` for the promotion type after type substitution of the underlying type. This is required in the case of an *opaque-enum-declaration* (see [dcl.enum]). If, instead, a full *enum-specifier* (subsequent curly braces) is provided, `Sema::ActOnEnumBody` is re-invoked on template instantiation overriding the promotion type and hiding the issue. Note that, in contrast to `Sema::ActOnEnumBody`, `Sema::ActOnTag` is *not* called again for the instantiated enumeration type. Fixes llvm#117960.
…n class templates These tests are variants of the crash reported in GitHub issue llvm#117960, in which an opaque-enum-declaration in a class template broke basic assumptions during parsing of arithmetic expressions due to missing promotion types of instances of EnumDecl.
Co-authored-by: cor3ntin <[email protected]>
thebrandre
force-pushed
the
fix-issue-117960
branch
from
c9f4a26 to
a2e9227
Compare
Thanks for pointing that out @cor3ntin. I messed that up in my last edit.
Sure. You won't notice it but I literally spent half an hour thinking about this sentence. It's so difficult to be clear, technically correct, and concise at the same time.
I am still undecided here. As far as I understand the standard, this is valid code as long as the enum is not used. It says The implicit instantiation of a class template specialization causes [...] the implicit instantiation of the definitions of unscoped member enumerations [temp.inst]. This feature extensively used for member functions. For example std::vector::resize is not valid for types that are not default-constructible but you can still use std::vector as long as you don't use the bad member functions. Admittedly, I can't think of any sane use case for this feature on unscoped member enumerations ... but there are very creative people out there. 😉 |
Co-authored-by: cor3ntin <[email protected]>
That would be great! 😊 |
|
@thebrandre Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
…class templates (llvm#121039) This commit fixes issues with enumeration types instantiated from an opaque-enum-declarations (see [dcl.enum]) in class templates broke basic assumptions during parsing of arithmetic expressions due to absent (NULL TYPE) promotion types of instances of EnumDecl. To this end, we repeat the simple steps in `Sema::ActOnTag` to evaluate the promotion type of a fixed enumeration based on its underlying type (see C++11 [conv.prom] p4). Note that if, instead, a full *enum-specifier* (subsequent curly braces) is provided, `Sema::ActOnEnumBody` is re-invoked on template instantiation anyway overriding the promotion type and hiding the issue. This is analog to how enumerations declarations outside of template declarations are handled. Note that, in contrast to `Sema::ActOnEnumBody`, `Sema::ActOnTag` is *not* called again for the instantiated enumeration type. Fixes llvm#117960. --------- Co-authored-by: cor3ntin <[email protected]>
This commit fixes issues with enumeration types instantiated from an opaque-enum-declarations
(see [dcl.enum]) in class templates broke basic assumptions during parsing of arithmetic
expressions due to absent (NULL TYPE) promotion types of instances of EnumDecl.
To this end, we repeat the simple steps in
Sema::ActOnTagto evaluate the promotion typeof a fixed enumeration based on its underlying type (see C++11 [conv.prom] p4).
Note that if, instead, a full enum-specifier (subsequent curly braces) is provided,
Sema::ActOnEnumBodyis re-invoked on template instantiation anyway overriding thepromotion type and hiding the issue. This is analog to how enumerations declarations
outside of template declarations are handled.
Note that, in contrast to
Sema::ActOnEnumBody,Sema::ActOnTagis not called againfor the instantiated enumeration type.
Fixes #117960.