Test coverage for double trailing commas is poor

[ExpHP]

Theory

Every place in the grammar which supports trailing commas should be tested that it fails for double commas, lest Rust be locked into supporting it forever. A particularly notable case of this is for macro_rules! macros, many of which must manually implement their own trailing comma support (leading to more chances for mistakes).

Reality

[ lampam @ 12:22:45 ] (master •2) ~/asd/clone/rust/src/test
$ ls **/*.rs -1 > files
$ python3

>>> import re
>>> contents = [open(x.strip()).read() for x in open('files')]
>>> r = re.compile(',\\s*,\\s*[\\])}]', re.MULTILINE)
>>> [x for x in map(r.findall, contents) if x]
[]

[ExpHP]

Agh, I should've waited a bit longer before posting.

My check did not take comments into account, so this may be a false alarm. I am hacking together a slightly more accurate test and will reopen if that test fails...

[ExpHP]

After more accurate testing, this still appears to be a problem.

#!/usr/bin/env python3
import re

# strip comments naively, under the assumption that tests which
# try to trip up comment parsers probably do not also test
# something such as double commas
BLOCK_COMMENT = re.compile('/\*(\*[^/]|[^*])*\*/', re.MULTILINE)
LINE_COMMENT = re.compile('//.*')
# Note: applied to a file's entire contents as a single string (with embedded newlines)
strip_comments = lambda s: BLOCK_COMMENT.sub('', LINE_COMMENT.sub('', s))

contents = [open(x.strip()).read() for x in open('files')]
contents = [strip_comments(s) for s in contents]

print("Test counts for trailing commas")
for end in ['\\]', '\\)', '\\}', '>']:
    r = re.compile(f',\\s*{end}', re.MULTILINE)
    print(f' ,{end[-1:]}:', len([x for x in map(r.findall, contents) if x]))

print("Test counts for double trailing commas")
for end in ['\\]', '\\)', '\\}', '>']:
    r = re.compile(f',\\s*,\\s*{end}', re.MULTILINE)
    print(f',,{end[-1:]}:', len([x for x in map(r.findall, contents) if x]))

Test counts for trailing commas
 ,]: 23
 ,): 118
 ,}: 1096
 ,>: 14
Test counts for double trailing commas
,,]: 0
,,): 0
,,}: 0
,,>: 6

[durka]

IMO the theory applied to macro_rules! macros is a bit unfair as it's a huge chore for a macro to support single trailing commas without supporting infinite trailing commas, since the $(,)* trick is the only way to do it without duplicating rules. I've been meaning to submit an RFC for $()? which would improve the situation.

[ExpHP]

I think libcore and libstd are well in a position that they can at least afford to do it right, regardless of the ergonomics. The only disadvantage I can see is that it may make documentation look unruly for more complex macros; however, none of the macros currently existing in std are that complex, except maybe select!.

---

Put another way, I guess my feeling is that this should be applied to macro_rules! macros as long as it is within reason. (and that it currently is within reason for all stable macros)

[durka]

The most complex is probably thread_local!. I looked back at my PR that made it parse multiple declarations, and it looks like my initial draft would have accepted multiple semicolons, but the accepted version does not. Its docs rendering is... not the best but not bad for a macro I guess.

[ExpHP]

One other thought: the possibility of a future $()? syntax imo makes it even more desirable that the stdlib macros today do not accept infinite delimiters, so that upgrading them to use $()? is a backwards compatible change. (this upgrade is definitely desirable, considering that people are likely to look to the macros in std for reference)

(looking forward to that RFC, by the way 😉 )

[steveklabnik]

triage: @ExpHP how do you feel about this coverage today?

[ExpHP]

The coverage is still extremely poor.

Checking today, tests currently exist for double trailing commas in only two places in the grammar:

• Generic parameter lists
• #[cfg_attr()]'s support for multiple attributes. (I wouldn't know whether this logic is reused by other attributes, but I suspect possibly not since this needs to use commas to delimit arbitrary attributes (which can contain arbitrary token trees rather than just meta)?)

There's still a plethora of places in the grammar where accidental ,, support could slip in: Parameter lists, argument lists, enum/struct/union definitions, various kinds of types/patterns/literals, where bounds, and probably more.

One strategy may be to locate those test files which test interior ,,, and add cases to them with trailing ,,.

And of course, it still is the case that each individual std/core macro implements its own trailing comma support. $(,)? certainly has made life easier for macro authors, but it's still possible to make mistakes. I would like to update macro-comma-support-rpass.rs and friends eventually at some point to add trailing double comma tests. (perhaps I can do that today now that I'm thinking about it...)