Attributes on templates are mishandled

[Quuxplusone]

Bugzilla Link	38302
Version	trunk
OS	All
CC	@DougGregor

Extended Description

// https://godbolt.org/g/pqog4L
template<class T>
[[noreturn]] void one();

template<>
void one<int>() {}

template<class T>
void two();

template<>
[[noreturn]] void two<int>() {}

As far as I can tell, this program produces bogus diagnostics in both directions. First, the explicit specialization one<int> seems to be inheriting [[noreturn]] from the primary template; I believe this is unwanted behavior.

    test.cc:2:18: warning: function declared 'noreturn' should not return [-Winvalid-noreturn]
    void one<int>() {}
                     ^

Second, Clang complains that the explicit specialization two<int> is marked [[noreturn]] when its "first declaration" is not so marked. Clang seems to be considering the primary template as the "first" declaration of every specialization (even though the accompanying note nonsensically points to the specialization's own declaration). This is problematic, because maybe the programmer wants to apply [[noreturn]] NOT to ALL possible instantiations of the primary template, but only to this one explicit specialization.

    test.cc:9:3: error: function declared '[[noreturn]]' after its first declaration
    [[noreturn]] void two<int>() {}
      ^
    test.cc:9:19: note: declaration missing '[[noreturn]]' attribute is here
    [[noreturn]] void two<int>() {}
                      ^

After some discussion on Slack, I think what ought to happen here is that the attribute on the primary template ought to be applied only to instantiations that come directly from that primary (unspecialized) template; and the attribute on the explicit specialization ought to be applied only to that explicit specialization. But we see that Clang doesn't do either of those things.

This is highly relevant to my interests because of P1144 [[trivially_relocatable]], and I'd love to put together a patch to fix it, but at least at the moment I'm completely lost as to where I would even start looking.

See also: llvm/llvm-project#21132

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[aaronpuchert]

Came across the same issue. Especially when adding [[noreturn]] to specializations I don't see any reason to disallow that.

The error is likely referring to [dcl.attr.noreturn]p1 and might not necessarily affect other attributes. My understanding is that an explicit specialization is not a redeclaration and thus the rule does not apply here.

[aaronpuchert]

My understanding is that an explicit specialization is not a redeclaration and thus the rule does not apply here.

Can't find explicit language for that, but [temp.expl.spec]p13 says that e.g. inline and constexpr on explicit specializations are independent, suggesting that these are not redeclarations.

@AaronBallman, any thoughts on this?

[AaronBallman]

My understanding is that an explicit specialization is not a redeclaration and thus the rule does not apply here.

A function template is not a function (template) declaration (https://eel.is/c++draft/temp.fct#general-1), see also https://en.cppreference.com/w/cpp/language/function_template for more human-readable information on it.

I think https://eel.is/c++draft/temp.expl.spec#13 is intended to clarify that the explicit specialization is independent of the primary template, but it's not clear whether attributes are explicitly elided from that list or an accidental omission. I've raised a question on the core reflectors.

[AaronBallman]

From the Core reflector (not an official committee response, just early feedback):

I think that https://eel.is/c++draft/temp.expl.spec#13 implies that the attributes of the specialization are also independent of the primary template, but I can't find anything in the standard which says what should happen one way or the other.

I think that's plainly the right interpretation, since explicit specializations must be declared (though not defined) everywhere they are used. Accordingly, there's never a need to consult the primary template declaration.

So it seems like we've confirmed this report, assuming nobody in Core disagrees.

[aaronpuchert]

The error seems to come out of Sema::MergeFunctionDecl. So is the problem that we call this for specializations at all, or should we just exclude specializations for CXX11NoReturnAttr?

If specializations are generally considered as independent, we should probably not even be there. But maybe there is a good reason why we are.

Of course fully independent they are not, e.g.

template<typename T> T f();
template<> double f<int>();

is not allowed. The signatures have to match, if only to find which template we're specializing. We also can't override the storage class. But it seems like attributes can generally be independent, and that this is generally desirable. After all, if there is need for a specialization, it might also behave differently.

[AaronBallman]

So is the problem that we call this for specializations at all, or should we just exclude specializations for CXX11NoReturnAttr?

I don't think we should handle attributes on a case by case basis (not unless we absolutely have to), but I think we have to call MergeFunctionDecl() because that's doing the type checking to ensure that the two functions with the same name are or are not a valid redeclaration and we have special logic in there for function template specializations. I'm not yet certain what the correct approach is here.

After all, if there is need for a specialization, it might also behave differently.

That's my thinking in general, but then you have a case like: https://godbolt.org/z/ba19jn16c where the user may reasonably have thought they were marking all specializations as being deprecated.

That said, I think there's more power to requiring users to specify the attributes on the explicit specializations. If we we don't add it automatically and that's wrong, the user can add it themselves. However, if we add it automatically and that's wrong, the user has no recourse.

[aaronpuchert]

That's my thinking in general, but then you have a case like: https://godbolt.org/z/ba19jn16c where the user may reasonably have thought they were marking all specializations as being deprecated.

Good point. Maybe there is a difference between "general" declaration attributes ([[maybe_unused]], [[deprecated]]) and function declaration attributes ([[noreturn]], or many static analysis attributes) here: the specialization still refers to the original (template) declaration and thus should probably inherit general declaration attributes, but it should not inherit function declaration attributes, because the specialization is a "new" function.

[aaronpuchert]

Perhaps we can say that "general declaration attributes" like [[maybe_unused]] or [[deprecated]] are considered to sit on the outer template declaration, whereas function declaration attributes are considered to sit on the contained template function. Syntactically of course they all sit on the function, because (unless I'm missing something) it's not possible to put attributes on the template itself.

As an example, for

template<typename T>
[[deprecated]] [[noreturn]] int f();

we have

FunctionTemplateDecl f
|-TemplateTypeParmDecl typename depth 0 index 0 T
`-FunctionDecl f 'int ()'
  |-DeprecatedAttr "" ""
  `-CXX11NoReturnAttr

and I would suggest to make this

FunctionTemplateDecl f
|-TemplateTypeParmDecl typename depth 0 index 0 T
|-DeprecatedAttr "" ""
`-FunctionDecl f 'int ()'
  `-CXX11NoReturnAttr

at least in spirit, if it would be a bad idea to actually change the AST.

Then we could also warn on

template<typename T>
class [[deprecated]] X;

template<template<typename> class T>
void f();

void test() {
    f<X>();
}

like GCC does. This doesn't instantiate the deprecated class. Understanding the attribute as belonging to the template declaration itself would justify a warning though. This is surely a bit contrived, because templates are usually instantiated and not just referenced.

[AaronBallman]

I'd like to hear what Core thinks about this, because it turns out I'm not the only person on the committee who would like the standard to say something about this. It's a bit difficult for me to see a general rule to apply here because standard attributes and vendor-specified attributes are pretty decent beasts. For example, the standard currently doesn't expose any type attributes, but vendor-specific calling convention attributes may be written on the type (this seems like a case where you'd want the specialization to inherit the attribute from the primary template), but [[maybe_unused]] seems like something you could potentially want to use on a case-by-case basis for specializations (maybe one of the specializations only exists to be called in an assert() macro).

While thinking on this, I realized there was a related case the standard also is pretty quiet about -- attributes written on the parameters of the primary function template. There's implementation divergence there too: https://godbolt.org/z/1eTrTorn1

So I've gone ahead and filed a core issue at https://github.com/cplusplus/CWG/issues/68 to track this on the committee's side.

My current thinking is that we do not want attributes from the primary template to also be applied to an explicit specialization in general. That removes expressivity from the explicit specialization, and I think there are too many situations in which the specialization may need a different set of attributes than the primary template. I would really like to avoid trying to do anything on an attribute-by-attribute basis, but the one situation I'm still thinking about heavily is with type attributes (thinking about things like calling convention attributes).

[AaronBallman]

So I've gone ahead and filed a core issue at https://github.com/cplusplus/CWG/issues/68 to track this on the committee's side.

This is now CWG2604.

[aaronpuchert]

So I've gone ahead and filed a core issue at cplusplus/CWG#68 to track this on the committee's side.

Thanks!

My current thinking is that we do not want attributes from the primary template to also be applied to an explicit specialization in general. That removes expressivity from the explicit specialization, and I think there are too many situations in which the specialization may need a different set of attributes than the primary template. I would really like to avoid trying to do anything on an attribute-by-attribute basis, but the one situation I'm still thinking about heavily is with type attributes (thinking about things like calling convention attributes).

Fully agreed. As we consider calling conventions as non-erasable part of the function type and allow overloading on calling conventions, we probably need to require them consistent across specializations.

Erasable type attributes however (like nullability) would not need to be consistent. (Though we could still require it.)

[AaronBallman]

Erasable type attributes however (like nullability) would not need to be consistent. (Though we could still require it.)

Agreed, there are type attributes which are erasable as well. There are also type attributes which act as qualifiers (like address_space). So it's a complicated space where it may be impossible to come up with a one-size-fits-all approach.

[msebor]

There was a discussion on this topic a few years ago on gcc-patches: [PATCH] diagnose specializations of deprecated templates (PR c++/84318). One concern with not applying attributes a primary is declared with to its specializations (either explicit or partial) is that it would make it possible for a user to bypass their effects by supplying their own partial specialization (e.g., std::auto_ptr all of whose uses and instances are meant to be deprecated). At the same time, a library author might need to declare the primary deprecated to discourage its specializations by users while providing a subset of specializations in the library for users to use without the deprecation warning. At some point someone suggested to rely on where the template and specializations are declared/defined (e.g., same header vs different headers) to distinguish between these two otherwise mutually exclusive use cases.

[hubert-reinterpretcast]

This is now CWG2604.

CWG2604 up for vote this week. Expect Clang behaviour will need to change.

[AaronBallman]

Thanks for the notice! I agree with the direction that's heading in (though I think it doesn't say what happens for class template specializations). Can I assume this point was discussed explicitly:

One concern with not applying attributes a primary is declared with to its specializations (either explicit or partial) is that it would make it possible for a user to bypass their effects by supplying their own partial specialization (e.g., std::auto_ptr all of whose uses and instances are meant to be deprecated). At the same time, a library author might need to declare the primary deprecated to discourage its specializations by users while providing a subset of specializations in the library for users to use without the deprecation warning.

and that the reasoning CWG went the chosen direction is because it provides the most flexibility despite the potential for misuse by "removing" attributes on explicit specializations?

[hubert-reinterpretcast]

Can I assume this point was discussed explicitly

iirc, it wasn't (at least recently). For the scenario raised, I vaguely recall (again, not discussed recently) a related concern that perhaps there should be a way to clearly make a template "deprecated" (because the current way to try applying "deprecated" to a primary template can be interpreted as only applying "deprecated" to specializations produced by instantiating the primary template). It seems the current resolution affirms the interpretation that the "deprecated" applies to specializations so instantiated.

that the reasoning CWG went the chosen direction is because it provides the most flexibility despite the potential for misuse by "removing" attributes on explicit specializations

My reading is that CWG went with the chosen direction for consistency with a model that explicit specializations are unrelated to what the primary template produces (except that certain properties are necessarily similar because they are needed to associate the explicit specialization with the template it specializes).

[AaronBallman]

My reading is that CWG went with the chosen direction for consistency with a model that explicit specializations are unrelated to what the primary template produces (except that certain properties are necessarily similar because they are needed to associate the explicit specialization with the template it specializes).

Thank you for the information! I think that's a reasonable direction (assuming @erichkeane has no concerns).

[erichkeane]

Yep, I agree, an explicit specialization should inherit, it is its 'own' declaration, and thus should have its own attributes. However, an instantiation of the primary template should still inherit the attributes (which I think we did some work towards at one point ~5 years ago). I know our attributes-on-templates rules are a bit of a mess though.

[aaronpuchert]

Interestingly, there was an earlier discussion in #12572 about how much [[noreturn]] should be part of the function type. But CWG2604 is pretty clear that we shouldn't treat it as part of the function type for the purpose of explicit specialization.

How do we fix this now? I suppose we still want to call Sema::MergeFunctionDecl for a template and its specialization, so we probably have to fix it in there, right?