Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorder trait bound modifiers *after* for<...> binder in trait bounds #127054

Merged
merged 6 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions compiler/rustc_ast_passes/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,6 @@ ast_passes_impl_trait_path = `impl Trait` is not allowed in path parameters
ast_passes_incompatible_features = `{$f1}` and `{$f2}` are incompatible, using them at the same time is not allowed
.help = remove one of these features

ast_passes_incompatible_trait_bound_modifiers = `{$left}` and `{$right}` are mutually exclusive

ast_passes_inherent_cannot_be = inherent impls cannot be {$annotation}
.because = {$annotation} because of this
.type = inherent impl for this type
Expand Down
11 changes: 0 additions & 11 deletions compiler/rustc_ast_passes/src/ast_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1435,17 +1435,6 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
};
self.dcx().emit_err(errors::TildeConstDisallowed { span, reason });
}
(
_,
BoundConstness::Always(_) | BoundConstness::Maybe(_),
BoundPolarity::Negative(_) | BoundPolarity::Maybe(_),
) => {
self.dcx().emit_err(errors::IncompatibleTraitBoundModifiers {
span: bound.span(),
left: modifiers.constness.as_str(),
right: modifiers.polarity.as_str(),
});
}
_ => {}
}

Expand Down
9 changes: 0 additions & 9 deletions compiler/rustc_ast_passes/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -657,15 +657,6 @@ pub enum TildeConstReason {
Item,
}

#[derive(Diagnostic)]
#[diag(ast_passes_incompatible_trait_bound_modifiers)]
pub struct IncompatibleTraitBoundModifiers {
#[primary_span]
pub span: Span,
pub left: &'static str,
pub right: &'static str,
}

#[derive(Diagnostic)]
#[diag(ast_passes_const_and_async)]
pub struct ConstAndAsync {
Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_parse/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ parse_bare_cr = {$double_quotes ->

parse_bare_cr_in_raw_string = bare CR not allowed in raw string

parse_binder_and_polarity = `for<...>` binder not allowed with `{$polarity}` trait polarity modifier
.label = there is not a well-defined meaning for a higher-ranked `{$polarity}` trait

parse_binder_before_modifiers = `for<...>` binder should be placed before trait bound modifiers
.label = place the `for<...>` binder before any modifiers

parse_bounds_not_allowed_on_trait_aliases = bounds are not allowed on trait aliases

parse_box_not_pat = expected pattern, found {$descr}
Expand Down Expand Up @@ -569,6 +575,9 @@ parse_missing_trait_in_trait_impl = missing trait in a trait impl
parse_modifier_lifetime = `{$modifier}` may only modify trait bounds, not lifetime bounds
.suggestion = remove the `{$modifier}`

parse_modifiers_and_polarity = `{$modifiers_concatenated}` trait not allowed with `{$polarity}` trait polarity modifier
.label = there is not a well-defined meaning for a `{$modifiers_concatenated} {$polarity}` trait

parse_more_than_one_char = character literal may only contain one codepoint
.followed_by = this `{$chr}` is followed by the combining {$len ->
[one] mark
Expand Down
30 changes: 30 additions & 0 deletions compiler/rustc_parse/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3041,3 +3041,33 @@ pub struct UnsafeAttrOutsideUnsafeSuggestion {
#[suggestion_part(code = ")")]
pub right: Span,
}

#[derive(Diagnostic)]
#[diag(parse_binder_before_modifiers)]
pub struct BinderBeforeModifiers {
#[primary_span]
pub binder_span: Span,
#[label]
pub modifiers_span: Span,
}

#[derive(Diagnostic)]
#[diag(parse_binder_and_polarity)]
pub struct BinderAndPolarity {
#[primary_span]
pub polarity_span: Span,
#[label]
pub binder_span: Span,
pub polarity: &'static str,
}

#[derive(Diagnostic)]
#[diag(parse_modifiers_and_polarity)]
pub struct PolarityAndModifiers {
#[primary_span]
pub polarity_span: Span,
#[label]
pub modifiers_span: Span,
pub polarity: &'static str,
pub modifiers_concatenated: String,
}
54 changes: 52 additions & 2 deletions compiler/rustc_parse/src/parser/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -930,6 +930,7 @@ impl<'a> Parser<'a> {
/// TRAIT_BOUND_MODIFIERS = [["~"] "const"] ["async"] ["?" | "!"]
/// ```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// TRAIT_BOUND_MODIFIERS = [["~"] "const"] ["async"] ["?" | "!"]
/// ```
/// CONSTNESS = [["~"] "const"]
/// ASYNCNESS = ["async"]
/// POLARITY = ["?" | "!"]
/// ```
///
/// See [`Self::parse_generic_ty_bound`] for the complete grammar of trait bound modifiers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yuup, the discrepancy between the grammar snippets & the recursive descent parser (not semantically only organizationally) is giving me the ick

Copy link
Member

@fmease fmease Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the EBNF snippets in rustc_parse are like my worst nightmare lol, switching between different styles (EBNF is an open family of dialects after all practically speaking) and being beyond outdated. I've been meaning to make everything consistent in one big PR for a while now, but er, not sure if it's worth my energy ^^'

fn parse_trait_bound_modifiers(&mut self) -> PResult<'a, TraitBoundModifiers> {
let modifier_lo = self.token.span;
let constness = if self.eat(&token::Tilde) {
let tilde = self.prev_token.span;
self.expect_keyword(kw::Const)?;
Expand Down Expand Up @@ -962,6 +963,7 @@ impl<'a> Parser<'a> {
} else {
BoundAsyncness::Normal
};
let modifier_hi = self.prev_token.span;

let polarity = if self.eat(&token::Question) {
BoundPolarity::Maybe(self.prev_token.span)
Expand All @@ -972,13 +974,40 @@ impl<'a> Parser<'a> {
BoundPolarity::Positive
};

// Enforce the mutual-exclusivity of `const`/`async` and `?`/`!`.
match polarity {
BoundPolarity::Positive => {
// All trait bound modifiers allowed to combine with positive polarity
}
BoundPolarity::Maybe(polarity_span) | BoundPolarity::Negative(polarity_span) => {
match (asyncness, constness) {
(BoundAsyncness::Normal, BoundConstness::Never) => {
// Ok, no modifiers.
}
(_, _) => {
let constness = constness.as_str();
let asyncness = asyncness.as_str();
let glue =
if !constness.is_empty() && !asyncness.is_empty() { " " } else { "" };
let modifiers_concatenated = format!("{constness}{glue}{asyncness}");
self.dcx().emit_err(errors::PolarityAndModifiers {
polarity_span,
polarity: polarity.as_str(),
modifiers_span: modifier_lo.to(modifier_hi),
modifiers_concatenated,
});
}
}
}
}

Ok(TraitBoundModifiers { constness, asyncness, polarity })
}

/// Parses a type bound according to:
/// ```ebnf
/// TY_BOUND = TY_BOUND_NOPAREN | (TY_BOUND_NOPAREN)
/// TY_BOUND_NOPAREN = [TRAIT_BOUND_MODIFIERS] [for<LT_PARAM_DEFS>] SIMPLE_PATH
/// TY_BOUND_NOPAREN = [for<LT_PARAM_DEFS>] [TRAIT_BOUND_MODIFIERS] SIMPLE_PATH
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// TY_BOUND_NOPAREN = [for<LT_PARAM_DEFS>] [TRAIT_BOUND_MODIFIERS] SIMPLE_PATH
/// TY_BOUND_NOPAREN = [for<GENERIC_PARAMS> CONSTNESS ASYNCNESS | POLARITY] SIMPLE_PATH

/// ```
///
/// For example, this grammar accepts `for<'a: 'b> ~const ?m::Trait<'a>`.
Expand All @@ -988,16 +1017,37 @@ impl<'a> Parser<'a> {
has_parens: bool,
leading_token: &Token,
) -> PResult<'a, GenericBound> {
let modifiers = self.parse_trait_bound_modifiers()?;
let (mut lifetime_defs, binder_span) = self.parse_late_bound_lifetime_defs()?;

let modifiers_lo = self.token.span;
let modifiers = self.parse_trait_bound_modifiers()?;
compiler-errors marked this conversation as resolved.
Show resolved Hide resolved
let modifiers_span = modifiers_lo.to(self.prev_token.span);

if let Some(binder_span) = binder_span {
match modifiers.polarity {
BoundPolarity::Negative(polarity_span) | BoundPolarity::Maybe(polarity_span) => {
self.dcx().emit_err(errors::BinderAndPolarity {
binder_span,
polarity_span,
polarity: modifiers.polarity.as_str(),
});
}
BoundPolarity::Positive => {}
}
}

// Recover erroneous lifetime bound with modifiers or binder.
// e.g. `T: for<'a> 'a` or `T: ~const 'a`.
if self.token.is_lifetime() {
let _: ErrorGuaranteed = self.error_lt_bound_with_modifiers(modifiers, binder_span);
return self.parse_generic_lt_bound(lo, has_parens);
}

if let (more_lifetime_defs, Some(binder_span)) = self.parse_late_bound_lifetime_defs()? {
lifetime_defs.extend(more_lifetime_defs);
self.dcx().emit_err(errors::BinderBeforeModifiers { binder_span, modifiers_span });
Copy link
Member Author

@compiler-errors compiler-errors Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also just make this into a warning if there's too much fallout 🤔

Checking crater for failures using an error, though.

}

let mut path = if self.token.is_keyword(kw::Fn)
&& self.look_ahead(1, |tok| tok.kind == TokenKind::OpenDelim(Delimiter::Parenthesis))
&& let Some(path) = self.recover_path_from_fn()
Expand Down
2 changes: 0 additions & 2 deletions src/tools/rustfmt/tests/source/type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,6 @@ trait T: ~ const Super {}

const fn not_quite_const<S: ~ const T>() -> i32 { <S as T>::CONST }

struct S<T:~ const ? Sized>(std::marker::PhantomData<T>);

impl ~ const T {}

fn apit(_: impl ~ const T) {}
Expand Down
6 changes: 0 additions & 6 deletions src/tools/rustfmt/tests/target/negative-bounds.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,3 @@ where
i32: !Copy,
{
}

fn maybe_const_negative()
where
i32: ~const !Copy,
{
}
2 changes: 0 additions & 2 deletions src/tools/rustfmt/tests/target/type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,6 @@ const fn not_quite_const<S: ~const T>() -> i32 {
<S as T>::CONST
}

struct S<T: ~const ?Sized>(std::marker::PhantomData<T>);

impl ~const T {}

fn apit(_: impl ~const T) {}
Expand Down
2 changes: 1 addition & 1 deletion src/tools/tidy/src/ui_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use std::path::{Path, PathBuf};
const ENTRY_LIMIT: u32 = 901;
// FIXME: The following limits should be reduced eventually.

const ISSUES_ENTRY_LIMIT: u32 = 1672;
const ISSUES_ENTRY_LIMIT: u32 = 1673;

const EXPECTED_TEST_FILE_EXTENSIONS: &[&str] = &[
"rs", // test source files
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/async-await/async-fn/higher-ranked-async-fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ async fn f(arg: &i32) {}

async fn func<F>(f: F)
where
F: async for<'a> Fn(&'a i32),
F: for<'a> async Fn(&'a i32),
{
let x: i32 = 0;
f(&x).await;
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/impl-trait/normalize-tait-in-const.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ mod foo {
}
use foo::*;

const fn with_positive<F: ~const for<'a> Fn(&'a Alias<'a>) + ~const Destruct>(fun: F) {
const fn with_positive<F: for<'a> ~const Fn(&'a Alias<'a>) + ~const Destruct>(fun: F) {
fun(filter_positive());
}

Expand Down
8 changes: 4 additions & 4 deletions tests/ui/impl-trait/normalize-tait-in-const.stderr
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
error: `~const` can only be applied to `#[const_trait]` traits
--> $DIR/normalize-tait-in-const.rs:27:42
|
LL | const fn with_positive<F: ~const for<'a> Fn(&'a Alias<'a>) + ~const Destruct>(fun: F) {
LL | const fn with_positive<F: for<'a> ~const Fn(&'a Alias<'a>) + ~const Destruct>(fun: F) {
| ^^^^^^^^^^^^^^^^^

error: `~const` can only be applied to `#[const_trait]` traits
--> $DIR/normalize-tait-in-const.rs:27:69
|
LL | const fn with_positive<F: ~const for<'a> Fn(&'a Alias<'a>) + ~const Destruct>(fun: F) {
LL | const fn with_positive<F: for<'a> ~const Fn(&'a Alias<'a>) + ~const Destruct>(fun: F) {
| ^^^^^^^^

error[E0015]: cannot call non-const closure in constant functions
Expand All @@ -19,7 +19,7 @@ LL | fun(filter_positive());
= note: calls in constant functions are limited to constant functions, tuple structs and tuple variants
help: consider further restricting this bound
|
LL | const fn with_positive<F: ~const for<'a> Fn(&'a Alias<'a>) + ~const Destruct + ~const Fn(&foo::Alias<'_>)>(fun: F) {
LL | const fn with_positive<F: for<'a> ~const Fn(&'a Alias<'a>) + ~const Destruct + ~const Fn(&foo::Alias<'_>)>(fun: F) {
| ++++++++++++++++++++++++++++
help: add `#![feature(effects)]` to the crate attributes to enable
|
Expand All @@ -29,7 +29,7 @@ LL + #![feature(effects)]
error[E0493]: destructor of `F` cannot be evaluated at compile-time
--> $DIR/normalize-tait-in-const.rs:27:79
|
LL | const fn with_positive<F: ~const for<'a> Fn(&'a Alias<'a>) + ~const Destruct>(fun: F) {
LL | const fn with_positive<F: for<'a> ~const Fn(&'a Alias<'a>) + ~const Destruct>(fun: F) {
| ^^^ the destructor for this type cannot be evaluated in constant functions
LL | fun(filter_positive());
LL | }
Expand Down
3 changes: 1 addition & 2 deletions tests/ui/issues/issue-39089.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
//@ check-pass
#![allow(dead_code)]
fn f<T: ?for<'a> Sized>() {}
//~^ ERROR `for<...>` binder should be placed before trait bound modifiers

fn main() {}
10 changes: 10 additions & 0 deletions tests/ui/issues/issue-39089.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: `for<...>` binder should be placed before trait bound modifiers
--> $DIR/issue-39089.rs:1:13
|
LL | fn f<T: ?for<'a> Sized>() {}
| - ^^^^
| |
| place the `for<...>` binder before any modifiers

error: aborting due to 1 previous error

15 changes: 13 additions & 2 deletions tests/ui/parser/bounds-type.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,30 @@
//@ compile-flags: -Z parse-only
//@ edition: 2021

struct S<
T: 'a + Tr, // OK
T: Tr + 'a, // OK
T: 'a, // OK
T:, // OK
T: ?for<'a> Trait, // OK
T: for<'a> ?Trait, //~ ERROR `for<...>` binder not allowed with `?` trait polarity modifier
T: Tr +, // OK
T: ?'a, //~ ERROR `?` may only modify trait bounds, not lifetime bounds

T: ~const Tr, // OK
T: ~const ?Tr, // OK
T: ~const ?Tr, //~ ERROR `~const` trait not allowed with `?` trait polarity modifier
T: ~const Tr + 'a, // OK
T: ~const 'a, //~ ERROR `~const` may only modify trait bounds, not lifetime bounds
T: const 'a, //~ ERROR `const` may only modify trait bounds, not lifetime bounds

T: async Tr, // OK
T: async ?Tr, //~ ERROR `async` trait not allowed with `?` trait polarity modifier
T: async Tr + 'a, // OK
T: async 'a, //~ ERROR `async` may only modify trait bounds, not lifetime bounds

T: const async Tr, // OK
T: const async ?Tr, //~ ERROR `const async` trait not allowed with `?` trait polarity modifier
T: const async Tr + 'a, // OK
T: const async 'a, //~ ERROR `const` may only modify trait bounds, not lifetime bounds
>;

fn main() {}
Loading
Loading