Skip to content

Commit

Permalink
Auto merge of #128006 - tgross35:missing-fragment-specifier-e2024, r=…
Browse files Browse the repository at this point in the history
…petrochenkov

Make `missing_fragment_specifier` an error in edition 2024

`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](https://rust-lang.zulipchat.com/#narrow/stream/268952-edition/topic/.60.20DBD.20-.3E.20hard.20error) (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](rust-lang/rfcs#3649) 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?)

Tracking:

- #128143
  • Loading branch information
bors committed Jul 27, 2024
2 parents 3942254 + 8c402f1 commit 8fe0c75
Show file tree
Hide file tree
Showing 7 changed files with 114 additions and 25 deletions.
5 changes: 5 additions & 0 deletions compiler/rustc_expand/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,11 @@ expand_meta_var_dif_seq_matchers = {$msg}
expand_meta_var_expr_unrecognized_var =
variable `{$key}` is not recognized in meta-variable expression
expand_missing_fragment_specifier = missing fragment specifier
.note = fragment specifiers must be specified in the 2024 edition
.suggestion_add_fragspec = try adding a specifier here
.valid = {$valid}
expand_module_circular =
circular modules: {$modules}
Expand Down
17 changes: 17 additions & 0 deletions compiler/rustc_expand/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,23 @@ pub struct DuplicateMatcherBinding {
pub prev: Span,
}

#[derive(Diagnostic)]
#[diag(expand_missing_fragment_specifier)]
#[note]
#[help(expand_valid)]
pub struct MissingFragmentSpecifier {
#[primary_span]
pub span: Span,
#[suggestion(
expand_suggestion_add_fragspec,
style = "verbose",
code = ":spec",
applicability = "maybe-incorrect"
)]
pub add_span: Span,
pub valid: &'static str,
}

#[derive(Diagnostic)]
#[diag(expand_invalid_fragment_specifier)]
#[help]
Expand Down
23 changes: 17 additions & 6 deletions compiler/rustc_expand/src/mbe/macro_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,16 @@ use rustc_errors::MultiSpan;
use rustc_lint_defs::BuiltinLintDiag;
use rustc_session::lint::builtin::{META_VARIABLE_MISUSE, MISSING_FRAGMENT_SPECIFIER};
use rustc_session::parse::ParseSess;
use rustc_span::edition::Edition;
use rustc_span::symbol::kw;
use rustc_span::{symbol::MacroRulesNormalizedIdent, ErrorGuaranteed, Span};

use smallvec::SmallVec;

use std::iter;

use super::quoted::VALID_FRAGMENT_NAMES_MSG_2021;

/// Stack represented as linked list.
///
/// Those are used for environments because they grow incrementally and are not mutable.
Expand Down Expand Up @@ -269,12 +272,20 @@ fn check_binders(
// FIXME: Report this as a hard error eventually and remove equivalent errors from
// `parse_tt_inner` and `nameize`. Until then the error may be reported twice, once
// as a hard error and then once as a buffered lint.
psess.buffer_lint(
MISSING_FRAGMENT_SPECIFIER,
span,
node_id,
BuiltinLintDiag::MissingFragmentSpecifier,
);
if span.edition() >= Edition::Edition2024 {
psess.dcx().emit_err(errors::MissingFragmentSpecifier {
span,
add_span: span.shrink_to_hi(),
valid: VALID_FRAGMENT_NAMES_MSG_2021,
});
} else {
psess.buffer_lint(
MISSING_FRAGMENT_SPECIFIER,
span,
node_id,
BuiltinLintDiag::MissingFragmentSpecifier,
);
}
}
if !macros.is_empty() {
psess.dcx().span_bug(span, "unexpected MetaVarDecl in nested lhs");
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_expand/src/mbe/quoted.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use rustc_span::Span;
const VALID_FRAGMENT_NAMES_MSG: &str = "valid fragment specifiers are \
`ident`, `block`, `stmt`, `expr`, `pat`, `ty`, `lifetime`, \
`literal`, `path`, `meta`, `tt`, `item` and `vis`";
const VALID_FRAGMENT_NAMES_MSG_2021: &str = "valid fragment specifiers are \
pub const VALID_FRAGMENT_NAMES_MSG_2021: &str = "valid fragment specifiers are \
`ident`, `block`, `stmt`, `expr`, `expr_2021`, `pat`, \
`ty`, `lifetime`, `literal`, `path`, `meta`, `tt`, \
`item` and `vis`";
Expand Down
Original file line number Diff line number Diff line change
@@ -1,25 +1,25 @@
error: missing fragment specifier
--> $DIR/macro-missing-fragment.rs:4:20
--> $DIR/macro-missing-fragment.rs:9:20
|
LL | ( $( any_token $field_rust_type )* ) => {};
| ^^^^^^^^^^^^^^^^

warning: missing fragment specifier
--> $DIR/macro-missing-fragment.rs:4:20
--> $DIR/macro-missing-fragment.rs:9:20
|
LL | ( $( any_token $field_rust_type )* ) => {};
| ^^^^^^^^^^^^^^^^
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #40107 <https://github.com/rust-lang/rust/issues/40107>
note: the lint level is defined here
--> $DIR/macro-missing-fragment.rs:1:9
--> $DIR/macro-missing-fragment.rs:6:9
|
LL | #![warn(missing_fragment_specifier)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: missing fragment specifier
--> $DIR/macro-missing-fragment.rs:12:7
--> $DIR/macro-missing-fragment.rs:19:7
|
LL | ( $name ) => {};
| ^^^^^
Expand All @@ -28,7 +28,7 @@ LL | ( $name ) => {};
= note: for more information, see issue #40107 <https://github.com/rust-lang/rust/issues/40107>

warning: missing fragment specifier
--> $DIR/macro-missing-fragment.rs:18:7
--> $DIR/macro-missing-fragment.rs:26:7
|
LL | ( $name ) => {};
| ^^^^^
Expand All @@ -40,45 +40,45 @@ error: aborting due to 1 previous error; 3 warnings emitted

Future incompatibility report: Future breakage diagnostic:
warning: missing fragment specifier
--> $DIR/macro-missing-fragment.rs:4:20
--> $DIR/macro-missing-fragment.rs:9:20
|
LL | ( $( any_token $field_rust_type )* ) => {};
| ^^^^^^^^^^^^^^^^
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #40107 <https://github.com/rust-lang/rust/issues/40107>
note: the lint level is defined here
--> $DIR/macro-missing-fragment.rs:1:9
--> $DIR/macro-missing-fragment.rs:6:9
|
LL | #![warn(missing_fragment_specifier)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^

Future breakage diagnostic:
warning: missing fragment specifier
--> $DIR/macro-missing-fragment.rs:12:7
--> $DIR/macro-missing-fragment.rs:19:7
|
LL | ( $name ) => {};
| ^^^^^
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #40107 <https://github.com/rust-lang/rust/issues/40107>
note: the lint level is defined here
--> $DIR/macro-missing-fragment.rs:1:9
--> $DIR/macro-missing-fragment.rs:6:9
|
LL | #![warn(missing_fragment_specifier)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^

Future breakage diagnostic:
warning: missing fragment specifier
--> $DIR/macro-missing-fragment.rs:18:7
--> $DIR/macro-missing-fragment.rs:26:7
|
LL | ( $name ) => {};
| ^^^^^
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #40107 <https://github.com/rust-lang/rust/issues/40107>
note: the lint level is defined here
--> $DIR/macro-missing-fragment.rs:1:9
--> $DIR/macro-missing-fragment.rs:6:9
|
LL | #![warn(missing_fragment_specifier)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
47 changes: 47 additions & 0 deletions tests/ui/macros/macro-missing-fragment.e2024.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
error: missing fragment specifier
--> $DIR/macro-missing-fragment.rs:9:20
|
LL | ( $( any_token $field_rust_type )* ) => {};
| ^^^^^^^^^^^^^^^^
|
= note: fragment specifiers must be specified in the 2024 edition
= help: valid fragment specifiers are `ident`, `block`, `stmt`, `expr`, `expr_2021`, `pat`, `ty`, `lifetime`, `literal`, `path`, `meta`, `tt`, `item` and `vis`
help: try adding a specifier here
|
LL | ( $( any_token $field_rust_type:spec )* ) => {};
| +++++

error: missing fragment specifier
--> $DIR/macro-missing-fragment.rs:19:7
|
LL | ( $name ) => {};
| ^^^^^
|
= note: fragment specifiers must be specified in the 2024 edition
= help: valid fragment specifiers are `ident`, `block`, `stmt`, `expr`, `expr_2021`, `pat`, `ty`, `lifetime`, `literal`, `path`, `meta`, `tt`, `item` and `vis`
help: try adding a specifier here
|
LL | ( $name:spec ) => {};
| +++++

error: missing fragment specifier
--> $DIR/macro-missing-fragment.rs:26:7
|
LL | ( $name ) => {};
| ^^^^^
|
= note: fragment specifiers must be specified in the 2024 edition
= help: valid fragment specifiers are `ident`, `block`, `stmt`, `expr`, `expr_2021`, `pat`, `ty`, `lifetime`, `literal`, `path`, `meta`, `tt`, `item` and `vis`
help: try adding a specifier here
|
LL | ( $name:spec ) => {};
| +++++

error: missing fragment specifier
--> $DIR/macro-missing-fragment.rs:9:20
|
LL | ( $( any_token $field_rust_type )* ) => {};
| ^^^^^^^^^^^^^^^^

error: aborting due to 4 previous errors

23 changes: 16 additions & 7 deletions tests/ui/macros/macro-missing-fragment.rs
Original file line number Diff line number Diff line change
@@ -1,23 +1,32 @@
//@ revisions: e2015 e2024
//@[e2015] edition:2015
//@[e2024] edition:2024
//@[e2024] compile-flags: -Zunstable-options

#![warn(missing_fragment_specifier)]

macro_rules! used_arm {
( $( any_token $field_rust_type )* ) => {};
//~^ ERROR missing fragment
//~| WARN missing fragment
//~| WARN this was previously accepted
//[e2015]~^ ERROR missing fragment
//[e2015]~| WARN missing fragment
//[e2015]~| WARN this was previously accepted
//[e2024]~^^^^ ERROR missing fragment
//[e2024]~| ERROR missing fragment
}

macro_rules! used_macro_unused_arm {
() => {};
( $name ) => {};
//~^ WARN missing fragment
//~| WARN this was previously accepted
//[e2015]~^ WARN missing fragment
//[e2015]~| WARN this was previously accepted
//[e2024]~^^^ ERROR missing fragment
}

macro_rules! unused_macro {
( $name ) => {};
//~^ WARN missing fragment
//~| WARN this was previously accepted
//[e2015]~^ WARN missing fragment
//[e2015]~| WARN this was previously accepted
//[e2024]~^^^ ERROR missing fragment
}

fn main() {
Expand Down

0 comments on commit 8fe0c75

Please sign in to comment.