[RFC-3086] Restrict the parsing of count

[c410-f3r]

Fix #111904

The original RFC didn't mention the possibility of using ${count(t,)} and such thing isn't very semantically accurate which can lead to confusion.

[rustbot]

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

[PatchMixolydic]

This change seems to be inconsistent with other comma separated lists in the language, which permit trailing commas. I believe ${count(t,)} should be allowed and should behave identically to ${count(t)} if at all possible.

[c410-f3r]

r? compiler

[compiler-errors]

Yeah, I agree with @PatchMixolydic here. Why is count(t,) being recovered to count(t, 0)? Is there a way we could get it to just act like count(t) and have this incongruency?

[c410-f3r]

AFAICT, count(t,) is being banned although it is possible to recover to count(t) which points to the outermost index that is equal or greater than 0 (#83527 (comment)).

There isn't a personal preference so, if desired, feel free to close this PR and #111904 to stick with the current behavior :)

[compiler-errors]

I may be misremembering, but I think petrochenkov reviewed previous RFC3086 work? Maybe you have more opinions about how to handle this and whether to error here.

r? petrochenkov

[petrochenkov]

This change seems to be inconsistent with other comma separated lists in the language, which permit trailing commas.

At which points something becomes a list though?
Standard library macros take it to absurd and consider even mac!(single_argument) a list, and therefore accept mac!(single_argument,) as well.

Neither ${count(t)}/${count(t, N)}, nor ${index(N)} or ${length(N)} are convincingly lists, so for now I think it's fine to report this as error instead of accepting things like ${count(t,)}, ${count(t, N,)}, ${index(N,)} and ${length(N,)}.

(But we could accept all the commas here too, just need to do it consistently.)

[Dylan-DPC]

@c410-f3r any updates on this?

[c410-f3r]

Sorry, life happened. I will return to this PR tomorrow.

[c410-f3r]

A little late but here it is

[petrochenkov]

@bors r+

[bors]

📌 Commit 6395dc2 has been approved by petrochenkov

It is now in the queue for this repository.

[bors]

⌛ Testing commit 6395dc2 with merge b9177c0...

[bors]

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing b9177c0 to master...

[rust-timer]

Finished benchmarking commit (b9177c0): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.9%	[0.8%, 2.9%]	2
Regressions ❌ (secondary)	2.3%	[2.0%, 2.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-3.1%, -3.1%]	1
All ❌✅ (primary)	1.9%	[0.8%, 2.9%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.4%	[-1.4%, -1.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.4%	[-1.4%, -1.4%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 634.503s -> 633.83s (-0.11%)
Artifact size: 346.93 MiB -> 347.06 MiB (0.04%)