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