feat: Add profile-specific configuration for disallowed methods and types - #15779
Conversation
|
@blyxyas ping |
|
Hi @Lallapallooza, |
There was a problem hiding this comment.
I'll recount what I understand here, so a user uses a special configuration for settings profiles in their clippy.toml so any function can opt-in to those profiles, enabling warnings to those designated disallows in that function.
For example, one might want to disallow functions that may execute arbitrary code from a string input, into user-input-handling functions, and this is a way to achieve via Clippy warning of those uses.
Am I correct, have I forgotten/misunderstood something? If that's the case, we might be interested in generalizing this a bit so that both (1.) we can use profiles for other lints without the need of adding a special attribute for each lint, and (2.) people can use the general idea of profiles in Clippy to segment their source code for several lints at the same time.
Also, @ojeda could this be interesting for the Rust4Linux project? Should I give it priority?
|
You read it correctly. This PR introduces profile-scoped disallow lists in I agree that generalizing this is a good idea. I can follow up with a patch if that would be useful. Do you have any preferences for the design or the user interface (i.e., how Clippy users would enable and use profiles)? |
|
Thanks @blyxyas for the ping! It is an interesting idea. IIUC, currently this can only be done at a crate granularity, right? So it is essentially a way to make Clippy more granular -- I agree that generalizing it makes sense. I will try to think if we could use it in Rust for Linux (@Lallapallooza What are the use cases that motivated this? It may help to give some in the docs to inspire others -- thanks!). |
|
Hi @ojeda. My initial contribution was motivated by a need in my project to disallow host-side operations (e.g., any conversion from a device tensor to a host tensor) in specific context (e.g. dir crates/lib/cuda/) so that such code fails to compile. |
I'll ask both the entire team, and the style team to see if they have anything to say. In Clippy we don't really have an in-house attribute style guide, so maybe they have some feedback about how this could be implemented. |
|
Thanks @Lallapallooza, that is good to know (if you have more cases you can think of, then it would be great to hear about them, of course). |
This comment has been minimized.
This comment has been minimized.
|
Hi Lallapallooza, I've talked with the team and I'm going to proceed with the review. Also, sorry for the delayed response, I've just returned from vacation. I was thinking that this may be useful for this message thread
I thought that we already had a configuration option for this, but it seems that we don't have one. So I think that this feature can also support this use case. |
a8c3a91 to
0ff61e7
Compare
This comment has been minimized.
This comment has been minimized.
|
@blyxyas Thanks, I have rebased pr. |
I'm walking back this statement, we already have another pull request taking care of this. So we won't focus on that front here. |
|
Reminder, once the PR becomes ready for a review, use |
|
Hi! I have addressed all comments, could you please check again. |
| pub fn active_profiles(&mut self, cx: &LateContext<'_>, hir_id: HirId) -> Option<&ProfileSelection> { | ||
| if self.cache.contains_key(&hir_id) { | ||
| return self.cache.get(&hir_id).and_then(|selection| selection.as_ref()); | ||
| } |
There was a problem hiding this comment.
self.cache.contains_key is only self.cache.get().is_some(), so it's performing the same lookup two times. It should be something more akin to:
if let Some(selection) = self.cache.get(&hir_id) {
return selection;
}
There was a problem hiding this comment.
added a NOTE why it's needed
|
@blyxyas Hi, could you check my patch again please. |
This comment has been minimized.
This comment has been minimized.
- document `ProfileEntry` and `ProfileSelection` - explain why `profiles` and `known_profiles` are separate - emit unknown-profile warning inline (drop intermediate SmallVec) - add tests covering `#[expect]` before/after `#[clippy::disallowed_profile]` with an unknown profile name, for both methods and types lints - sort_by -> sort_by_key to satisfy dogfood
647bc06 to
03128d7
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@blyxyas Thanks! I rebased MR and now it's mergeable. |
|
Why did this not get an FCP? I remember there being a zulip thread quite a while ago that had some objections that got left hanging. |
|
I'll revert this pull request, I forgot about those objections (I'll look them up right now). I'm sorry for all nightly users that got an un-FCP'd feature in their nightly. |
|
The objection was more a vague vibes based objection, not any specific issues. Not an insurmountable thing in any way, but it should've been addressed before this was merged. |
|
This has now been reverted. Again, this isn't rejection of the feature in some form (or even the current form). The issue here is that this is a notable config change/lint extension which had some concerns raised which weren't addressed before it was merged. |
Thanks. Please let me know after the FCP if any changes are required from me. I’m happy to adjust the patch based on the requirements. |
…ds and types" (rust-lang#17251) Reverts rust-lang#15779 due to un-FCP'd concerns. This is wholy my fault. I didn't want to leave a user hanging and didn't do my due-diligence of getting all FCP concerns checked out and resolved. r? @Jarcho cc @Lallapallooza changelog: [`disallowed_methods`]: Revert profile-specific configuration changelog: [`disallowed_types`]: Revert profile-specific configuration
|
Can you open a new PR please. It makes tracking things easier if the FCP is linked to an open PR. |
|
View all comments
Add profile-scoped disallow lists for methods and types, wiring the new configuration tables through a shared resolver that can be toggled with #[clippy::disallowed_profile] attributes.
changelog: [disallowed_methods]: allow selecting per-scope disallow lists via disallowed-methods-profiles and the clippy::disallowed_profile attribute
changelog: [disallowed_types]: allow selecting per-scope disallow lists via disallowed-types-profiles and the clippy::disallowed_profile attribute