llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.7k stars 11.4k forks source link

Clang shouldn't accept extraneous template headers as an extension #99296

Closed zyn0217 closed 2 weeks ago

zyn0217 commented 1 month ago

(This is uncovered while working on https://github.com/llvm/llvm-project/pull/75697)

https://github.com/llvm/llvm-project/commit/65911498eff34ac4fc2f9c250efd49aa51c35f85 results in an extension where Clang accepts templates with redundant template headers. However, this is clearly ill-formed and no other compilers would accept it. I think we should probably remove the extension in Sema::MatchTemplateParametersToScopeSpecifier() and reject such templates in the end.

https://compiler-explorer.com/z/TcPGjWMGM

llvmbot commented 1 month ago

@llvm/issue-subscribers-clang-frontend

Author: Younan Zhang (zyn0217)

(This is uncovered while working on https://github.com/llvm/llvm-project/pull/75697) https://github.com/llvm/llvm-project/commit/65911498eff34ac4fc2f9c250efd49aa51c35f85 results in an extension where Clang accepts templates with redundant template headers. However, this is clearly ill-formed and no other compilers would accept it. I think we should probably remove the extension in `Sema::MatchTemplateParametersToScopeSpecifier()` and reject such templates in the end. https://compiler-explorer.com/z/TcPGjWMGM
cor3ntin commented 1 month ago

@AaronBallman @zygoloid You remember how this behavior came about and whether there are still reasons to keep it? Otherwise I strongly agree we should align with other implementations

AaronBallman commented 1 month ago

It's unnecessary and can eventually be removed: https://github.com/llvm/llvm-project/commit/65911498eff34ac4fc2f9c250efd49aa51c35f85

We should probably have a deprecation period given that we've supported the extension for 15 years though.

zyn0217 commented 1 month ago

@AaronBallman How about deprecating this in clang 19 and turning all warnings into errors in clang 20?

AaronBallman commented 1 month ago

I'm not opposed but I do worry about the timing. The branch date is in ~four days, so we won't get early smoke testing on the deprecation before we cut the release candidate. Given that we've supported this for so long and nothing is on fire as a result, I'd feel more comfortable deprecating first thing in Clang 20 and plan to turn the warnings into errors in Clang 21. WDYT?

cor3ntin commented 1 month ago

I'm not opposed but I do worry about the timing. The branch date is in ~four days, so we won't get early smoke testing on the deprecation before we cut the release candidate. Given that we've supported this for so long and nothing is on fire as a result, I'd feel more comfortable deprecating first thing in Clang 20 and plan to turn the warnings into errors in Clang 21. WDYT?

There is already an on-by-default warning; We just need to reword:

extraneous template parameter list in template specialization

To something along the lines of:

extraneous template parameter list in template specialization is non-conforming and will not be supported by future clang versions

AaronBallman commented 1 month ago

I'm not opposed but I do worry about the timing. The branch date is in ~four days, so we won't get early smoke testing on the deprecation before we cut the release candidate. Given that we've supported this for so long and nothing is on fire as a result, I'd feel more comfortable deprecating first thing in Clang 20 and plan to turn the warnings into errors in Clang 21. WDYT?

There is already an on-by-default warning; We just need to reword:

extraneous template parameter list in template specialization

To something along the lines of:

extraneous template parameter list in template specialization is non-conforming and will not be supported by future clang versions

Oh I hadn't checked that we diagnose this by default already, that alleviates my concerns about timing. I would recommend using a typical deprecation message though, something like use of an extraneous template parameter list in a template specialization is a Clang extension and is deprecated. The current diagnostic has no diagnostic group associated with it, so we should probably also invent a diagnostic group for it and put that group under -Wdeprecated.

zygoloid commented 1 month ago

I think the reason we had this extension is that historically the rules on whether a template<> is required (and if so, how many) were pretty murky and there was both widespread implementation divergence and a core issue suggesting to change the rules. If that's no longer the case, then let's get rid of the extension!

zygoloid commented 1 month ago

See CWG529, CWG531 (which contemplated changing the rule), and CWG1993 (which suggests making the template<> optional in the unclear case, that is, standardizing the extension).

zygoloid commented 1 month ago

It looks like all major compilers accept the standard form these days. MSVC and EDG accept only the standard form. For class and function member templates of an explicitly specialized class, GCC silently accepts the cases that Clang accepts with an extension warning. For non-static data members, GCC accepts a spurious template<> with a warning whereas no-one else (including Clang) accepts, and Clang accepts too many template<>s for an explicit specialization of a non-static data member template within an explicitly specialized class (which GCC does not): https://godbolt.org/z/v8Pr3TEMz

So there is some risk that we'll break code that silently works on GCC if we remove the extension, but it does seem like standard rule is widely implemented at this point, and it's just GCC straggling a bit.

Given that we've always warned by default without any way to turn off the warning, I don't think we really gain anything from rushing a last-minute change to mark the warning as deprecated and to allow it to be turned off. How about we make no code changes for Clang 19, and turn this to an error by default on trunk as soon as we've branched? (I think it's probably worth keeping a warning flag to turn the error back off for people coming from GCC, but maybe just making it a hard error is fine too.)