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

[WIP] Add getter prefix name lint #3616

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@0xazure
Copy link

0xazure commented Jan 3, 2019

Add a lint to detect getter methods prefixed with get_ which do not
follow the Rust convention for getter names as described in the
naming section of the Rust API Guidelines.

The get_ prefix is not used unless there is a single and obvious thing
that could be gotten by a getter, including:

  • get
  • get_mut
  • get_unchecked
  • get_unchecked_mut
  • get_ref

Closes #1673.

TODO:

The basic lint is implemented in this pull request, but there are still a couple things I'd like to add/finalize before I feel this is ready to merge. I wanted to open a pull request as soon as possible to collect and start incorporating feedback though, so this is currently a WIP.

  • finalize lint name and lint span message
  • implement machine-applicable renaming suggestions that would remove the get_ prefix from the method name (using span_lint_and_sugg?)

Questions:

Since this is my first contribution to Clippy, I would really appreciate your feedback! I found the rustc documentation to be a bit lacking, so if there are better ways to approach this lint I'm all ears.

I also have a few specific items I would appreciate some guidance on:

  1. LintPass::get_lints and thus tests/ui/lint_without_lint_pass.rs (as well as a few other rustc methods like rustc_codegen_llvm::callee::get_fn and syntax::ext::tt::quoted::TokenTree::get_tt, for example) will not pass this lint rule in its current form. I'm not sure of the next step with these methods; exceptions can be added to the allowed name list, or is this something that will need to be changed in rustc first before this lint can land?
  2. I wasn't able to run target/debug/cargo-clippy clippy --all-targets --all-features -- -D clippy::all -D clippy::internal -D clippy::pedantic against my local changes, I got the following error:
    clippy/target/debug/cargo-clippy clippy --all-targets --all-features -- -D clippy::all -D clippy::internal -D clippy::pedantic
    error: failed to run `rustc` to learn about target-specific information
    
    Caused by:
    process didn't exit successfully: `clippy/target/debug/clippy-driver rustc - --crate-name ___ --print=file-names --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro` (signal: 6, SIGABRT: process abort signal)
    --- stderr
    dyld: Library not loaded: @rpath/librustc_driver-b630426988dbbdb0.dylib
    Referenced from: clippy/target/debug/clippy-driver
    Reason: image not found
    
    Looks like a linker error, I'm not sure what's going on here.
  3. I'm not totally sure about the lint organization in Clippy. I created a separate file for this new lint, but should it actually live somewhere like clippy_lints/src/functions.rs or another existing file?
  4. In #1673 @flip1995 brought up an interesting point about linting just prefixes (e.g. just get_unchecked* instead of get_unchecked and get_unchecked_mut separately). Are there any opinions on how rigid we want the exceptions? As I commented on the issue, more permissive prefix matching could lead to some methods being missed simply because they start with one of the allowed names.
  5. This lint only looks at impls, but what about functions not associated with a type? As far as I can tell they aren't covered by the API guidelines but could be added to this lint as well.
  6. The current implementation is naive name comparison, should there be some further inspection to test that a function actually accesses self to be considered a 'getter'?

0xazure added some commits Dec 7, 2018

Add getter prefix name lint
Add a lint to detect getter methods prefixed with `get_` which do not
follow the Rust convention for getter names as described in the
[naming section][0] of the Rust API Guidelines.

The `get_` prefix is not used unless there is a single and obvious thing
that could be gotten by a getter, including:
- `get`
- `get_mut`
- `get_unchecked`
- `get_unchecked_mut`
- `get_ref`

Closes #1673.

[0]: https://rust-lang-nursery.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 3, 2019

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

@flip1995

This comment has been minimized.

Copy link
Collaborator

flip1995 commented Jan 3, 2019

