-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 lint: danger_not_accepted
#11600
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Alexendoo (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged. You can start a rebase with the following commands:
The following commits are merge commits: |
I am having trouble getting the doc-tests to pass locally. After fixing a typo where I declared my module with Here are the offending snippets: #[clippy::dangerous(use_of_lib_1_dangerous_module)]
pub mod dangerous_module {
# fn break_the_program() {}
#[clippy::dangerous(may_break_program)]
pub fn do_something_innocuous_looking() {
break_the_program();
}
}
pub mod unsuspecting_module {
fn do_something() {
// This function call causes clippy to issue a warning
crate::dangerous_module::do_something_innocuous_looking();
}
} ...produces the error:
...and the snippet: #[clippy::dangerous(use_of_lib_1_dangerous_module)]
pub mod dangerous_module {
# fn break_the_program() {}
#[clippy::dangerous(may_break_program)]
pub fn do_something_innocuous_looking() {
break_the_program();
}
}
// This entire module can use functions with the danger `use_of_lib_1_dangerous_module`.
#[clippy::accept_danger(use_of_lib_1_dangerous_module)]
pub mod unsuspecting_module {
fn do_something() {
// Only this statement can call functions with the danger `may_break_program`.
#[clippy::accept_danger(may_break_program)]
crate::dangerous_module::do_something_innocuous_looking();
}
} ...produces the error:
These two snippets pass this fork of Clippy when checked on a standalone project so I'm not sure what's happening. |
The doctests don't get run on a local |
Nominating this for discussion in tomorrow's meeting (Tuesday, 15:00 UTC in https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy) since it's a novel lint |
Initial thoughts: I quite like the idea! Currently the closest to this is either making it an #[clippy::accept_danger(a)]
expands_to_smth_dangerous!(my_dangerous_function); Would apply the attribute to the macro invocation afaik, which is stripped during macro expansion, and thus is still linted. You have to apply the attribute to a block, which is really ugly, or ignore macros entirely (which would be valid, but imo is a bit silly). Another one: #[clippy::allow_dangerous(my_dangerous_function)]
my_dangerous_function() + 1; An attribute in this position is currently nightly-only (afaik because of ambiguities) and so you'd need a block to enclose this, too. Both of these are issues with allowing any lint but I think allowing a lint is rarer than, for example, I'm not sure how this would be solved. |
This is already possible with |
Whoops—that explanation was a bit ambiguous. What I meant by that post-MVP item was that, in much the same way that some users may wish to #[clippy::dangerous(d)]
mod whee {
pub fn woo() {}
}
#[clippy::forbid_danger(my_danger)]
mod i_am_super_safe {
pub fn waz() {
#[clippy::accept_danger(my_danger)] // This would be forbidden.
super::whee::woo();
}
} I could also see the benefit in forbidding users from accepting any more dangers beyond the ones declared by a similar attribute like Again, however, I'm under the impression that making too big an addition to the Rust programming language is frowned upon so I'm leaving these as post-MVP features that can be taken up later. |
Hmm... this second issue is somewhat difficult. Here are a few ideas! Like you said, the second issue only seems to be a problem in return positions and, luckily, fn main() {
#[clippy::accept_danger(whee)]
let foo = dangerous() + 3;
woo();
}
fn woo() -> u32 {
#[clippy::accept_danger(whee)]
return dangerous() + 4;
}
#[clippy::dangerous(whee)]
fn dangerous() -> u32 {
3
} ...which is admittedly a bit goofy. This still leaves sub-expressions and the return-position of blocks as potential pain points (I mean, you could Another thing a user (or we!) could do is expose a macro to accept dangers in expression positions: macro_rules! accept_danger {
($($allow:ident),*$(,)? ; $expr:expr) => {{
#[clippy::accept_danger($($allow)*)]
let res = { $expr };
res
}};
}
fn main() {
let foo = {
println!("Hi!");
accept_danger!(waz; dangerous() + 3)
};
}
#[clippy::dangerous(waz)]
fn dangerous() -> i32 {
4
} I'm not a massive fan of this idea: Clippy is supposed to be a linter and it would be a bit weird to expose something that's not an attribute. I think the best solution in the long-term would be to wait for Rust to add support for at least marking block expressions with attributes. That way, the user could type: fn main() {
let foo = {
println!("Hi!");
#[clippy::accept_danger(waz)] {
dangerous() + 3
}
};
}
#[clippy::dangerous(waz)]
fn dangerous() -> i32 {
4
} ...which looks pretty close to an Alternatively, users could just mark their entire function as accepting a danger but I don't know how well that will signal the dangers. |
Isn't this already possible? This is what you usually do to allow a lint for a macro (though rustfmt moves the opening bracket onto its own line; which, honestly, I hate). I'm a bit unsure what you mean by "only happening in return positions", as an attribute there is perfectly fine: fn awa() {
#[allow(warnings)]
()
} The reason why it didn't work there was because it could either be applied to the function call or the addition. The parser currently goes with the function call with Relevant rustfmt issue that shows what the parser does here: rust-lang/rustfmt#5873 I do agree that clippy shouldn't expose a macro like this, and I'm unsure how it would even work. I see what you mean now about forbidding specific dangers. The reverse ( |
fn awa() -> () {
#[clippy::msrv = ":)"] // attributes on expressions are experimental
()
} It may be worthwhile seeing if we can get fn woo() -> i32 {
id(
#[clippy::accept_danger(my_danger)]
(dangerous() + 1),
)
}
#[clippy::dangerous(my_danger)]
fn dangerous() -> i32 {
3
}
fn id<T>(v: T) -> T {
v
} ...which feels pretty convenient to me. |
Huh, TIL From the meeting we'd like for it to have a human readable message for more detailed error messages similar to We also discussed that it should work across crate boundaries, but it looks like that should already work, I misread it as using a HIR visitor for both attributes initially |
re: the attributes, yeah some of those are unfortunate but it's still the only real option for syntax. Some of those issues like the macro expansion one are shared with |
This should be implemented now for |
tests/ui/danger_not_accepted.rs
Outdated
// Edge case attr tests | ||
#[rustfmt::skip] | ||
#[clippy::dangerous(whee, woo,)] | ||
#[clippy::dangerous(whee, woo, reason = "sdfhsdf",)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the reason should be per danger, i.e. #[clippy::dangerous(foo = "reason")]
Bikeshedding a bit, what about something like #[clippy::danger::foo = "reason"]
? It's more similar to #[must_use]
that way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I like the #[clippy::dangerous(a, b, reason = "foo")]
syntax because it allows you to share the same reason among multiple dangers. For example, foo_internal_api
and raw_foo_access
could share the reason "low level building block; use bar instead." If you want several different reasons, you could always write it as:
#[clippy::dangerous(foo, reason = "reason 1")]
#[clippy::dangerous(bar, reason = "reason 2")]
fn baz() {}
However, I do see the verbosity benefit of your solution compared to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I don't think it will be that common to want to use multiple dangers on the same node, particularly so where the reason can be the same for all of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relatedly there was discussion about whether or not the reason should be required, I don't think we fully came to an agreement on that but initially let's make it required, it can be made optional later if we want that but not the other way around
What if two crates use the same ident? Should they be namespaced to the crate? |
I don't think they should have to be namespaced to a crate since some dangers may be quite common (e.g. |
Sure, supporting |
☔ The latest upstream changes (presumably #11791) made this pull request unmergeable. Please resolve the merge conflicts. |
Sorry for the long wait. Danger identifiers can now take the form of paths and reasons are mandatory for |
Did the |
Hey @Radbuglet , this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation? If you have any questions, you're always welcome to ask them in this PR or on Zulip. @rustbot author |
Whoops. I meant to respond to this thread but forgot. Sorry! Yes, the The following, however, are not implemented:
|
Bumping this PR a bit... are there any additional changes I should make to this PR? My previous comment includes a list of incomplete things I remember being mentioned. |
Yeah that's fine, the PR itself is still using the old syntax though
What do you mean by namespace?
Let's start conservative and have it be direct only like |
Huh? The line you shared uses the new syntax: #[clippy::dangerous(may_deadlock = "your program may deadlock in calling this function")]
pub fn woo2() {} The new syntax is: fn caller() {
#[clippy::accept_danger(path::to::danger, reason = "this part is optional")]
callee();
}
#[clippy::dangerous(path::to::danger = "why it's dangerous")]
fn callee() {
}
I think it might be useful to be able to accept entire classes of dangers. For example: #[clippy::accept_danger(my_lib, reason = "we know what we're doing")]
mod trusted {
fn whee() {
super::func_1();
super::func_2();
}
}
#[clippy::dangerous(my_lib::func1 = "reason 1")]
fn func_1() { ... }
#[clippy::dangerous(my_lib::func2 = "reason 2")]
fn func_2() { ... } However, if this is out-of-scope for an initial implementation, we can skip it.
Sounds good! |
Ah I see the confusion, the intention was to have the danger be part of the attr path e.g. #[clippy::danger::may_deadlock = "your program may deadlock in calling this function"]
pub fn woo2() {} We could accept paths on top of just idents, but it may be worth avoiding so people don't feel the need to add namespaces everywhere |
Hey @Radbuglet , this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation? If you have any questions, you're always welcome to ask them in this PR or on Zulip. @rustbot author |
☔ The latest upstream changes (presumably #11700) made this pull request unmergeable. Please resolve the merge conflicts. |
Hey this is triage, I'm closing this due to inactivity. If you want to continue this implementation, you're welcome to create a new PR. Thank you for the time, you already put into this! Interested parties are welcome to pick this implementation up as well :) @rustbot label +S-inactive-closed -S-waiting-on-author -S-waiting-on-review |
Adds the
danger_not_accepted
lint:The following features have been deemed as future improvements and have not been included in this MVP:
accept_danger
in a given module.accept_danger
anddangerous
to be used as inner attributes on stable Rust.changelog: new lint:
danger_not_accepted