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

Bad parse error on token sequences safe unsafe and unsafe safe #134580

Open
ionicmc-rs opened this issue Dec 20, 2024 · 15 comments
Open

Bad parse error on token sequences safe unsafe and unsafe safe #134580

ionicmc-rs opened this issue Dec 20, 2024 · 15 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST D-confusing Diagnostics: Confusing error or lint that should be reworked. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ionicmc-rs
Copy link

ionicmc-rs commented Dec 20, 2024

Code

// #1
unsafe extern {
    safe unsafe fn foo();
    unsafe safe fn bar();
}
// #2
unsafe safe fn baz() {}
safe unsafe fn qux() {}
// #3
unsafe safe extern "C" fn ham() {}
safe unsafe extern "C" fn egg() {}

Current output

# example #1:
`safe` must come before `unsafe`: `safe unsafe`
`unsafe` must come before `safe`: `unsafe safe`
# Example #2
items outside an `unsafe extern {...} block may not be annotated with `safe``
`safe` must come before `unsafe`: `safe unsafe`
`unsafe` must come before `safe`: `unsafe safe`
# example #3 is the same as #2

Desired output

# Example #1
error: `safe` and `unsafe` are incompatible
note: extern functions are unsafe by default
help: if you want to make this function safe, remove `unsafe`
# Example #2
error: items outside an `unsafe extern {...}` block may not be annotated with `safe`
help: remove `safe`
# example #3
error: items outside an `unsafe extern {...}` block may not be annotated with `safe`
note: extern functions are safe by default
help: remove `safe`

Rationale and extra context

look at the errors for #2 and #3:

`safe` must come before `unsafe`: `safe unsafe`
`unsafe` must come before `safe`: `unsafe safe`

this means that it thinks that the order is incorrect

which is likely because they are both included in the check for order

Other cases

// #1
unsafe safe static FOO: i32 = 42;
unsafe safe const BAR: i32 = 42;
unsafe safe trait Baz {}
unsafe trait _X {}
unsafe safe impl _X for i32 {}
// #2 
unsafe extern {
    unsafe safe static QUX: usize = 42;
    // the rest of the examples just copy pasted
}

Output:

# #1 
items outside an `unsafe extern` blocks cannot be `safe`
# shuffle safe and unsafe like usual
# #2 
# same as 1 but without the 'items outside `unsafe extern` ...'

Rust Version

$ rustc --version --verbose
rustc 1.83.0 (90b35a623 2024-11-26)
binary: rustc
commit-hash: 90b35a6239c3d8bdabc530a6a0816f7ff89a0aaf
commit-date: 2024-11-26
host: x86_64-unknown-linux-gnu
release: 1.83.0
LLVM version: 19.1.1

Anything else?

This is a follow up to #133586 and #133631

#133586 has the PR #133618
(all of the above are closed)

@ionicmc-rs ionicmc-rs added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 20, 2024
@ionicmc-rs

This comment has been minimized.

@rustbot rustbot added D-confusing Diagnostics: Confusing error or lint that should be reworked. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. labels Dec 20, 2024
@ionicmc-rs

This comment has been minimized.

@rustbot rustbot added A-edition-2024 Area: The 2024 edition A-parser Area: The parsing of Rust source code to an AST C-bug Category: This is a bug. labels Dec 20, 2024
@fmease fmease removed the C-bug Category: This is a bug. label Dec 20, 2024
@fmease fmease changed the title Safe Keyword incorrect Parsing in functions (follow up to #133586 and #133630) Bad parse error on token sequences safe unsafe and unsafe safe (before free (extern) fns and in extern blocks) Dec 20, 2024
@ionicmc-rs

This comment has been minimized.

@fmease

This comment has been minimized.

@ionicmc-rs

This comment has been minimized.

@fmease

This comment has been minimized.

@ionicmc-rs

This comment has been minimized.

@ionicmc-rs

This comment has been minimized.

@ionicmc-rs

This comment has been minimized.

@rustbot rustbot added D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. labels Dec 20, 2024
@ionicmc-rs

This comment has been minimized.

@rustbot

This comment has been minimized.

@ionicmc-rs

This comment has been minimized.

@rustbot rustbot added the E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label Dec 20, 2024
@fmease fmease changed the title Bad parse error on token sequences safe unsafe and unsafe safe (before free (extern) fns and in extern blocks) Bad parse error on token sequences safe unsafe and unsafe safe Dec 20, 2024
@compiler-errors compiler-errors removed the E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label Dec 20, 2024
@compiler-errors
Copy link
Member

compiler-errors commented Dec 20, 2024

@ionicmc-rs: E-* issue tags are used as call for participation, not priority. It's probably not wise to add them unless you can gauge the difficulty level from experience with the compiler. I would consider this to be difficult, but also E-hard is reserved for large changes that require mentorship, so it's probably best if you leave the tags alone in this issue.

This is just a diagnostic issue. As alluded to in the PR that attempted to fix a subset of this issue, this requires some delicate treatment around contextual/"soft" keywords because a bad implementation is likely to regress legitimate code if it's not aware of the grammatical implications of the recovery path.

Thanks for raising this issue, but it's also not going to get the issue fixed faster by trying to push on things.

@ionicmc-rs
Copy link
Author

@ionicmc-rs: E-* issue tags are used as call for participation, not priority. It's probably not wise to add them unless you can gauge the difficulty level from experience with the compiler. I would consider this to be difficult, but also E-hard is reserved for large changes that require mentorship, so it's probably best if you leave the tags alone in this issue.

This is just a diagnostic issue. As alluded to in the PR that attempted to fix a subset of this issue, this requires some delicate treatment around contextual/"soft" keywords because a bad implementation is likely to regress legitimate code if it's not aware of the grammatical implications of the recovery path.

Thanks for raising this issue, but it's also not going to get the issue fixed faster by trying to push on things.

Ok sorry for my mistake

@ionicmc-rs
Copy link
Author

Ok so it seems the issue is just with unsafe safe and safe unsafe and not the actual item

@compiler-errors compiler-errors removed the A-edition-2024 Area: The 2024 edition label Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST D-confusing Diagnostics: Confusing error or lint that should be reworked. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants