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

[WIP] Add getter prefix name lint #3616

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,7 @@ All notable changes to this project will be documented in this file.
[`forget_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_copy
[`forget_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_ref
[`get_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_unwrap
[`getter_prefix`]: https://rust-lang.github.io/rust-clippy/master/index.html#getter_prefix
[`identity_conversion`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_conversion
[`identity_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_op
[`if_let_redundant_pattern_matching`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_let_redundant_pattern_matching
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.

[There are 290 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
[There are 291 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)

We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:

Expand Down
4 changes: 4 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ pub mod multiple_crate_versions;
pub mod mut_mut;
pub mod mut_reference;
pub mod mutex_atomic;
pub mod naming;
pub mod needless_bool;
pub mod needless_borrow;
pub mod needless_borrowed_ref;
Expand Down Expand Up @@ -486,6 +487,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
reg.register_late_lint_pass(box ptr_offset_with_cast::Pass);
reg.register_late_lint_pass(box redundant_clone::RedundantClone);
reg.register_late_lint_pass(box slow_vector_initialization::Pass);
reg.register_early_lint_pass(box naming::GetterPrefix);

reg.register_lint_group("clippy::restriction", Some("clippy_restriction"), vec![
arithmetic::FLOAT_ARITHMETIC,
Expand Down Expand Up @@ -697,6 +699,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
misc_early::ZERO_PREFIXED_LITERAL,
mut_reference::UNNECESSARY_MUT_PASSED,
mutex_atomic::MUTEX_ATOMIC,
naming::GETTER_PREFIX,
needless_bool::BOOL_COMPARISON,
needless_bool::NEEDLESS_BOOL,
needless_borrowed_ref::NEEDLESS_BORROWED_REFERENCE,
Expand Down Expand Up @@ -837,6 +840,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
misc_early::MIXED_CASE_HEX_LITERALS,
misc_early::UNNEEDED_FIELD_PATTERN,
mut_reference::UNNECESSARY_MUT_PASSED,
naming::GETTER_PREFIX,
neg_multiply::NEG_MULTIPLY,
new_without_default::NEW_WITHOUT_DEFAULT,
non_expressive_names::JUST_UNDERSCORES_AND_DIGITS,
Expand Down
95 changes: 95 additions & 0 deletions clippy_lints/src/naming.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use crate::rustc::lint::{EarlyContext, EarlyLintPass, LintArray, LintPass};
use crate::rustc::{declare_tool_lint, lint_array};
use crate::syntax::ast;
use crate::utils::span_lint;

/// **What it does:** Checks for the `get_` prefix on getters.
///
/// **Why is this bad?** The Rust API Guidelines section on naming
/// [specifies](https://rust-lang-nursery.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter)
/// that the `get_` prefix is not used for getters in Rust code unless
/// there is a single and obvious thing that could reasonably be gotten by
/// a getter.
///
/// The exceptions to this naming convention are as follows:
/// - `get` (such as in
/// [`std::cell::Cell::get`](https://doc.rust-lang.org/std/cell/struct.Cell.html#method.get))
/// - `get_mut`
/// - `get_unchecked`
/// - `get_unchecked_mut`
/// - `get_ref`
///
/// **Known problems:** None.
///
/// **Example:**
///
/// ```rust
/// // Bad
/// impl B {
/// fn get_id(&self) -> usize {
/// ..
/// }
/// }
///
/// // Good
/// impl G {
/// fn id(&self) -> usize {
/// ..
/// }
/// }
///
/// // Also allowed
/// impl A {
/// fn get(&self) -> usize {
/// ..
/// }
/// }
/// ```
declare_clippy_lint! {
pub GETTER_PREFIX,
Copy link
Contributor

Choose a reason for hiding this comment

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

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".

style,
"prefixing a getter with `get_`, which does not follow convention"
}

#[derive(Copy, Clone)]
pub struct GetterPrefix;

#[rustfmt::skip]
const ALLOWED_METHOD_NAMES: [&'static str; 5] = [
"get",
"get_mut",
"get_unchecked",
"get_unchecked_mut",
"get_ref"
];

impl LintPass for GetterPrefix {
fn get_lints(&self) -> LintArray {
lint_array!(GETTER_PREFIX)
}
}

impl EarlyLintPass for GetterPrefix {
fn check_impl_item(&mut self, cx: &EarlyContext<'_>, implitem: &ast::ImplItem) {
if let ast::ImplItemKind::Method(..) = implitem.node {
let name = implitem.ident.name.as_str().get();
if name.starts_with("get_") && !ALLOWED_METHOD_NAMES.contains(&name) {
span_lint(
cx,
GETTER_PREFIX,
implitem.span,
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

"prefixing a getter with `get_` does not follow naming conventions",
);
}
}
}
}
48 changes: 48 additions & 0 deletions tests/ui/naming.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

pub struct MyStruct {
id: usize
}

impl MyStruct {
pub fn get_id(&self) -> usize {
self.id
}

pub fn get(&self) -> usize {
self.id
}

pub fn get_mut(&mut self) -> usize {
self.id
}

pub fn get_unchecked(&self) -> usize {
self.id
}

pub fn get_unchecked_mut(&mut self) -> usize {
self.id
}

pub fn get_ref(&self) -> usize {
self.id
}
}

fn main() {
let mut s = MyStruct { id: 42 };
s.get_id();
s.get();
s.get_mut();
s.get_unchecked();
s.get_unchecked_mut();
s.get_ref();
}
12 changes: 12 additions & 0 deletions tests/ui/naming.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error: prefixing a getter with `get_` does not follow naming conventions
--> $DIR/naming.rs:15:5
|
LL | / pub fn get_id(&self) -> usize {
LL | | self.id
LL | | }
| |_____^
|
= note: `-D clippy::getter-prefix` implied by `-D warnings`

error: aborting due to previous error

4 changes: 2 additions & 2 deletions tests/ui/unused_unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
struct Unitter;
impl Unitter {
// try to disorient the lint with multiple unit returns and newlines
pub fn get_unit<F: Fn() -> (), G>(&self, f: F, _g: G) ->
pub fn get<F: Fn() -> (), G>(&self, f: F, _g: G) ->
()
where G: Fn() -> () {
let _y: &Fn() -> () = &f;
Expand All @@ -41,7 +41,7 @@ fn return_unit() -> () { () }

fn main() {
let u = Unitter;
assert_eq!(u.get_unit(|| {}, return_unit), u.into());
assert_eq!(u.get(|| {}, return_unit), u.into());
return_unit();
loop {
break();
Expand Down
6 changes: 3 additions & 3 deletions tests/ui/unused_unit.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error: unneeded unit return type
--> $DIR/unused_unit.rs:25:59
--> $DIR/unused_unit.rs:25:54
|
LL | pub fn get_unit<F: Fn() -> (), G>(&self, f: F, _g: G) ->
| ___________________________________________________________^
LL | pub fn get<F: Fn() -> (), G>(&self, f: F, _g: G) ->
| ______________________________________________________^
LL | | ()
| |__________^ help: remove the `-> ()`
|
Expand Down