Make missing_fragment_specifier an error in edition 2024

[tgross35]

missing_fragment_specifier has been a future compatibility warning since 2017. Uplifting it to an unconditional hard error was attempted in 2020, but eventually reverted due to fallout.

Make it an error only in edition >= 2024, leaving the lint for older editions. This change will make it easier to support more macro syntax that relies on usage of $.

Fixes #40107

---

It is rather late for the edition but since this change is relatively small, it seems worth at least bringing up. This follows a brief Zulip discussion (cc @tmandry).

Making this an edition-dependent lint has come up before but there was not a strong motivation. I am proposing it at this time because this would simplify the named macro capture groups RFC, which has had mildly positive response, and makes use of new $ syntax in the matcher. The proposed syntax currently parses as metavariables without a fragment specifier; this warning is raised, but there are no errors.

It is obviously not known that this specific RFC will eventually be accepted, but forbidding missing_fragment_specifier should make it easier to support any new syntax in the future that makes use of $ in different ways. The syntax conflict is also not impossible to overcome, but making it clear that unnamed metavariables are rejected makes things more straightforward and should allow for better diagnostics.

@Mark-Simulacrum suggested making this forbid-by-default instead of an error at #40107 (comment), but I don't think this would allow the same level of syntax flexibility.

It is also possible to reconsider making this an unconditional error since four years have elapsed since the previous attempt, but this seems likely to hit the same pitfalls. (Possibly worth a crater run?)

Nominating for discussion to get this on the radar.

r? @petrochenkov

Tracking:

• Tracking Issue for Rust 2024: Make missing_fragment_specifier an error #128143

[tgross35]

This PR is really just a proof of concept at this point to start discussion, needs more tests and maybe we can reuse the diagnostics somehow. And I am unsure why the error fires twice both for the lint (as noted in the FIXME that should be removed) and the diagnostic (petrochenkov I could use your guidance).

[petrochenkov]

I'd prefer to turn this into a hard error if possible.
Maybe make it FutureReleaseErrorReportInDeps first, if crater still finds too many errors after all these years.

[joshtriplett]

We discussed this in today's lang team meeting. We are in favor of making this change for the edition.

Independently, we'd also be in favor of confirming whether we can do it in past editions at this point (in case things have changed since 2020). That checking should not be a blocker for proceeding with making it a hard error for Rust 2024, though.

[traviscross]

@rustbot labels -needs-fcp -I-lang-nominated -S-waiting-on-team

We had 5/6 members present and this seemed a clear call. This is OK to proceed as far as lang is concerned (without FCP).

On the edition team side...

We've opened an edition tracking issue for this:

• Tracking Issue for Rust 2024: Make missing_fragment_specifier an error #128143

@tgross35: To be included in the edition, in addition to this PR, this will need a chapter added to the edition guide, and the Reference will need to be checked and a PR made for any updates that should happen there.

Also, will this cause an error, as desired, if someone using one of these fragments writes (in Rust 2021)?:

#![deny(rust_2024_compatibility)]

cc @ehuss

[tgross35]

Thank you for discussing this. It seems like it makes sense to move this PR forward, with a change to unconditional for crater coming as a followup since that isn't time-sensitive.

Also, will this cause an error, as desired, if someone using one of these fragments writes (in Rust 2021)?:

#![deny(rust_2024_compatibility)]

Is there a way to add a lint to multiple lint groups? It seems to lint on rust_2024_compatibility that the reason needs to be an EditionError, but it seems like we might not want to lose the seemingly more noisy (in a good way) FutureReleaseErrorReportInDeps. From #128122.

I don't know exactly what is needed to make the changes in this PR make sense, if they don't already. So:

@rustbot ready

[bors]

☔ The latest upstream changes (presumably #128155) made this pull request unmergeable. Please resolve the merge conflicts.

[ehuss]

Is there a way to add a lint to multiple lint groups?

Not easily AFAIK.

I don't recall us ever having something that needed to be both an edition lint and a future reporting lint.

I think this PR shouldn't be a problem in itself, and I don't think this needs a migration lint. The lint is already deny-by-default, and we generally expect that a project should be warning-free before starting the migration. I also don't see anyone in crates.io allowing it.

I'm assuming the intent here is that this PR is just a temporary change, since it looks like the intent is to make it a hard error in all editions. I do not know what criteria you will be using to make that change, since it looks like from #76605 there was a decent amount of regressions.

[traviscross]

OK. Per the reply from @ehuss, since this is already deny-by-default, this is OK from an edition perspective.

@petrochenkov: This is ready for your review and good to go both as far as lang and edition are concerned.

[tgross35]

I'm assuming the intent here is that this PR is just a temporary change, since it looks like the intent is to make it a hard error in all editions. I do not know what criteria you will be using to make that change, since it looks like from #76605 there was a decent amount of regressions.

As I am understanding it, the process will roughly be:

• Upgrade the error, report in dependencies to make it noisier (done, Mark missing_fragment_specifier as FutureReleaseErrorReportInDeps #128122).
• Merge this reasonably soon, with necessary changes, to give us the largest buffer possible before edition.
• Create a new PR that removes the lint and makes the error unconditional. Crater that. (edit: [crater] Make missing_fragment_specifier an unconditional error #128425)
• If breakage is considered acceptable (meaning significantly less than missing_fragment_specifier hard error #76605), go forward with merging that and cleaning up the codebase.
• If breakage is not acceptable, we will just have the edition-dependent lint. Maybe reevaluate again in another few years.

[petrochenkov]

This breaks edition hygiene and uses global psess.edition instead of span.edition(), so 2024 edition code won't be able to use old crates with missing metavar kinds.
But that should be fine if it's eventually going to turn into a hard error anyway.

[tgross35]

Changed to span.edition() to be safe.

[petrochenkov]

r=me after rebase.
@rustbot author

[tgross35]

Rebased, tests still pass after using span.edition(). Thanks for taking a look.

@bors r=petrochenkov

[bors]

📌 Commit 8c402f1 has been approved by petrochenkov

It is now in the queue for this repository.

[bors]

⌛ Testing commit 8c402f1 with merge 8fe0c75...

[bors]

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

[rust-timer]

Finished benchmarking commit (8fe0c75): 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 (primary -3.2%)

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)	-3.2%	[-3.2%, -3.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.2%	[-3.2%, -3.2%]	1

Cycles

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

Binary size

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

Bootstrap: 771.748s -> 772.472s (0.09%)
Artifact size: 329.08 MiB -> 329.05 MiB (-0.01%)