More unsafe attr verification
This code denies unsafe on attributes such as `#[test]` and `#[ignore]`, while also changing the `MetaItem` parsing so `unsafe` in args like `#[allow(unsafe(dead_code))]` is not accidentally allowed.
Tracking:
- https://github.com/rust-lang/rust/issues/123757
When collecting tokens there are two kinds of range:
- a range relative to the parser's full token stream (which we get when
we are parsing);
- a range relative to a single AST node's token stream (which we use
within `LazyAttrTokenStreamImpl` when replacing tokens).
These are currently both represented with `Range<u32>` and it's easy to
mix them up -- until now I hadn't properly understood the difference.
This commit introduces `ParserRange` and `NodeRange` to distinguish
them. This also requires splitting `ReplaceRange` in two, giving the new
types `ParserReplacement` and `NodeReplacement`. (These latter two names
reduce the overloading of the word "range".)
The commit also rewrites some comments to be clearer.
The end result is a little more verbose, but much clearer.
`parse_expr_assoc_with` has an awkward structure -- sometimes the lhs is
already parsed. This commit splits the post-lhs part into a new method
`parse_expr_assoc_rest_with`, which makes everything shorter and
simpler.
It has a single use. This makes the `let` handling case in
`parse_stmt_without_recovery` more similar to the statement path and
statement expression cases.
This makes it possible for the `unsafe(...)` syntax to only be
valid at the top level, and the `NestedMetaItem`s will automatically
reject `unsafe(...)`.
Mark `Parser::eat`/`check` methods as `#[must_use]`
These methods return a `bool`, but we probably should either use these values or explicitly throw them away (e.g. when we just want to unconditionally eat a token if it exists).
I changed a few places from `eat` to `expect`, but otherwise I tried to leave a comment explaining why the `eat` was okay.
This also adds a test for the `pattern_type!` macro, which used to silently accept a missing `is` token.
Add limit for unclosed delimiters in lexer diagnostic
Fixes#127868
The first commit shows the original diagnostic, and the second commit shows the changes.
improve error message when `global_asm!` uses `asm!` options
specifically, what was
error: expected one of `)`, `att_syntax`, or `raw`, found `preserves_flags`
--> $DIR/bad-options.rs:45:25
|
LL | global_asm!("", options(preserves_flags));
| ^^^^^^^^^^^^^^^ expected one of `)`, `att_syntax`, or `raw`
is now
error: the `preserves_flags` option cannot be used with `global_asm!`
--> $DIR/bad-options.rs:45:25
|
LL | global_asm!("", options(preserves_flags));
| ^^^^^^^^^^^^^^^ the `preserves_flags` option is not meaningful for global-scoped inline assembly
mirroring the phrasing of the [reference](https://doc.rust-lang.org/reference/inline-assembly.html#options).
This is also a bit of a refactor for a future `naked_asm!` macro (for use in `#[naked]` functions). Currently this sort of error can come up when switching from inline to global asm, or when a user just isn't that experienced with assembly. With `naked_asm!` added to the mix hitting this error is more likely.
Improve `extern "<abi>" unsafe fn()` error message
These errors were already reported in #87217, and fixed by #87235 but missed the case of an explicit ABI.
This PR does not cover multiple keywords like `extern "C" pub const unsafe fn()`, but I don't know what a good way to cover this would be. It also seems rarer than `extern "C" unsafe` which I saw happen a few times in workshops.
Remove unnecessary range replacements
This PR removes an unnecessary range replacement in `collect_tokens_trailing_token`, and does a couple of other small cleanups.
r? ````@petrochenkov````
A fully imperative style is easier to read than a half-iterator,
half-imperative style. Also, rename `inner_attr` as `attr` because it
might be an outer attribute.
Imagine you have replace ranges (2..20,X) and (5..15,Y), and these tokens:
```
a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x
```
If we replace (5..15,Y) first, then (2..20,X) we get this sequence
```
a,b,c,d,e,Y,_,_,_,_,_,_,_,_,_,p,q,r,s,t,u,v,w,x
a,b,X,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,u,v,w,x
```
which is what we want.
If we do it in the other order, we get this:
```
a,b,X,_,_,_,_,_,_,_,_,_,_,_,_,p,q,r,s,t,u,v,w,x
a,b,X,_,_,Y,_,_,_,_,_,_,_,_,_,_,_,_,_,_,u,v,w,x
```
which is wrong. So it's true that we need the `.rev()` but the comment
is wrong about why.
The current code is this:
```
self.capture_state.replace_ranges.push((start_pos..end_pos, Some(target)));
self.capture_state.replace_ranges.extend(inner_attr_replace_ranges);
```
What's not obvious is that every range in `inner_attr_replace_ranges`
must be a strict sub-range of `start_pos..end_pos`. Which means, in
`LazyAttrTokenStreamImpl::to_attr_token_stream`, they will be done
first, and then the `start_pos..end_pos` replacement will just overwrite
them. So they aren't needed.
This has been bugging me for a while. I find complex "if any of these
are true" conditions easier to think about than complex "if all of these
are true" conditions, because you can stop as soon as one is true.
Reorder trait bound modifiers *after* `for<...>` binder in trait bounds
This PR suggests changing the grammar of trait bounds from:
```
[CONSTNESS] [ASYNCNESS] [?] [BINDER] [TRAIT_PATH]
const async ? for<'a> Sized
```
to
```
([BINDER] [CONSTNESS] [ASYNCNESS] | [?]) [TRAIT_PATH]
```
i.e., either
```
? Sized
```
or
```
for<'a> const async Sized
```
(but not both)
### Why?
I think it's strange that the binder applies "more tightly" than the `?` trait polarity. This becomes even weirder when considering that we (or at least, I) want to have `async` trait bounds expressed like:
```
where T: for<'a> async Fn(&'a ()) -> i32,
```
and not:
```
where T: async for<'a> Fn(&'a ()) -> i32,
```
### Fallout
No crates on crater use this syntax, presumably because it's literally useless. This will require modifying the reference grammar, though.
### Alternatives
If this is not desirable, then we can alternatively keep parsing `for<'a>` after the `?` but deprecate it with either an FCW (or an immediate hard error), and begin parsing `for<'a>` *before* the `?`.
Adding details, clarifying lots of little things, etc. In particular,
the commit adds details of an example. I find this very helpful, because
it's taken me a long time to understand how this code works.
Currently in `collect_tokens_trailing_token`, `start_pos` and `end_pos`
are 1-indexed by `replace_ranges` is 0-indexed, which is really
confusing. Making them both 0-indexed makes debugging much easier.
The `Option`s within the `ReplaceRange`s within the hashmap are always
`None`. This PR omits them and inserts them when they are extracted from
the hashmap.
There are three places where we currently check `force_collect` and call
`collect_tokens_no_attrs` for `ForceCollect::Yes` and a vanilla parsing
function for `ForceCollect::No`.
But we can instead just pass in `force_collect` and let
`collect_tokens_trailing_token` do the appropriate thing.
Fix ICE in suggestion caused by `⩵` being recovered as `==`
The second suggestion shown here would previously incorrectly assume that the span corresponding to `⩵` was 2 bytes wide composed by 2 1 byte wide chars, so a span pointing at `==` could point only at one of the `=` to remove it. Instead, we now replace the whole thing (as we should have the whole time):
```
error: unknown start of token: \u{2a75}
--> $DIR/unicode-double-equals-recovery.rs:1:16
|
LL | const A: usize ⩵ 2;
| ^
|
help: Unicode character '⩵' (Two Consecutive Equals Signs) looks like '==' (Double Equals Sign), but it is not
|
LL | const A: usize == 2;
| ~~
error: unexpected `==`
--> $DIR/unicode-double-equals-recovery.rs:1:16
|
LL | const A: usize ⩵ 2;
| ^
|
help: try using `=` instead
|
LL | const A: usize = 2;
| ~
```
Fix#127823.
The second suggestion shown here would previously incorrectly assume that the span corresponding to `⩵` was 2 bytes wide composed by 2 1 byte wide chars, so a span pointing at `==` could point only at one of the `=` to remove it. Instead, we now replace the whole thing (as we should have the whole time):
```
error: unknown start of token: \u{2a75}
--> $DIR/unicode-double-equals-recovery.rs:1:16
|
LL | const A: usize ⩵ 2;
| ^
|
help: Unicode character '⩵' (Two Consecutive Equals Signs) looks like '==' (Double Equals Sign), but it is not
|
LL | const A: usize == 2;
| ~~
error: unexpected `==`
--> $DIR/unicode-double-equals-recovery.rs:1:16
|
LL | const A: usize ⩵ 2;
| ^
|
help: try using `=` instead
|
LL | const A: usize = 2;
| ~
```
Remove `TrailingToken`.
It's used in `Parser::collect_tokens_trailing_token` to decide whether to capture a trailing token. But the callers actually know whether to capture a trailing token, so it's simpler for them to just pass in a bool.
Also, the `TrailingToken::Gt` case was weird, because it didn't result in a trailing token being captured. It could have been subsumed by the `TrailingToken::MaybeComma` case, and it effectively is in the new code.
r? `@petrochenkov`
It's used in `Parser::collect_tokens_trailing_token` to decide whether
to capture a trailing token. But the callers actually know whether to
capture a trailing token, so it's simpler for them to just pass in a
bool.
Also, the `TrailingToken::Gt` case was weird, because it didn't result
in a trailing token being captured. It could have been subsumed by the
`TrailingToken::MaybeComma` case, and it effectively is in the new code.
More accurate span for anonymous argument suggestion
Use smaller span for suggesting adding `_:` ahead of a type:
```
error: expected one of `(`, `...`, `..=`, `..`, `::`, `:`, `{`, or `|`, found `)`
--> $DIR/anon-params-denied-2018.rs:12:47
|
LL | fn foo_with_qualified_path(<Bar as T>::Baz);
| ^ expected one of 8 possible tokens
|
= note: anonymous parameters are removed in the 2018 edition (see RFC 1685)
help: explicitly ignore the parameter name
|
LL | fn foo_with_qualified_path(_: <Bar as T>::Baz);
| ++
```
Some parser improvements
I was looking closely at attribute handling in the parser while debugging some issues relating to #124141, and found a few small improvements.
``@spastorino``
Use smaller span for suggesting adding `_:` ahead of a type:
```
error: expected one of `(`, `...`, `..=`, `..`, `::`, `:`, `{`, or `|`, found `)`
--> $DIR/anon-params-denied-2018.rs:12:47
|
LL | fn foo_with_qualified_path(<Bar as T>::Baz);
| ^ expected one of 8 possible tokens
|
= note: anonymous parameters are removed in the 2018 edition (see RFC 1685)
help: explicitly ignore the parameter name
|
LL | fn foo_with_qualified_path(_: <Bar as T>::Baz);
| ++
```
It only has two call sites, and it extremely similar to
`Parser::parse_expr_dot_or_call_with`, in both name and behaviour. The
only difference is the latter has an `attrs` argument and an
`ensure_sufficient_stack` call. We can pass in an empty `attrs` as
necessary, as is already done at some `parse_expr_dot_or_call_with` call
sites.
Make parse error suggestions verbose and fix spans
Go over all structured parser suggestions and make them verbose style.
When suggesting to add or remove delimiters, turn them into multiple suggestion parts.
Fix `DebugParser`.
I tried using this and it didn't work at all. `prev_token` is never eof, so the accumulator is always false, which means the `then_some` always returns `None`, which means `scan` always returns `None`, and `tokens` always ends up an empty vec. I'm not sure how this code was supposed to work.
(An aside: I find `Iterator::scan` to be a pretty wretched function, that produces code which is very hard to understand. Probably why this is just one of two uses of it in the entire compiler.)
This commit changes it to a simpler imperative style that produces a valid `tokens` vec.
r? `@workingjubilee`
Clear `inner_attr_ranges` regularly.
There's a comment saying we don't do it for performance reasons, but it doesn't actually affect performance.
The commit also tweaks the control flow, to make clearer that two code paths are mutually exclusive.
r? ````@petrochenkov````
It currently doesn't work at all. This commit changes it to a simpler
imperative style that produces a valid `tokens` vec.
(An aside: I find `Iterator::scan` to be a pretty wretched function,
that produces code which is very hard to understand. Probably why this
is just one of two uses of it in the entire compiler.)
That method is currently badly broken, and the test output reflects
this. The obtained tokens list is always empty, except in the case where
we go two `bump`s past the final token, whereupon it will produce as
many `Eof` tokens as asked for.
Fix `Parser::look_ahead`
`Parser::look_ahead` has a slow but simple general case, and a fast special case that is hit most of the time. But the special case is buggy and behaves differently to the general case. There are also no unit tests. This PR fixes all of this, resulting in a `Parser::look_ahead` that is equally fast, slightly simpler, more correct, and better tested.
r? `@davidtwco`
This new special case is simpler than the old special case because it
only is used when `dist == 1`. But that's still enough to cover ~98% of
cases. This results in equivalent performance to the old special case,
and identical behaviour as the general case.
The general case at the bottom of `look_ahead` is slow, because it
clones the token cursor. Above it there is a special case for
performance that is hit most of the time and avoids the cloning.
Unfortunately, its behaviour differs from the general case in two ways.
- When within a pair of delimiters, if you look any distance past the
closing delimiter you get the closing delimiter instead of what comes
after the closing delimiter.
- It uses `tree_cursor.look_ahead(dist - 1)` which totally confuses
tokens with token trees. This means that only the first token in a
token tree will be seen. E.g. in a sequence like `{ a }` the `a` and
`}` will be skipped over. Bad!
It's likely that these differences weren't noticed before now because
the use of `look_ahead` in the parser is limited to small distances and
relatively few contexts.
Removing the special case causes slowdowns up of to 2% on a range of
benchmarks. The next commit will add a new, correct special case to
regain that lost performance.
Go over all structured parser suggestions and make them verbose style.
When suggesting to add or remove delimiters, turn them into multiple suggestion parts.
The new condition is equivalent in practice, but it's much more obvious
that it would result in an empty range, because the condition lines up
with the contents of the iterator.
There's a comment saying we don't do it for performance reasons, but it
doesn't actually affect performance.
The commit also tweaks the control flow, to make clearer that two code
paths are mutually exclusive.
Currently the second element is a `Vec<(FlatToken, Spacing)>`. But the
vector always has zero or one elements, and the `FlatToken` is always
`FlatToken::AttrTarget` (which contains an `AttributesData`), and the
spacing is always `Alone`. So we can simplify it to
`Option<AttributesData>`.
An assertion in `to_attr_token_stream` can can also be removed, because
`new_tokens.len()` was always 0 or 1, which means than `range.len()`
is always greater than or equal to it, because `range.is_empty()` is
always false (as per the earlier assertion).
And update the comment. Clearly the return type of this function was
changed at some point in the past, but its name and comment weren't
updated to match.
The number of source code bytes can't exceed a `u32`'s range, so a token
position also can't. This reduces the size of `Parser` and
`LazyAttrTokenStreamImpl` by eight bytes each.
Move binder and polarity parsing into `parse_generic_ty_bound`
Let's pull out the parts of #127054 which just:
1. Make the parsing code less confusing
2. Fix `?use<>` (to correctly be denied)
3. Improve `T: for<'a> 'a` diagnostics
This should have no user-facing effects on stable parsing.
r? fmease
It currently goes one token too far.
Example: line 259 of `tests/ui/abi/compatibility.rs`:
```
test_abi_compatible!(fn_fn, fn(), fn(i32) -> i32);
```
This commit changes the span for the second element from `fn(),` to
`fn()`, i.e. removes the extraneous comma.
coverage: Overhaul validation of the `#[coverage(..)]` attribute
This PR makes sweeping changes to how the (currently-unstable) coverage attribute is validated:
- Multiple coverage attributes on the same item/expression are now treated as an error.
- The attribute must always be `#[coverage(off)]` or `#[coverage(on)]`, and the error messages for this are more consistent.
- A trailing comma is still allowed after off/on, since that's part of the normal attribute syntax.
- Some places that silently ignored a coverage attribute now produce an error instead.
- These cases were all clearly bugs.
- Some places that ignored a coverage attribute (with a warning) now produce an error instead.
- These were originally added as lints, but I don't think it makes much sense to knowingly allow new attributes to be used in meaningless places.
- Some of these errors might soon disappear, if it's easy to extend recursive coverage attributes to things like modules and impl blocks.
---
One of the goals of this PR is to lay a more solid foundation for making the coverage attribute recursive, so that it applies to all nested functions/closures instead of just the one it is directly attached to.
Fixes#126658.
This PR incorporates #126659, which adds more tests for validation of the coverage attribute.
`@rustbot` label +A-code-coverage
Special case when a code line only has multiline span starts
Minimize multline span overlap when there are multiple of them starting on the same line:
```
3 | X0 Y0 Z0
| _____^ - -
| | _______| |
| || _________|
4 | ||| X1 Y1 Z1
5 | ||| X2 Y2 Z2
| |||____^__-__- `Z` label
| ||_____|__|
| |______| `Y` is a good letter too
| `X` is a good letter
```
Add hard error and migration lint for unsafe attrs
More implementation work for https://github.com/rust-lang/rust/issues/123757
This adds the migration lint for unsafe attributes, as well as making it a hard error in Rust 2024.
Merge `PatParam`/`PatWithOr`, and `Expr`/`Expr2021`, for a few reasons.
- It's conceptually nice, because the two pattern kinds and the two
expression kinds are very similar.
- With expressions in particular, there are several places where both
expression kinds get the same treatment.
- It removes one unreachable match arm.
- Most importantly, for #124141 I will need to introduce a new type
`MetaVarKind` that is very similar to `NonterminalKind`, but records a
couple of extra fields for expression metavars. It's nicer to have a
single `MetaVarKind::Expr` expression variant to hold those extra
fields instead of duplicating them across two variants
`MetaVarKind::{Expr,Expr2021}`. And then it makes sense for patterns
to be treated the same way, and for `NonterminalKind` to also be
treated the same way.
I also clarified the comments, because I have long found them a little
hard to understand.
`StaticForeignItem` and `StaticItem` are the same
The struct `StaticItem` and `StaticForeignItem` are the same, so remove `StaticForeignItem`. Having them be separate is unique to `static` items -- unlike `ForeignItemKind::{Fn,TyAlias}`, which use the normal AST item.
r? ``@spastorino`` or ``@oli-obk``
Make edition dependent `:expr` macro fragment act like the edition-dependent `:pat` fragment does
Parse the `:expr` fragment as `:expr_2021` in editions <=2021, and as `:expr` in edition 2024. This is similar to how we parse `:pat` as `:pat_param` in edition <=2018 and `:pat_with_or` in >=2021, and means we can get rid of a span dependency from `nonterminal_may_begin_with`.
Specifically, this fixes a theoretical regression since the `expr_2021` macro fragment previously would allow `const {}` if the *caller* is edition 2024. This is inconsistent with the way that the `pat` macro fragment was upgraded, and also leads to surprising behavior when a macro *caller* crate upgrades to edtion 2024, since they may have parsing changes that they never asked for (with no way of opting out of it).
This PR also allows using `expr_2021` in all editions. Why was this was disallowed in the first place? It's purely additive, and also it's still feature gated?
r? ```@fmease``` ```@eholk``` cc ```@vincenzopalazzo```
cc #123865
Tracking:
- https://github.com/rust-lang/rust/issues/123742
Improve conflict marker recovery
<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.
This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using
r? <reviewer name>
-->
closes#113826
r? ```@estebank``` since you reviewed #115413
cc: ```@rben01``` since you opened up the issue in the first place
Properly gate `safe` keyword in pre-expansion
This PR gates `safe` keyword in pre-expansion contexts. Should mitigate the fallout of https://github.com/rust-lang/rust/issues/126755, which is that `safe` is now usable on beta lol.
r? `@spastorino` or `@oli-obk`
cc #124482 tracking #123743
Clean up some comments near `use` declarations
#125443 will reformat all `use` declarations in the repository. There are a few edge cases involving comments on `use` declarations that require care. This PR cleans up some clumsy comment cases, taking us a step closer to #125443 being able to merge.
r? ``@lqd``
We currently use `can_begin_literal_maybe_minus` in a couple of places
where only string literals are allowed. This commit introduces a
more specific function, which makes things clearer. It doesn't change
behaviour because the two functions affected (`is_unsafe_foreign_mod`
and `check_keyword_case`) are always followed by a call to `parse_abi`,
which checks again for a string literal.
It's clearer this way, because the `Interpolated` cases in
`can_begin_const_arg` and `is_pat_range_end_start` are more permissive
than the `Interpolated` cases in `can_begin_literal_maybe_minus`.
Fix duplicated attributes on nonterminal expressions
This PR fixes a long-standing bug (#86055) whereby expression attributes can be duplicated when expanded through declarative macros.
First, consider how items are parsed in declarative macros:
```
Items:
- parse_nonterminal
- parse_item(ForceCollect::Yes)
- parse_item_
- attrs = parse_outer_attributes
- parse_item_common(attrs)
- maybe_whole!
- collect_tokens_trailing_token
```
The important thing is that the parsing of outer attributes is outside token collection, so the item's tokens don't include the attributes. This is how it's supposed to be.
Now consider how expression are parsed in declarative macros:
```
Exprs:
- parse_nonterminal
- parse_expr_force_collect
- collect_tokens_no_attrs
- collect_tokens_trailing_token
- parse_expr
- parse_expr_res(None)
- parse_expr_assoc_with
- parse_expr_prefix
- parse_or_use_outer_attributes
- parse_expr_dot_or_call
```
The important thing is that the parsing of outer attributes is inside token collection, so the the expr's tokens do include the attributes, i.e. in `AttributesData::tokens`.
This PR fixes the bug by rearranging expression parsing to that outer attribute parsing happens outside of token collection. This requires a number of small refactorings because expression parsing is somewhat complicated. While doing so the PR makes the code a bit cleaner and simpler, by eliminating `parse_or_use_outer_attributes` and `Option<AttrWrapper>` arguments (in favour of the simpler `parse_outer_attributes` and `AttrWrapper` arguments), and simplifying `LhsExpr`.
r? `@petrochenkov`
It now parses outer attributes before collecting tokens. This avoids the
problem where the outer attribute tokens were being stored twice -- for
the attribute tokesn, and also for the expression tokens.
Fixes#86055.