The current WIP looks good, so I'll just answer your questions.

  1. This will come down to just get_tt (and maybe LintPass::get_lints) after answering the other questions.
  2. Sometimes you have to set the LD_LIBRARY_PATH (resp. PATH in windows) for this to work. See
    export LD_LIBRARY_PATH=$(rustc --print sysroot)/lib
  3. There is the clippy_lints/src/methods module, where this lint would fit in well IMO.
  4. Maybe someone else has an opinion on this :)
  5. +6. IMO it would be good to only apply this lint on methods and exclude non-associated functions (maybe also exclude associated functions? I don't know). This lint should only lint impls (not Trait impls) and Trait definitions. There is already a lint with the same restrictions in the methods module (I think), so the code to filter for this is already there. I can't remember which lint exactly that was, but if you can't find it, I'm happy to figure it out and point you there.

With 5+6 answered, this lint won't trigger on tests/ui/lint_without_lint_pass.rs since that is a Trait impl. Also it would not trigger on rustc_codegen_llvm::callee::get_fn since this is a non-associated function. For syntax::ext::tt::quoted::TokenTree::get_tt (->token_tree(&self, index: usize)?) and LintPass::get_lints maybe a refactoring in rustc could be done.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 7, 2019

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

@flip1995

This comment has been minimized.

Copy link
Collaborator

flip1995 commented Jan 14, 2019

ping from triage @0xazure. Any updates on this?

@0xazure

This comment has been minimized.

Copy link

0xazure commented Jan 15, 2019

Hey @flip1995 thanks for the ping, I lost track of this with the start of the new year. I should have more time to revisit this at the end of the week.

  1. Thanks for pointing that out, I'll give it a try and maybe put together a quick PR against the contributing docs if it turns out to help
  2. Thoughts on clippy_lints/src/methods vs clippy_lints/src/functions.rs then? My quick skim of both leaves me with the sense that methods seems to be about calling conventions and method usage, whereas functions.rs seems to be more about function signatures (e.g. too many arguments), but as I mentioned I'm not that familiar with the lint organization.
  3. Would definitely appreciate some more perspectives, any suggestions on who to ping?

As for 1/5/6 that's interesting! I was just running through on the basis of the function names (get_) rather than looking at their source.

maybe also exclude associated functions

Maybe I've misunderstood, but aren't associated functions the ones in the impl? This might just be a confusion of terminology for me though.

This lint should only lint impls (not Trait impls) and Trait definitions.

I probably haven't implemented it to look at Trait impls, but since I'm not familiar with Trait impls could you expand on why they should be excluded? From a consistency standpoint I would expect it to disallow the get_ prefix on all methods, regardless of where they're defined.

I'll need to verify if it lints Trait definitions, and add some more test cases to document that behaviour along with updating the doc comment.

@flip1995

This comment has been minimized.

Copy link
Collaborator

flip1995 commented Jan 16, 2019

  1. Oh yeah you're right. In that case functions.rs is a good place for this lint!
  1. Would definitely appreciate some more perspectives, any suggestions on who to ping?

@oli-obk any opinions on that, as you created the original issue?

aren't associated functions the ones in the impl?

Yes!
non-associated functions: Outside of any impl
associated functions: Inside an impl
methods: Inside an impl with a self argument

Trait impls could you expand on why they should be excluded

Sure! With Trait impls I mean thinks like impl MyTrait for Foo { ... }. These shouldn't be linted because Traits can come from extern crates. In that case the programmer doesn't has any influence on the function names of the Trait.

The author of the crate which includes the Trait has influence on the names of the function names. So the Trait definition (trait MyTrait { ... }) should be linted. Now if you change the name of a function in the trait definition, rustc will take over and tell you on every impl of that Trait that the old function name wasn't found. => Everything is linted.

TL;DR: Lint on impl Foo methods/functions and trait MyTrait methods/functions, but not on impl MyTrait for Foo methods/functions.

/// }
/// ```
declare_clippy_lint! {
pub GETTER_PREFIX,

This comment has been minimized.

@oli-obk

oli-obk Jan 18, 2019

Collaborator

The lint naming guidelines state that we should be able to read #[allow(foo)] sort of as a sentence, so "allow getter prefix" should probably be pluralized to "allow getter prefixes".

span_lint(
cx,
GETTER_PREFIX,
implitem.span,

This comment has been minimized.

@oli-obk

oli-obk Jan 18, 2019

Collaborator

I think implitem.ident.span should give you the span of just the name (which both leads to a better diagnostic message and should be helpful for a structured suggestion)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment