Page MenuHomePhabricator

[clang] Require parameter pack to be last argument in concepts.

Authored by luken-google on Nov 17 2022, 7:26 AM.



Fixes GH48182.
Applies the patch proposed by @rsmith in the bug and adds a test.

Diff Detail

Event Timeline

luken-google created this revision.Nov 17 2022, 7:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 7:26 AM
luken-google requested review of this revision.Nov 17 2022, 7:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 7:26 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
luken-google edited the summary of this revision. (Show Details)Nov 17 2022, 7:27 AM
luken-google added reviewers: rsmith, ilya-biryukov.
luken-google added a subscriber: rsmith.

Adding release note.

The error itself makes sense, but I don't quite understand why usages of invalid<int> did not produce errors before. Any idea why that happened? Are there some other bugs hiding?
It seems that at some point when parsing this code:

static_assert(invalid<int> also here ;

we chose to silently recover by skipping until the semicolon, but never actually produced any errors. Is there a code path that should also be updated to handle that?

ilya-biryukov added a comment.EditedNov 21 2022, 8:24 AM

E.g. gcc actually accepts the concept definition, but the later usage fails with "wrong number of template arguments". I wonder why Clang has not been doing the latter before this patch.

There is logic in the template expansion that returns an error state if the template parameter pack was not at the end of the template parameter list. It returns true but does not generate any diagnostics, but the outer code error machinery assumes that diagnostics have already been generated and so considers the expression invalid and moves on.

As this fix no longer accepts these ill-formed templates as valid, that code no longer fires. I have strengthened the check there to an assert. PTAL.

Strengthen assert.

Thanks for the explanation. LGTM! And thanks for adding an assert.

It's interesting that recovery for classes seems to be a bit better here:

template <class ...T, class ...U> struct invalid {};
int a = invalid<int>(10); // there is no error: undefined identified 'invalid'

but I suspect chasing the improvements there is out of scope for this particular GH issue. Maybe worth adding a FIXME/filing a GH issue for improving this in the future.

ilya-biryukov accepted this revision.Nov 28 2022, 8:48 AM
This revision is now accepted and ready to land.Nov 28 2022, 8:48 AM

For future reference it's handy to include the "Differential Revision:" (arc will create this for you, if you use that) in the commit message both so folks looking at the commit can find the review/know that it was reviewed on Phab, and that way Phab will auto-close the review and include a link to the commit.

Also I think we're still using the "PR" prefix predominantly for bug references, rather than the "GH" suffix you've used here (handy for consistency - easier to search up the bugs if the prefix is more consistent).