Correct class-template deprecation behavior

[erichkeane]

Summary

Based on the comment in the test, and my reading of the standard, a deprecated warning should be issued in the following case:
template<typename T> [[deprecated]] class Foo{}; Foo<int> f;

This was not the case, because the ClassTemplateSpecializationDecl creation did not also copy the deprecated attribute.

Note: I did NOT audit the complete set of attributes to see WHICH ones should be copied, so instead I simply copy ONLY the deprecated attribute.

[erichkeane]

Corrected single line statement formatting re-squiggly braces.

[rsmith]

Thanks!

If you look at Sema::InstantiateClass, we instantiate all of the attributes there. The problem seems to be that that happens only when instantiating the class definition, which happens after the point at which we would diagnose use of a deprecated declaration.

This change will result in us having two copies of the deprecated attribute after we instantiate the class definition -- one from instantiating the declaration and another from instantiating the definition. Perhaps a better way to handle this would be to add a flag to the attribute definitions (in Attr.td) to indicate whether they apply to a declaration or just a definition (and thus whether they should be instantiated with a declaration, or only with a definition), and then instantiate the relevant subset when creating the ClassTemplateSpecializationDecl (and other kinds of decl -- I expect the same thing will happen for member classes of class templates, for function templates, and so on).

@aaron.ballman Does that seem reasonable to you? If so, we should do some investigation of the attribute set to figure out what the best default is.

[erichkeane]

In D27486#615174, @rsmith wrote:

Thanks!

If you look at Sema::InstantiateClass, we instantiate all of the attributes there. The problem seems to be that that happens only when instantiating the class definition, which happens after the point at which we would diagnose use of a deprecated declaration.

This change will result in us having two copies of the deprecated attribute after we instantiate the class definition -- one from instantiating the declaration and another from instantiating the definition.

Well darn, I didn't notice this.

Perhaps a better way to handle this would be to add a flag to the attribute definitions (in Attr.td) to indicate whether they apply to a declaration or just a definition (and thus whether they should be instantiated with a declaration, or only with a definition), and then instantiate the relevant subset when creating the ClassTemplateSpecializationDecl (and other kinds of decl -- I expect the same thing will happen for member classes of class templates, for function templates, and so on).

I can definitely look into this. I have a feeling (based on the fact that this attribute IS picked up by function-template-instantiations) that it likely wouldn't be necessary for the others, but I can start to look into the scaffolding to get this to happen.

@aaron.ballman Does that seem reasonable to you? If so, we should do some investigation of the attribute set to figure out what the best default is.

As far as this investigation, I'd suspect that the current behavior (never applies to declaration) is likely the correct 'default' is. I suspect that there is also a massive work-item to go through the current list to see which SHOULD apply to declarations.

[majnemer]

lib/Sema/SemaTemplate.cpp

2355

Please capitalize Attr.

2356

I think you can remove the clang::

[erichkeane]

Thanks David!
To all - I'm actually doing my best to rewrite this based on Richard's suggestions, so look for a 'in progress' update to this review as soon as I get something that is reasonably presentable.

[erichkeane]

I've been trying to do what @rsmith suggested, so this is a WIP checkpoint, I was hoping you could take a look and tell me if I'm on the right track. I beleive I'm very nearly done, and all but 1 of the tests pass, which if you have spare cycles and could suggest something, I would much appreciate it. Currently:

template<Class T> struct C {
enum [[deprecated]] Enum {c0;}};

void foo() {
C<int>Enum x; Used to warn, no longer does
x = C<int>::c0; Used to warn, no longer does
}

[erichkeane]

Updated based on Richard Smith's suggestion, all tests pass with no alteration, and the initial incorrect behavior was corrected in an existing test.

[aaron.ballman]

include/clang/Basic/Attr.td

301 ↗

(On Diff #82909)

template declaration -> template declarations

302 ↗

(On Diff #82909)

on definition -> to the definition

include/clang/Sema/Sema.h

7388–7391 ↗

(On Diff #82909)

Did clang-format produce this formatting?

lib/Sema/SemaTemplateInstantiateDecl.cpp

320 ↗

(On Diff #82909)

Are you sure ND is always non-null? If so, then you should use cast<> above instead of dyn_cast<>.

utils/TableGen/ClangAttrEmitter.cpp

2456 ↗

(On Diff #82909)

What does "time" mean in DeclTime?

2507 ↗

(On Diff #82909)

Add a newline above this comment to separate it from the previous function body.

[erichkeane]

All Aaron's comments addressed in a patch that is on its way!

lib/Sema/SemaTemplateInstantiateDecl.cpp

320 ↗

(On Diff #82909)

I hadn't thought much about it actually, this is from a section in InstantiateAttrs (see 410 in this file). Also reinspecting, it seems that ND can be moved above the for-loop as well, so I'm going to do that, so that we can perhaps save the attributes in that case.

utils/TableGen/ClangAttrEmitter.cpp

2456 ↗

(On Diff #82909)

Right, good point. I'll choose a better name in the next patch.

[erichkeane]

Updated based on Aarons comments.

[aaron.ballman]

lib/Sema/SemaTemplateInstantiateDecl.cpp

322 ↗

(On Diff #83072)

No need to check for ND here; already done above. You might also want to remove ThisContext and just sink the initialization.

utils/TableGen/ClangAttrEmitter.cpp

2520 ↗

(On Diff #83072)

DeclTime -> AppliesToDecl

2525 ↗

(On Diff #83072)

DeclTime -> AppliesToDecl

[erichkeane]

Aaron's new comments

[erichkeane]

Marking Aaron's comments done.

[aaron.ballman]

This LGTM, but you should wait for @rsmith to sign off as well.

[erichkeane]

Thanks for your help here Aaron.

I agree about waiting for @rsmith, thanks!

[rsmith]

include/clang/Basic/Attr.td

301 ↗

(On Diff #83081)

I find this confusing -- it seems to suggest the attribute would be applied to the template declaration, not the templated declaration. I also think that the property we're modelling here is something more general than something about templates -- rather, I think the property is "is this attribute only meaningful when applied to / inherited into a defintiion?" It would also be useful to make clear that this only applies to class templates; for function templates, we always instantiate all the attributes with the declaration.

Looking through our current attribute set, it looks like at least AbiTag should also get this set, and maybe also Visibility. (Though I wonder if there would be any problem with always instantiating the full attribute set for a class declaration.)

lib/Sema/SemaTemplate.cpp

2357–2358

You should also presumably do this when instantiating a member CXXRecordDecl nested within a class template, and when instantiating a local class in a function template.

What should happen if more attributes are added between uses of the template? Example:

template<typename T> struct A;
A<int> *p;
template<typename T> struct [[deprecated]] A;
A<int> *q; // warn here?

[erichkeane]

Thanks for the feedback Richard! I'll look into whether instantiating the full attribute set upon creation is a possibility.

include/clang/Basic/Attr.td

301 ↗

(On Diff #83081)

(Though I wonder if there would be any problem with always instantiating the full attribute set for a class declaration.)

This is definitely a good point. It seems that just about every other usage of instantiating attributes happens right after creation, class template specialization is the lone exception it seems.

If I were to simply revert most of this change, then alter my SemaTemplate.cpp changes to just call InstantiateAttrs and presumably remove the other call to InstantiateAttrs (since it results in 2 sets of attributes), would you consider that to be more acceptable?

[erichkeane]

I've updated this patch based on some of Richard's suggestions, simplified my implementation, and added a few more tests. Currently the A<int> and B<int> notes are in the wrong place, but I was hoping someone could help me figure out how to correct those? They are on the initial declaration.

Also, @rsmith: Could you suggest a few more tests? I'm not sure I captured all of your concerns.

[erichkeane]

I've actually figured out how to fix the diagnosis location. I switched the diagnosis to use the location of the actual [[deprecated]] attribute instead of the Declaration location. IMO, this is a BETTER note anyway (since the arrow points to the actual [[deprecated]] label!). I'll be uploading a new diff once I clean up a few things.

[erichkeane]

Fixed the deprecated location 'note'. Observe the changes to a variety of tests.

[rsmith]

include/clang/Basic/Attr.td

301 ↗

(On Diff #83081)

Yes, I think we should at least try that and see if there's a reason why we would need the extra complexity here.

[erichkeane]

@rsmith did you ever get a chance to re-review this? Is this what you were wanting for this?

lib/Sema/SemaTemplate.cpp

if (!Decl) {
       // the set of specializations.
       Decl = ClassTemplateSpecializationDecl::Create(Context,
                             ClassTemplate->getTemplatedDecl()->getTagKind(),
                                                 ClassTemplate->getDeclContext(),
                             ClassTemplate->getTemplatedDecl()->getLocStart(),
                                                 ClassTemplate->getLocation(),
                                                      ClassTemplate,
                                                      Converted, nullptr);
+      if (auto *attr = ClassTemplate->getTemplatedDecl()
+                           ->getAttr<clang::DeprecatedAttr>()) {
+        Decl->addAttr(attr->clone(Context));
+      }
       ClassTemplate->AddSpecialization(Decl, InsertPos);
       if (ClassTemplate->isOutOfLine())
         Decl->setLexicalDeclContext(ClassTemplate->getLexicalDeclContext());
     }
 
     // Diagnose uses of this specialization.
     (void)DiagnoseUseOfDecl(Decl, TemplateLoc);
 

test/CXX/dcl.dcl/dcl.attr/dcl.attr.deprecated/p1.cpp

 enum [[deprecated]] e { E }; // expected-note {{'e' has been explicitly marked deprecated here}}
 e my_enum; // expected-warning {{'e' is deprecated}}
 
 template <typename T> class X {};
 template <> class [[deprecated]] X<int> {}; // expected-note {{'X<int>' has been explicitly marked deprecated here}}
 X<char> x1;
 X<int> x2; // expected-warning {{'X<int>' is deprecated}}
 
-template <typename T> class [[deprecated]] X2 {};
+template <typename T> class [[deprecated]] X2 {}; //expected-note {{'X2<char>' has been explicitly marked deprecated here}}
 template <> class X2<int> {};
-X2<char> x3; // FIXME: no warning!
-X2<int> x4;
+X2<char> x3; // expected-warning {{'X2<char>' is deprecated}}
+X2<int> x4; // No warning, the specialization removes it.
+