Two improvements to disallowed_*
#13669
Conversation
rustbot
added
the
S-waiting-on-review
| @@ -1,5 +1,3 @@ | |||
| //@compile-flags: --crate-name conf_disallowed_methods | |||
There was a problem hiding this comment.
Is there any reason for this being here? I checked others in ui-toml and quite a few have this. I vaguely remember about this, but I don't have the specifics...
There was a problem hiding this comment.
Is there any reason for this being here?
I assumed it was some kind of a leftover, since conf_disallowed_methods is the name that ui_test chooses for the test crate. (Aside: with the EDIT: Mistake.)compile-flags in place, ui_test cannot test the fix.
1e1ac2b appears to be the commit that introduced the original directive. @Alexendoo do you recall why that was?
| @@ -14,4 +14,5 @@ disallowed-methods = [ | |||
| "conf_disallowed_methods::Struct::method", | |||
| "conf_disallowed_methods::Trait::provided_method", | |||
| "conf_disallowed_methods::Trait::implemented_method", | |||
| "conf_disallowed_methods :: path :: with :: spaces", | |||
There was a problem hiding this comment.
Not that I know of. I assumed not allowing spaces was an oversight (but maybe I'm missing something).
There was a problem hiding this comment.
I see now that the .split("::") pattern appears many places, and that a larger fix is required. I will back this change out for now.
| span, | ||
| self.reason().map_or_else(|| String::from("use"), ToOwned::to_owned), | ||
| replacement, | ||
| Applicability::MachineApplicable, |
There was a problem hiding this comment.
I'm not fully sure this is the right choice, I would be curious first to hear what others have to say. It's not guaranteed to be machine applicable but it is something the user intends to always be replaced and it's also at the user's discretion to provide something valid, and it could get annoying. The majority of the improvement does come from the fact that it can be applied automatically after all.
There was a problem hiding this comment.
...it's also at the user's discretion to provide something valid...The majority of the improvement does come from the fact that it can be applied automatically after all.
This was precisely my reasoning.
There was a problem hiding this comment.
It's sound. I doubt others will disagree but we'll see regardless
| @@ -173,7 +174,7 @@ declare_clippy_lint! { | |||
| impl_lint_pass!(AwaitHolding => [AWAIT_HOLDING_LOCK, AWAIT_HOLDING_REFCELL_REF, AWAIT_HOLDING_INVALID_TYPE]); | |||
|
|
|||
| pub struct AwaitHolding { | |||
| def_ids: DefIdMap<(&'static str, Option<&'static str>)>, | |||
| def_ids: DefIdMap<(&'static str, &'static DisallowedPath)>, | |||
There was a problem hiding this comment.
I don't think we should allow specifying a replacement here, even if it's never used. Also the reasoning should usually be inferred due to it being a specific lint, right?
There was a problem hiding this comment.
I see your point. I'll work on a fix.
There was a problem hiding this comment.
My solution may be over-engineered. Other alternatives I considered were:
- Use
serde-untagged, but it is not in the list of permitted dependencies. - Implement a custom visitor like
serde-untaggeddoes, but that seemed yet more complicated. - Don't error during deserialization and instead warn when the replacement is discovered.
I'm open to suggestions if another approach is desired.
smoelius
force-pushed
the
disallowed-methods
branch
from
1c5ef1b
to
f3401bd
Compare
smoelius
changed the title
disallowed_methodsdisallowed_*
The improvements are as follows:
compile-flagsdirective intests/ui-toml/toml_disallowed_methods/conf_disallowed_methods.rsAllow paths to contain spaces inclippy.toml(e.g.,"path :: with :: spaces")changelog: support replacements in
disallowed_methods