Parsing issues around qualifiers and modifiers

[fmease]

Fn vs. trait qualifiers

We fail to parse

const unsafe trait Trait {} //~ ERROR expected one of `extern` or `fn`, found keyword `trait`

and

const unsafe auto trait Trait {} //~ ERROR expected one of `extern` or `fn`, found `auto`

even though check_trait_front_matter would return true here in theory. That's because in parse_item_kind we check_fn_front_matter first which returns true once there are >=2 "fn" qualifiers (†) which most notably includes the sequence const unsafe. Two basic solutions:

• (†): check_fn_front_matter already contains certain exceptions like unsafe extern {, async gen { and async gen move {; we could extend this with !self.is_trait_front_matter() but that's kind of awful because we would basically check the sequence const unsafe three times over. Moreover, we gonna wanna have both is_* (&self) and check_* (&mut self) variants but that means bad duplication.
• We could move the check_trait_front_matter check above/before the fn one since it's more strict. However, I wonder if this breaks some soft invariants: check_trait_front_matter is also used elsewhere to determine whether a fn item follows. With this change, it would have "false positives". Likely not a problem in practice though.

Fn ptr ty qualifiers vs. trait bound modifiers in bare trait object tys

What's more, we also use check_fn_front_matter inside parse_ty_common for fn ptr tys but later reject const and async qualifiers in parse_fn_front_matter. Now, since that happens before we check can_begin_bound we incorrectly reject bare trait object tys that start with >=2 trait bound modifiers if they coincide with "fn" qualifiers (thanks to check_fn_front_matter yet again), i.e., async const bare trait object tys:

#![feature(const_trait_impl, async_trait_bounds)]

#[cfg(false)]
type X = (const Trait, async Trait, dyn const async Trait); // OK!

#[cfg(false)]
type Y = const async Trait;
//~^ ERROR an `fn` pointer type cannot be `const`
//~| ERROR an `fn` pointer type cannot be `async`
//~| ERROR expected one of `extern`, `fn`, `gen`, `safe`, or `unsafe`, found `Trait`

Array/slice ty vs. conditional constness inside bare trait object tys

For consistency, the following should get parsed as a bare trait object ty with a conditionally-const trait bound. However, we commit to parsing an array/slice ty before that:

#[cfg(false)] type X = dyn [const] Trait; // OK!

#[cfg(false)] type Y = [const] Trait; //~ ERROR expected identifier, found `]`

[fmease]

I can't lie, check_fn_front_matter is beyond scuffed and gnarly with its ">=2 qualifiers rule", just awful. It doesn't compose well and certain diags suffer, too. OTOH, while check_trait_front_matter is way stricter, it's also quite illegible, verbose and otherwise hard to maintain. Qualifier parsing is hell.

[fmease]

Technically speaking not specific to const_trait_impl but it's CTI that now suffers the consequences.

[estebank]

I think that the right strategy for qualifier parsing is to detect that there's any qualifier and then collect qualifiers in a loop, regardless of order, and then check the order after we get to the actual item. That would let us produce good diagnostics for any combination (including duplicates) and simplify the parsing logic.

[fmease]

I think that the right strategy for qualifier parsing is to detect that there's any qualifier and then collect qualifiers in a loop, regardless of order, and then check the order after we get to the actual item.

That would indeed be very nice but it's not that simple. const (but not const {) could start a const item, const fn or const trait (and we want to recover from const impl for impl const). const auto could be the start of a const auto trait or of a const item (e.g., const auto: () = ();), extern could be start of an extern fn, crate or a block, so we must interpret the context-sensitive keyword safe as a modifier without regressing safe as a normal identifier (for item macro calls or if we're in stmt ctxt for arbitrary exprs¹: safe!(), safe(), safe, safe + 1, …) and so on.

Footnotes

1. Well, we already fail to parse safe fn f() {} as an item if we're inside of a fn body which is bad; this just goes to show how awful this all is. ↩