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

New lints: Safe items inside unsafe extern #975

Open
1 of 2 tasks
obi1kenobi opened this issue Oct 17, 2024 · 6 comments
Open
1 of 2 tasks

New lints: Safe items inside unsafe extern #975

obi1kenobi opened this issue Oct 17, 2024 · 6 comments
Labels
A-lint Area: new or existing lint E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.

Comments

@obi1kenobi
Copy link
Owner

obi1kenobi commented Oct 17, 2024

A new feature in Rust 1.82+ is the ability to define safe items inside an unsafe extern block:
https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safe-items-with-unsafe-extern

unsafe extern {
    pub safe static TAU: f64;
    pub safe fn sqrt(x: f64) -> f64;
    pub unsafe fn strlen(p: *const u8) -> usize;
}

Making a previously-safe item become unsafe is a major breaking change, since it now requires unsafe at the point of use.

At least two lints, and possibly more:

  • safe extern static became unsafe
  • safe extern fn became unsafe

I believe we'll also need additional schema in the adapter to model extern items and show which items are safe vs unsafe. This shouldn't be too difficult to add, though it's an extra step compared to "just writing a lint." For folks new to contributing to Trustfall, I'd recommend first writing another lint that doesn't need this extra step before moving to this issue.

@obi1kenobi obi1kenobi added A-lint Area: new or existing lint E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue. labels Oct 17, 2024
@aDotInTheVoid
Copy link

rustdoc-json doesn't model functions declared in extern blocks differently to normal functions: rust-lang/rust#131517. If you handle functions changing safety, then you handle extern functions as-well.

@aDotInTheVoid
Copy link

Relatedly, you probably want a lint for changing pub static X: i32 = 0; to pub static mut X: i32 = 0, as the latter is unsafe to access.

@obi1kenobi
Copy link
Owner Author

rustdoc-json doesn't model functions declared in extern blocks differently to normal functions: rust-lang/rust#131517. If you handle functions changing safety, then you handle extern functions as-well.

Neat, TIL -- thank you!

We might still want a separate lint for the two cases so we can offer a more specific diagnostic message in each case. I'll make a test crate and see what happens currently, and go from there.

Relatedly, you probably want a lint for changing pub static X: i32 = 0; to pub static mut X: i32 = 0, as the latter is unsafe to access.

Nice catch! I think it might already be on a to-do list somewhere, but I'll make sure of it :)

@obi1kenobi
Copy link
Owner Author

I decided the current lint for safe -> unsafe fn is fine here, and just added test cases here and in the adapter:
#1003
obi1kenobi/trustfall-rustdoc-adapter#611

Leaving this issue open until we can lint for safe -> unsafe transitions in extern static.

Thanks again @aDotInTheVoid!

@aDotInTheVoid
Copy link

For the extern static case, you'll need the (not yet merged) rust-lang/rust#133715 for rustdoc-json to have safety information for statics.

Also worth noting: static's are safe by default, so going from static X: i32 to extern { static X: i32 } is much more likely than unsafe extern { safe static X: i32 } to unsafe extern { unsafe static X: i32 }

@obi1kenobi
Copy link
Owner Author

Awesome, thanks for putting the PR together! I'll hold the next cargo-semver-checks release until that's merged and available in rustdoc-types so the new release can support the new format.

Also worth noting: static's are safe by default, so going from static X: i32 to extern { static X: i32 } is much more likely than unsafe extern { safe static X: i32 } to unsafe extern { unsafe static X: i32 }

Oh good call! We definitely want to lint for that.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Dec 2, 2024
…umeGomez

rustdoc-json: Include safety of `static`s

`static`s in an `extern` block can have an associated safety annotation ["because there is nothing guaranteeing that the bit pattern at the static’s memory is valid for the type it is declared with"](https://doc.rust-lang.org/reference/items/external-blocks.html#statics). Rustdoc already knows this and displays in for HTML. This PR also includes it in JSON.

Inspired by obi1kenobi/cargo-semver-checks#975 which needs this, but it's probably useful in other places.

r? `@GuillaumeGomez.` Possibly easier to review commit-by-commit.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 2, 2024
Rollup merge of rust-lang#133715 - aDotInTheVoid:rdj-static, r=GuillaumeGomez

rustdoc-json: Include safety of `static`s

`static`s in an `extern` block can have an associated safety annotation ["because there is nothing guaranteeing that the bit pattern at the static’s memory is valid for the type it is declared with"](https://doc.rust-lang.org/reference/items/external-blocks.html#statics). Rustdoc already knows this and displays in for HTML. This PR also includes it in JSON.

Inspired by obi1kenobi/cargo-semver-checks#975 which needs this, but it's probably useful in other places.

r? `@GuillaumeGomez.` Possibly easier to review commit-by-commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: new or existing lint E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.
Projects
None yet
Development

No branches or pull requests

2 participants