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

Inconsistent struct constructor #6769

Merged
merged 3 commits into from
Feb 22, 2021

Conversation

Y-Nak
Copy link
Contributor

@Y-Nak Y-Nak commented Feb 20, 2021

fixes: #6352
r? @matthiaskrgr

I added the lint that checks for the struct constructors where the order of the field init shorthands is inconsistent with that in the struct definition.

changelog: Add style lint: inconsistent_struct_constructor

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 20, 2021
@Y-Nak Y-Nak force-pushed the inconsistent-struct-constructor branch from c706d74 to 4adea7d Compare February 20, 2021 15:16
@Y-Nak Y-Nak force-pushed the inconsistent-struct-constructor branch from 4adea7d to d646aa2 Compare February 20, 2021 20:08
Copy link
Member

@matthiaskrgr matthiaskrgr left a comment

Choose a reason for hiding this comment

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

In general this looks good to me.
I ran lintcheck with your changes (cargo dev lintcheck ), the lint only triggered 3 times in our set of crates but all of them looked like true positives to me 👍

Comment on lines 13 to 41
/// **What it does:** Checks for struct constructors where the order of the field init
/// shorthand in the constructor is inconsistent with the order in the struct definition.
///
/// **Why is this bad?** It decreases readability and consistency.
///
/// **Known problems:** None.
///
/// **Example:**
///
/// ```rust
/// struct Foo {
/// x: i32,
/// y: i32,
/// }
/// let x = 1;
/// let y = 2;
/// Foo { y, x };
/// ```
///
/// Use instead:
/// ```rust
/// # struct Foo {
/// # x: i32,
/// # y: i32,
/// # }
/// # let x = 1;
/// # let y = 2;
/// Foo { x, y };
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should add some kind of short demonstration that Foo {x, y} and Foo {y,x} are the same?

#[derive(Eq, PartialEq, Debug)]
struct Foo {x: i32, y: i32}

fn main() {
    let x = 0;
    let y = 1;
    assert_eq!(Foo {x, y}, Foo {y, x});
}

Comment on lines 76 to 78
}
fields_snippet.push_str(&format!("{}", last_ident));

Copy link
Member

Choose a reason for hiding this comment

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

Hmm shouldn't this trigger clippy::useless_format...? 🤔
This can be just
fields_snippet.push_str(&last_ident.to_string());

let base_snippet = if let Some(base) = base {
format!(", ..{}", snippet(cx, base.span, ".."))
} else {
"".to_string()
Copy link
Member

Choose a reason for hiding this comment

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

nit: String::new()

@matthiaskrgr matthiaskrgr 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 Feb 21, 2021
@Y-Nak
Copy link
Contributor Author

Y-Nak commented Feb 22, 2021

Changes:

  1. Add a description for fields order.
  2. Reflect suggested changes related to String.

@matthiaskrgr
Copy link
Member

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Feb 22, 2021

📌 Commit bfdf0fa has been approved by matthiaskrgr

@bors
Copy link
Contributor

bors commented Feb 22, 2021

⌛ Testing commit bfdf0fa with merge fe01ddc...

@bors
Copy link
Contributor

bors commented Feb 22, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: matthiaskrgr
Pushing fe01ddc to master...

@bors bors merged commit fe01ddc into rust-lang:master Feb 22, 2021
@Y-Nak Y-Nak deleted the inconsistent-struct-constructor branch February 22, 2021 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

false negative if_same_then_else / new lint catch wrongly ordered struct init shorthand
4 participants