Fix valid-for-expr ellipses eaten as invalid decl
ClosedPublic

Authored by hubert.reinterpretcast on Thu, May 18, 2:28 PM.

Details

Summary

The trial parse for declarative syntax accepts an invalid pack declaration syntax, which is ambiguous with valid pack expansions of expressions. This commit removes the invalid pack declaration syntax to avoid mistaking valid pack expansions as invalid declarator components.

Additionally, the trial parse of a template-argument-list then needs to handle the optional ellipsis that is part of that grammar, as opposed to relying on the trial parse for declarators accepting stray ellipses.

lib/Parse/ParseTentative.cpp
542

The change here is primarily for maintainability purposes. This is "morally" where the ellipsis should be checked for, but the ellipsis that we are looking for here is going to be consumed as "stray" anyway.

938

The check for what qualifies as "trailing" is not the strongest, but I find it to be the most straightforward change that should do the job. One alternative is to track whether an ellipsis was consumed within the current loop iteration.

rsmith added a comment.EditedThu, May 18, 6:51 PM

Should I assume our "misplaced ellipsis" diagnostic requires that we disambiguate the ill-formed ellipsis-after-declarator-id as a declarator in some cases? If so, do we have tests for that somewhere?

include/clang/Parse/Parser.h
2138 ↗(On Diff #99495)

Given that this flag is enabling parsing of things that are *not* valid declarators, I think the default should be the other way around. If some calling code believes it's safe to parse a trailing ellipsis as part of a declarator, it should be explicitly opting into that.

lib/Parse/ParseTentative.cpp
938

Use !NextToken().isOneOf(tok::r_paren, tok::comma) here.

Should I assume our "misplaced ellipsis" diagnostic requires that we disambiguate the ill-formed ellipsis-after-declarator-id as a declarator in some cases? If so, do we have tests for that somewhere?

Tests for the diagnostics are in clang/test/FixIt/fixit-cxx0x.cpp and clang/test/Parser/cxx11-templates.cpp.
The check-all target passes even if the ellipsis-after-declarator-id disambiguation as a declarator is removed entirely. The disambiguation process is not needed in many cases, and—when it is—can choose the declarative interpretation for other reasons.

I find the most interesting cases affected by the patch here to be those involving disambiguation between the declaration of a function template and that of a variable template.
The misplaced-ellipsis diagnostic will still trigger (if we accept non-trailing stray ellipses) on a case like

template <typename ...T> int f(T (t) ...(int), int (x));

which, although it works, is not very motivating.

Removing the (int) turns it into a possibly-valid variable template declaration in terms of parsing.
Removing the parentheses around t (whether or not the (int) is there) makes the parser decide to parse as a function declaration (and the misplaced-ellipsis diagnostic is triggered) regardless of the treatment of "stray" ellipses.
The logic implemented here does not generate the misplaced-ellipsis diagnostic for

template <typename ...T> int f(T (t) ..., int x);

So, on the whole, the stray ellipsis treatment is both too complicated and not complicated enough.

include/clang/Parse/Parser.h
2138 ↗(On Diff #99495)

Okay; will do.

lib/Parse/ParseTentative.cpp
938

Will do.

The check-all target passes even if the ellipsis-after-declarator-id disambiguation as a declarator is removed entirely.

[...]

So, on the whole, the stray ellipsis treatment is both too complicated and not complicated enough.

I think if we want to keep it, the way to do that would be to carry on through the disambiguation process and treat it as a tiebreaker (that's what we do, for instance, if we see an undeclared identifier in a position where we're looking for a type). I'm not convinced that's worthwhile, especially since our existing testcases do not need this disambiguation rule, but perhaps we could remove the stray ellipsis treatment entirely for now and reconsider adding it back if we find poor diagnostics result from it later?

The check-all target passes even if the ellipsis-after-declarator-id disambiguation as a declarator is removed entirely.

[...]

So, on the whole, the stray ellipsis treatment is both too complicated and not complicated enough.

I think if we want to keep it, the way to do that would be to carry on through the disambiguation process and treat it as a tiebreaker (that's what we do, for instance, if we see an undeclared identifier in a position where we're looking for a type). I'm not convinced that's worthwhile, especially since our existing testcases do not need this disambiguation rule, but perhaps we could remove the stray ellipsis treatment entirely for now and reconsider adding it back if we find poor diagnostics result from it later?

Okay. I'll update the patch to remove the stray ellipsis treatment entirely.

Remove stray ellipsis treatment entirely

hubert.reinterpretcast edited the summary of this revision. (Show Details)Fri, May 19, 3:17 PM
rsmith accepted this revision.Fri, May 19, 5:00 PM

LGTM, thanks!

This revision is now accepted and ready to land.Fri, May 19, 5:00 PM