Skip to content

Add targeted suggestion for incompatible {un,}safe keywords when parsing fn-frontmatter-like fragments #133618

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
+144 −4
Closed

Add targeted suggestion for incompatible {un,}safe keywords when parsing fn-frontmatter-like fragments #133618

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 17 additions & 4 deletions compiler/rustc_parse/src/parser/item.rs
View file
Original file line number Diff line number Diff line change
@@ -2620,6 +2620,8 @@ impl<'a> Parser<'a> {
enum WrongKw {
Duplicated(Span),
Misplaced(Span),
// E.g. `safe` *and* `unsafe`
Incompatible { first: Span, second: Span },
}

// We may be able to recover
@@ -2666,8 +2668,8 @@ impl<'a> Parser<'a> {
match safety {
Safety::Unsafe(sp) => Some(WrongKw::Duplicated(sp)),
Safety::Safe(sp) => {
recover_safety = Safety::Unsafe(self.token.span);
Some(WrongKw::Misplaced(sp))
// `safe unsafe` -- well which one is it?
Some(WrongKw::Incompatible { first: sp, second: self.token.span })
}
Safety::Default => {
recover_safety = Safety::Unsafe(self.token.span);
@@ -2678,8 +2680,8 @@ impl<'a> Parser<'a> {
match safety {
Safety::Safe(sp) => Some(WrongKw::Duplicated(sp)),
Safety::Unsafe(sp) => {
recover_safety = Safety::Safe(self.token.span);
Some(WrongKw::Misplaced(sp))
// `unsafe safe` -- well which one is it?
Some(WrongKw::Incompatible { first: sp, second: self.token.span })
}
Safety::Default => {
recover_safety = Safety::Safe(self.token.span);
@@ -2719,6 +2721,17 @@ impl<'a> Parser<'a> {
).note("keyword order for functions declaration is `pub`, `default`, `const`, `async`, `unsafe`, `extern`");
}
}
// The user wrote incompatible keywords like `safe unsafe` or `unsafe safe`
else if let Some(WrongKw::Incompatible { first, second }) = wrong_kw {
if let Ok(first_kw) = self.span_to_snippet(first)
&& let Ok(second_kw) = self.span_to_snippet(second)
{
Comment on lines +2724 to +2728
Copy link
Member Author

Choose a reason for hiding this comment

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

Remark: this is more of a band-aid fix, because this recovery code path is taken when we just so happen to be trying to parse anything that's fn-frontmatter-like that does not immediately include an fn keyword (as part of trying to parse item shaped things). So it's kinda best-effort anyway.

Copy link
Contributor

@ionicmc-rs ionicmc-rs Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

safe isnt supposed to be incompatible with unsafe in this context; safe is not valid in this context

safe unsafe extern {} // safe is only valid inside of this block
// Should give the err: safe is only valid inside of an unsafe extern block

so this error should be for the extern block

unsafe extern {
    unsafe safe fn foo() // Err: safe and unsafe are incompatible
}

this is atleast what i understand
to sum up:

safe /* unsafe */ extern { // err: safe is only valid inside of `unsafe extern` blocks
   // ...
}

unsafe extern {
    safe fn foo() -> i32;
    /* implicit unsafe */ fn bar() -> i32;
    unsafe safe fn x() -> i32; // err: safe and unsafe are incompatible
}
// Now foo can be called safely
println!("{}", foo()); // out: {some number, e.g. 42}
// but not bar
println!("{}", bar()) // err: this operation is unsafe

The desired output:

help: safe is only valid inside of `unsafe extern` blocks

error: safe and unsafe are incompatible # at x
help: if you want to mark this function as safe, remove unsafe

# ...

err.span_help(
first.to(second),
format!("`{first_kw}` and `{second_kw}` are incompatible, use only one of the keywords"),
);
}
}
// Recover incorrect visibility order such as `async pub`
else if self.check_keyword(kw::Pub) {
let sp = sp_start.to(self.prev_token.span);
31 changes: 31 additions & 0 deletions tests/ui/parser/incompatible-safe-unsafe-keywords-extern-block-1.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
//! Check that we don't try to suggest reordering incompatible keywords `safe` and `unsafe` when
//! parsing things that looks like fn frontmatter/extern blocks.
//!
//! # Context
//!
//! Previously, there was some recovery logic related to misplaced keywords (e.g. `safe` and
//! `unsafe`) when we tried to parse fn frontmatter (this is what happens when trying to parse
//! something malformed like `unsafe safe extern {}` or `safe unsafe extern {}`). Unfortunately, the
//! recovery logic only really handled duplicate keywords or misplaced keywords. This meant that
//! incompatible keywords like {`unsafe`, `safe`} when used together produces some funny suggestion
//! e.g.
//!
//! ```text
//! help: `unsafe` must come before `safe`: `unsafe safe`
//! ```
//!
//! and then if you applied that suggestion, another suggestion in the recovery logic will tell you
//! to flip it back, ad infinitum.
//!
//! # References
//!
//! See <https://github.com/rust-lang/rust/issues/133586>.
//!
//! See `incompatible-safe-unsafe-keywords-extern-block-2.rs` for the `safe unsafe extern {}`
//! version.
#![crate_type = "lib"]

safe unsafe extern {}
//~^ ERROR expected one of `extern` or `fn`, found keyword `unsafe`
//~| NOTE expected one of `extern` or `fn`
//~| HELP `safe` and `unsafe` are incompatible, use only one of the keywords
14 changes: 14 additions & 0 deletions tests/ui/parser/incompatible-safe-unsafe-keywords-extern-block-1.stderr
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: expected one of `extern` or `fn`, found keyword `unsafe`
--> $DIR/incompatible-safe-unsafe-keywords-extern-block-1.rs:28:6
|
LL | safe unsafe extern {}
| ^^^^^^ expected one of `extern` or `fn`
|
help: `safe` and `unsafe` are incompatible, use only one of the keywords
--> $DIR/incompatible-safe-unsafe-keywords-extern-block-1.rs:28:1
|
LL | safe unsafe extern {}
| ^^^^^^^^^^^

error: aborting due to 1 previous error

15 changes: 15 additions & 0 deletions tests/ui/parser/incompatible-safe-unsafe-keywords-extern-block-2.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
//! Check that we don't try to suggest reordering incompatible keywords `safe` and `unsafe` when
//! parsing things that looks like fn frontmatter/extern blocks.
//!
//! # References
//!
//! See <https://github.com/rust-lang/rust/issues/133586>.
//!
//! See `incompatible-safe-unsafe-keywords-extern-block-1.rs` for the `unsafe safqe extern {}`
//! version.
#![crate_type = "lib"]

unsafe safe extern {}
//~^ ERROR expected one of `extern` or `fn`, found `safe`
//~| NOTE expected one of `extern` or `fn`
//~| HELP `unsafe` and `safe` are incompatible, use only one of the keywords
14 changes: 14 additions & 0 deletions tests/ui/parser/incompatible-safe-unsafe-keywords-extern-block-2.stderr
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: expected one of `extern` or `fn`, found `safe`
--> $DIR/incompatible-safe-unsafe-keywords-extern-block-2.rs:12:8
|
LL | unsafe safe extern {}
| ^^^^ expected one of `extern` or `fn`
|
help: `unsafe` and `safe` are incompatible, use only one of the keywords
--> $DIR/incompatible-safe-unsafe-keywords-extern-block-2.rs:12:1
|
LL | unsafe safe extern {}
| ^^^^^^^^^^^

error: aborting due to 1 previous error

11 changes: 11 additions & 0 deletions tests/ui/parser/incompatible-safe-unsafe-keywords-extern-fn-in-extern-block.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//! Check that we emit a targeted suggestion for an extern fn that uses incompatible `safe` and
//! `unsafe` keywords.
#![crate_type = "lib"]

unsafe extern {
//~^ NOTE while parsing this item list starting here
pub safe unsafe extern fn foo() {}
//~^ ERROR expected one of `extern` or `fn`, found keyword `unsafe`
//~| NOTE expected one of `extern` or `fn`
//~| `safe` and `unsafe` are incompatible, use only one of the keywords
} //~ NOTE the item list ends here
20 changes: 20 additions & 0 deletions tests/ui/parser/incompatible-safe-unsafe-keywords-extern-fn-in-extern-block.stderr
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
error: expected one of `extern` or `fn`, found keyword `unsafe`
--> $DIR/incompatible-safe-unsafe-keywords-extern-fn-in-extern-block.rs:7:14
|
LL | unsafe extern {
| - while parsing this item list starting here
LL |
LL | pub safe unsafe extern fn foo() {}
| ^^^^^^ expected one of `extern` or `fn`
...
LL | }
| - the item list ends here
|
help: `safe` and `unsafe` are incompatible, use only one of the keywords
--> $DIR/incompatible-safe-unsafe-keywords-extern-fn-in-extern-block.rs:7:9
|
LL | pub safe unsafe extern fn foo() {}
| ^^^^^^^^^^^

error: aborting due to 1 previous error

8 changes: 8 additions & 0 deletions tests/ui/parser/incompatible-safe-unsafe-keywords-extern-fn.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
//! Check that we emit a targeted suggestion for an extern fn that uses incompatible `safe` and
//! `unsafe` keywords.
#![crate_type = "lib"]

pub safe unsafe extern fn foo() {}
//~^ ERROR expected one of `extern` or `fn`, found keyword `unsafe`
//~| NOTE expected one of `extern` or `fn`
//~| `safe` and `unsafe` are incompatible, use only one of the keywords
14 changes: 14 additions & 0 deletions tests/ui/parser/incompatible-safe-unsafe-keywords-extern-fn.stderr
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: expected one of `extern` or `fn`, found keyword `unsafe`
--> $DIR/incompatible-safe-unsafe-keywords-extern-fn.rs:5:10
|
LL | pub safe unsafe extern fn foo() {}
| ^^^^^^ expected one of `extern` or `fn`
|
help: `safe` and `unsafe` are incompatible, use only one of the keywords
--> $DIR/incompatible-safe-unsafe-keywords-extern-fn.rs:5:5
|
LL | pub safe unsafe extern fn foo() {}
| ^^^^^^^^^^^

error: aborting due to 1 previous error