Skip to content

[Draft] Add direct_method_call lint #9937

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

Closed
wants to merge 18 commits into from
Closed

[Draft] Add direct_method_call lint #9937

wants to merge 18 commits into from

Conversation

blyxyas
Copy link
Member

@blyxyas blyxyas commented Nov 24, 2022

This is a draft just to get some comments, currently working on it.

TODOs

  • Implement non-local functions
    Lint activated for all function calls.

  • Clearer error messages (Current: this is poorly readable)
    'this is poorly readable' is a message that could be applied to almost all lints.

  • Better test
    A better test/ui/direct_method_call.rs will be created after the first two issues are solved.

  • Add configuration
    The user can choose if they want to ignore an explicit module use in clippy.toml.

  • Manage ambigous types
    If two or more traits can handle an input, ignore the function call (It would be ambigious).

Possible Solution
Check if any trait other than X has a method Y. If There's a A::Y, ignore (maybe warn?)

  • Manage type inference
    A trait can infer types from a numeric value. e.g f32::floor(3.5) means that 3.5 is a f32 value.

Possible solution
Track the input type that the method receives, and convert the numeric value to that type if possible.
For example:

impl X for T {
    fn Y(self) { ... }
          ^
          self is type T (a numeric type)
}
X::Y(5.5) ==> (5.5_T).Y()

fixes #9789
changelog: [direct_method_call]: Add direct_method_call lint

@rust-highfive
Copy link

r? @Alexendoo

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 24, 2022
@bors
Copy link
Contributor

bors commented Nov 25, 2022

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

@Alexendoo
Copy link
Member

Something that makes this difficult is that the non-method call syntax can guide type inference, e.g.

// This compiles
f32::floor(1.5);

// This will fail to compile
(1.5).floor();

// error[E0689]: can't call method `floor` on ambiguous numeric type `{float}`
//  --> src/lib.rs:2:7
//   |
// 2 | (1.5).floor()
//   |       ^^^^^
//   |
// help: you must specify a concrete type for this numeric value, like `f32`
//   |
// 2 | (1.5_f32).floor()
//   |  ~~~~~~~

@blyxyas
Copy link
Member Author

blyxyas commented Nov 27, 2022

Very true, thanks for warning about this, going to add this to the list of requisites.

@bors
Copy link
Contributor

bors commented Nov 27, 2022

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

@Alexendoo
Copy link
Member

A non numeric example:

use std::net::IpAddr;

fn direct(a: &str) -> bool {
    IpAddr::is_ipv4(&a.parse().unwrap())
}

fn method(a: &str) -> bool {
    a.parse().unwrap().is_ipv4()
    
    // error[E0282]: type annotations needed
    //  --> src/lib.rs:8:7
    //   |
    // 8 |     a.parse().unwrap().is_ipv4()
    //   |       ^^^^^ cannot infer type of the type parameter `F` declared on the associated function `parse`
    //   |
    // help: consider specifying the generic argument
    //   |
    // 8 |     a.parse::<F>().unwrap().is_ipv4()
    //   |            +++++
}

Personally I would consider this infeasibly difficult to solve in the general case

@blyxyas
Copy link
Member Author

blyxyas commented Nov 27, 2022

Thinking about this, you're right, adding a lint this complex not only wastes a lot of time, but gives a warning that a lot of times isn't necessary. This lint is made to ignore a lot of cases, a lot of the cases it doesn't ignore are cases which saying explicitly the trait is intended. What is the utility of it?

I think I'm going to close the pull request. At least it was a draft and I learned a little bit more. Sorry for wasting your time

@blyxyas blyxyas closed this Nov 27, 2022
@Alexendoo
Copy link
Member

No worries, not a waste of time at all. Thanks for giving it a shot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prefer X.floor() instead of FP::floor(x)
8 participants