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

implement disallowed trait methods #12194

Closed

Conversation

Emilgardis
Copy link

resolves #8581

changelog: [disallowed_methods]: Allow specific trait impls via qualified call syntax to be linted.

@rustbot
Copy link
Collaborator

rustbot commented Jan 24, 2024

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 (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 24, 2024
@Emilgardis Emilgardis marked this pull request as draft January 24, 2024 13:39
@Emilgardis Emilgardis force-pushed the disallowed_trait_method branch 2 times, most recently from a68178c to a9c80e9 Compare January 24, 2024 13:43
continue;
};
let self_segs: Vec<_> = self_ty.split("::").collect();
let self_ress: Vec<_> = clippy_utils::def_path_res(cx, &self_segs);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this does not support things like <[T] as U>::u_method, <&T as U>::u_method etc due to how the lookup in def_path_res works

Comment on lines 134 to 149
let self_res: Res<rustc_hir::HirId> = match self_ty.kind() {
TyKind::Bool | TyKind::Char | TyKind::Int(_) | TyKind::Uint(_) | TyKind::Float(_) => {
Res::PrimTy(PrimTy::from_name(self_ty.primitive_symbol().unwrap()).unwrap())
},
TyKind::Str => Res::PrimTy(PrimTy::Str),
TyKind::Adt(adt, _) => Res::Def(
match adt.adt_kind() {
AdtKind::Struct => rustc_hir::def::DefKind::Struct,
AdtKind::Union => rustc_hir::def::DefKind::Union,
AdtKind::Enum => rustc_hir::def::DefKind::Enum,
},
adt.did(),
),
// FIXME: these other kinds are not currently supported by disallowed_methods due to how
// def_path_ref is implemented
_ => return,
};
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope there is a better way to do this.

@Emilgardis Emilgardis force-pushed the disallowed_trait_method branch from a9c80e9 to fd7ec8c Compare January 24, 2024 13:51
@Emilgardis Emilgardis force-pushed the disallowed_trait_method branch from fd7ec8c to c2b94ec Compare January 24, 2024 14:03
@Emilgardis
Copy link
Author

Emilgardis commented Jan 24, 2024

I want to support <T as std::convert::Into<U>>::into also, but with the current parsing of the qualified path i'm not sure how to do that without just implementing parse_qpath

@Alexendoo
Copy link
Member

What are your thoughts on this @flip1995? IIRC we didn't want to extend the capabilities of our path parsing too much

@flip1995
Copy link
Member

flip1995 commented Feb 7, 2024

I already dislike the parsing code like it is implemented in this PR. I.e. the split on >::. What if a user wants to put this inside the <>? parse_qpath has handling logic to find the correct >. Also what if someone were to write > :: for whatever reason? Parsing a string as a string instead of tokens just leads to problems.

IIUC the goal here is to specify that a certain trait method should not be allowed on a certain type? Why not invent our own config syntax for this. You could also configure this as

"std::string::String : std::default::Default::default"

meaning: On the type String, you can't call Default::default. Or disallowing Default::default on every type:

"std::default::Default::default"

Or go crazy with the toml parsing:

disallowed_methods = [{ type = "std::string::String", method = "std::default::Default::default" }, "my_crate::my_method" ]

But I wouldn't go crazy with the path parsing, as we can't cover everything without completely copying parse_qpath.

@bors
Copy link
Contributor

bors commented Feb 17, 2024

☔ The latest upstream changes (presumably #12198) made this pull request unmergeable. Please resolve the merge conflicts.

@xFrednet
Copy link
Member

xFrednet commented Apr 1, 2024

Hey @Emilgardis, 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

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Apr 1, 2024
@Emilgardis
Copy link
Author

Yes, I'm going to continue this eventually!

@xFrednet
Copy link
Member

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

@xFrednet xFrednet closed this Jun 20, 2024
@rustbot rustbot added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive-closed Status: Closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

disallowed_methods on trait impls
6 participants