Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
zyn0217 opened this issue Jul 17, 2024 · 10 comments · Fixed by #104046
Closed

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

zyn0217 opened this issue Jul 17, 2024 · 10 comments · Fixed by #104046
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"

Comments

@zyn0217
Copy link
Contributor

zyn0217 commented Jul 17, 2024

(This is uncovered while working on #75697)

6591149 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

@zyn0217 zyn0217 added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Jul 17, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 17, 2024

@llvm/issue-subscribers-clang-frontend

Author: Younan Zhang (zyn0217)

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

6591149 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
Copy link
Contributor

@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
Copy link
Collaborator

It's unnecessary and can eventually be removed: 6591149

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

@zyn0217
Copy link
Contributor Author

zyn0217 commented Jul 19, 2024

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

@AaronBallman
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants