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

Add an allow-by-default lint against #[no_mangle] #72188

Closed
Nemo157 opened this issue May 14, 2020 · 9 comments · Fixed by #72209
Closed

Add an allow-by-default lint against #[no_mangle] #72188

Nemo157 opened this issue May 14, 2020 · 9 comments · Fixed by #72209
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Nemo157
Copy link
Member

Nemo157 commented May 14, 2020

Given that #[no_mangle] is unsafe there should be a lint that is capable of detecting it (there was a suggestion that unsafe_code should also detect it, but it might make sense to be able to lint it individually too).

@RalfJung
Copy link
Member

So maybe unsafe_code should be a lint group, and include this new lint?

I am suggesting that because I think deny(unsafe_code) should suffice to trigger the new lint.

@Nemo157
Copy link
Member Author

Nemo157 commented May 14, 2020

I've got the basic lint working now. @RalfJung do you think it's worth adding it as a separate lint, renaming unsafe_code to something else (which I can't think of a name for), and adding them both as lints in an unsafe_code group; or better to just integrate it into the current unsafe_code lint?

@RalfJung
Copy link
Member

I'll defer to @rust-lang/wg-diagnostics for that :D

@estebank
Copy link
Contributor

I am partial towards having a hard error when encountering multiple no_mangle functions with the same name in a given workspace, as originally suggested.

Adding this to unsafe_code might be problematic in the wild. Let's make a PR that rolls it into unsafe_code and see the behavior in the wild with a crater run, but if it causes too much fallout, lets make it an independent lint.

@estebank estebank added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 14, 2020
@Nemo157
Copy link
Member Author

Nemo157 commented May 14, 2020

There's other ways of using no_mangle which create UB than just overriding within a workspace, e.g. the example in geiger-rs/cargo-geiger#104. While adding a hard error for cases that can be detected would be nice, I don't think it's possible in general.

@jyn514 jyn514 added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Sep 15, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 15, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 15, 2020

Removing I-prioritize since this is already tracked at #28179

@jyn514 jyn514 removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 15, 2020
@bors bors closed this as completed in f8b330d Feb 9, 2021
@tarcieri
Copy link
Contributor

tarcieri commented Feb 24, 2021

Is there a tracking issue for additional lints like this to similar "unsafe attributes"? I think that'd be great!

@jyn514
Copy link
Member

jyn514 commented Feb 24, 2021

@tarcieri not that I know of but you could open one :)

@tarcieri
Copy link
Contributor

Done #82499

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants