[Clang] Add [[clang::no_specializations]] #101469
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Nikolas Klauser (philnik777) ChangesThis can be used to inform users when a template should not be specialized. For example, this is the case for the standard type traits (except for Full diff: https://github.com/llvm/llvm-project/pull/101469.diff 7 Files Affected:
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 8ac2079099c85..e074cc8b285a9 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -103,6 +103,9 @@ def NonParmVar : SubsetSubject<Var,
def NonLocalVar : SubsetSubject<Var,
[{!S->hasLocalStorage()}],
"variables with non-local storage">;
+def VarTmpl : SubsetSubject<Var, [{S->getDescribedVarTemplate()}],
+ "variable template">;
+
def NonBitField : SubsetSubject<Field,
[{!S->isBitField()}],
"non-bit-field non-static data members">;
@@ -3327,6 +3330,14 @@ def DiagnoseIf : InheritableAttr {
let Documentation = [DiagnoseIfDocs];
}
+def DiagnoseSpecializations : InheritableAttr {
+ let Spellings = [Clang<"diagnose_specializations", /*AllowInC*/0>];
+ let Subjects = SubjectList<[ClassTmpl, VarTmpl]>;
+ let Documentation = [DiagnoseSpecializationsDocs];
+ let MeaningfulToClassTemplateDefinition = 1;
+ let TemplateDependent = 1;
+}
+
def ArcWeakrefUnavailable : InheritableAttr {
let Spellings = [Clang<"objc_arc_weak_reference_unavailable">];
let Subjects = SubjectList<[ObjCInterface], ErrorDiag>;
@@ -4581,7 +4592,7 @@ def HLSLResource : InheritableAttr {
let Spellings = [];
let Subjects = SubjectList<[Struct]>;
let LangOpts = [HLSL];
- let Args = [
+ let Args = [
EnumArgument<
"ResourceKind", "llvm::hlsl::ResourceKind",
/*is_string=*/0,
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index 94c284fc73158..4ca67a63714d4 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -975,6 +975,15 @@ Query for this feature with ``__has_attribute(diagnose_if)``.
}];
}
+def DiagnoseSpecializationsDocs : Documentation {
+ let Category = DocCatDecl;
+ let Content = [{
+``clang::diagnose_specializations`` can be appied to class templates which
+should not be specialized by users. This is primarily used to diagnose user
+specializations of standard library type traits.
+ }];
+}
+
def PassObjectSizeDocs : Documentation {
let Category = DocCatVariable; // Technically it's a parameter doc, but eh.
let Heading = "pass_object_size, pass_dynamic_object_size";
@@ -7388,10 +7397,10 @@ def HLSLLoopHintDocs : Documentation {
let Content = [{
The ``[loop]`` directive allows loop optimization hints to be
specified for the subsequent loop. The directive allows unrolling to
-be disabled and is not compatible with [unroll(x)].
+be disabled and is not compatible with [unroll(x)].
Specifying the parameter, ``[loop]``, directs the
-unroller to not unroll the loop.
+unroller to not unroll the loop.
.. code-block:: hlsl
@@ -8306,4 +8315,3 @@ Declares that a function potentially allocates heap memory, and prevents any pot
of ``nonallocating`` by the compiler.
}];
}
-
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 19c3f1e043349..d6f6111f70868 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -472,6 +472,7 @@ def ExpansionToDefined : DiagGroup<"expansion-to-defined">;
def FlagEnum : DiagGroup<"flag-enum">;
def IncrementBool : DiagGroup<"increment-bool", [DeprecatedIncrementBool]>;
def InfiniteRecursion : DiagGroup<"infinite-recursion">;
+def InvalidSpecialization : DiagGroup<"invalid-specialization">;
def PureVirtualCallFromCtorDtor: DiagGroup<"call-to-pure-virtual-from-ctor-dtor">;
def GNUImaginaryConstant : DiagGroup<"gnu-imaginary-constant">;
def IgnoredGCH : DiagGroup<"ignored-gch">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 581434d33c5c9..5972d630347ec 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5407,6 +5407,9 @@ def note_dependent_function_template_spec_discard_reason : Note<
"candidate ignored: %select{not a function template|"
"not a member of the enclosing %select{class template|"
"namespace; did you mean to explicitly qualify the specialization?}1}0">;
+def warn_diag_specialization : Warning<
+ "specializing a template which should not be specialized">,
+ DefaultError, InGroup<InvalidSpecialization>;
// C++ class template specializations and out-of-line definitions
def err_template_spec_needs_header : Error<
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 9011fa547638e..eb0a705fef044 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -1215,6 +1215,12 @@ static void handlePreferredName(Sema &S, Decl *D, const ParsedAttr &AL) {
<< TT->getDecl();
}
+static void handleDiagnoseSpecializations(Sema &S, Decl *D,
+ const ParsedAttr &AL) {
+ D->getDescribedTemplate()->addAttr(
+ DiagnoseSpecializationsAttr::Create(S.Context, AL));
+}
+
bool Sema::isValidPointerAttrType(QualType T, bool RefOkay) {
if (RefOkay) {
if (T->isReferenceType())
@@ -6700,6 +6706,9 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
case ParsedAttr::AT_PreferredName:
handlePreferredName(S, D, AL);
break;
+ case ParsedAttr::AT_DiagnoseSpecializations:
+ handleDiagnoseSpecializations(S, D, AL);
+ break;
case ParsedAttr::AT_Section:
handleSectionAttr(S, D, AL);
break;
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index c22e329bef2b9..ddcc3272cff1f 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -3976,6 +3976,9 @@ DeclResult Sema::ActOnVarTemplateSpecialization(
<< IsPartialSpecialization;
}
+ if (VarTemplate->hasAttr<DiagnoseSpecializationsAttr>())
+ Diag(TemplateNameLoc, diag::warn_diag_specialization);
+
// Check for unexpanded parameter packs in any of the template arguments.
for (unsigned I = 0, N = TemplateArgs.size(); I != N; ++I)
if (DiagnoseUnexpandedParameterPack(TemplateArgs[I],
@@ -8085,6 +8088,9 @@ DeclResult Sema::ActOnClassTemplateSpecialization(
return true;
}
+ if (ClassTemplate->hasAttr<DiagnoseSpecializationsAttr>())
+ Diag(TemplateNameLoc, diag::warn_diag_specialization);
+
bool isMemberSpecialization = false;
bool isPartialSpecialization = false;
diff --git a/clang/test/SemaCXX/attr-diagnose-specializations.cpp b/clang/test/SemaCXX/attr-diagnose-specializations.cpp
new file mode 100644
index 0000000000000..bf9ea2c56c18a
--- /dev/null
+++ b/clang/test/SemaCXX/attr-diagnose-specializations.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 %s -verify
+
+#if !__has_cpp_attribute(clang::diagnose_specializations)
+# error
+#endif
+
+struct [[clang::diagnose_specializations]] S {}; // expected-warning {{'diagnose_specializations' attribute only applies to class templates}}
+
+template <class T, class U>
+struct [[clang::diagnose_specializations]] is_same {
+ static constexpr bool value = __is_same(T, U);
+};
+
+template <>
+struct is_same<int, char> {}; // expected-error {{specializing a template which should not be specialized}}
+
+template <class>
+struct Template {};
+
+template <class T>
+struct is_same<Template<T>, Template <T>> {}; // expected-error {{specializing a template which should not be specialized}}
+
+bool test_instantiation1 = is_same<int, int>::value;
+
+template <class T, class U>
+[[clang::diagnose_specializations]] inline constexpr bool is_same_v = __is_same(T, U);
+
+template <>
+inline constexpr bool is_same_v<int, char> = false; // expected-error {{specializing a template which should not be specialized}}
+
+template <class T>
+inline constexpr bool is_same_v<Template <T>, Template <T>> = true; // expected-error {{specializing a template which should not be specialized}}
+
+bool test_instantiation2 = is_same_v<int, int>;
|
|
A list of places this could be used: https://eel.is/c++draft/cmp.result#1 Also consider adding an argument that lets you diagnose https://eel.is/c++draft/namespace.std#2.1
To diagnose specializations for |
Given that this seems like a very standard-library-specific feature I'm not sure it makes a ton of sense to add it to this attribute. I think adding an ad-hoc diagnostic for this would be better. |
There was a problem hiding this comment.
I like the general idea
- I would prefer this to be called
no_specializationsordisallow_specializations - I would like to see more tests to what it can mor cannot be applied to (for example an explicit specialization, a variable template, an alias)
- In the same vein, can this be applied to a partial specialization to prevent subsequent specializations?
- I think the diagnostic should say
note: marked[[clang::no_specialization]] here`
cor3ntin
changed the title
|
Does it makes sense to also support function templates? |
I can't find anything explicit about function template specializations, so I'm not 100% sure whether it's allowed. Since there is no requirement that the template arguments of any function match the standards-declared ones exactly, I don't think you're allowed to specialize any standard library function. |
|
I did think about it more, and I think it would be more consistent to support the attribute on any sort of specializable entity, including functions - regardless of STL needs @AaronBallman @ldionne |
Agreed; if we're adding a custom attribute, we might as well support it in a general form so users can also make use of it for their needs (which may be different than STL needs). |
While I don't see much of a use-case for functions, I also don't see much of a downside (assuming implementing the attribute is as simple for functions as it is for variables and classes). Is there anything else that can be specialized? |
Classes, functions, variables |
That looks horrendous, but I don't think that's specializing the enum. It's specializing the enclosing class and the specialization happens to also define an enum with the same name as the enum in the base class. You can replace the enum with anything you want, including nothing at all. |
|
The list is there https://eel.is/c++draft/temp.expl.spec#1 |
|
I went for only adding |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Your test coverage also handles class templates and variable templates in addition to function templates, which I think covers the most common cases and is fine. |
Ah, yes, I meant "I only added function templates in addition to class and variable templates (which was the original set)". |
|
@AaronBallman @cor3ntin are you happy as well? |
| static void handleNoSpecializations(Sema &S, Decl *D, const ParsedAttr &AL) { | ||
| StringRef Message; | ||
| if (AL.getNumArgs() != 0) | ||
| S.checkStringLiteralArgumentAttr(AL, 0, Message); |
There was a problem hiding this comment.
static analysis flags this b/c you are not checking the return value of checkStringLiteralArgumentAttr and AFAICT every other use is checking the value.
Please confirm the intent and please fix to check the return value or please modify to purposely ignore it.
There was a problem hiding this comment.
I'm not 100% sure what the correct thing to do is. I don't think there's harm in adding the attribute without a string if the arguments are incorrect, since they don't affect what the attribute does in a significant way. OTOH there isn't that much benefit in adding it, since the program is ill-formed anyways. @AaronBallman any thoughts on this?
P.S. why isn't checkStringLiteralArgumentAttr marked [[nodiscard]]?
There was a problem hiding this comment.
P.S. why isn't checkStringLiteralArgumentAttr marked [[nodiscard]]?
Pretty sure this function, or its origin predate [[nodiscard]] by a decade or so :) Also, the use you have is completely reasonable here. The Message argument is left zero-initalized, which is the behavior you want.
I don't see any good reason to change the code here, I think the SA tool isn't really adding value here, and I don't see anything I'd change.
There was a problem hiding this comment.
It stands out as completely inconsistent with the rest of the file, literally everywhere else it is checked and they return early. I am guessing if we return early here we should just get the proper diagnostic, right? Is this case somehow different from all the two dozen or so other uses?
There was a problem hiding this comment.
Yes, it is inconsistent, but not really an issue. About 1/2 the uses have some additional processing that needs to happen, so the early return is sensible, though perhaps they could better reflect the code in that case.
But the other 1/2 could very well have their return removed with no negative effects (and likely, an improvement, as the error-AST would better reflect the syntax).
The only reason NOT to do a mass-changeover is that we're not sure which were written in a way to not properly expect an empty string here in codegen/etc.
That said, a bug report marked 'good-first-issue' for someone to go through the uses and ensure we are tolerant of them would be a good idea. @shafik: mind filing one?
This can be used to inform users when a template should not be specialized. For example, this is the case for the standard type traits (except for
common_typeandcommon_reference, which have more complicated rules).