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.

Sign up for GitHub

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

Jump to bottom

Don't use to_string in impl Display #5831

Merged
merged 1 commit into from
Aug 16, 2020
Merged

Don't use to_string in impl Display #5831

merged 1 commit into from
Aug 16, 2020

Conversation

chansuke
Copy link
Contributor

@chansuke chansuke commented Jul 22, 2020 •
edited by flip1995
Loading

fixes #3876

this PR is derived from Toxyxer's implementation.
changelog: add [to_string_in_display] lint

@rust-highfive
Copy link

r? @flip1995

(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 Jul 22, 2020
flip1995
flip1995 requested changes Jul 26, 2020
View reviewed changes
clippy_lints/src/to_string_in_display.rs Outdated Show resolved Hide resolved
tests/ui/to_string_in_display.stderr Outdated Show resolved Hide resolved
tests/ui/to_string_in_display.stderr Outdated Show resolved Hide resolved
@flip1995 flip1995 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 Jul 26, 2020
@bors
Copy link
Contributor

bors commented Aug 4, 2020

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

flip1995
flip1995 reviewed Aug 8, 2020
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

LGTM overall. We can use the lint infrastructure better, to make the code more readable though.

tests/ui/to_string_in_display.rs Outdated Show resolved Hide resolved
clippy_lints/src/to_string_in_display.rs Outdated Show resolved Hide resolved
clippy_lints/src/to_string_in_display.rs Outdated Show resolved Hide resolved
@chansuke chansuke force-pushed the to_string_in_display branch 5 times, most recently from 0932baa to 84315f7 Compare August 13, 2020 15:57
flip1995
flip1995 reviewed Aug 13, 2020
View reviewed changes
clippy_lints/src/to_string_in_display.rs Outdated
Comment on lines 97 to 98
if match_def_path(cx, did, &paths::DISPLAY_TRAIT);
then {
true
} else {
false
}
Copy link
Member

Choose a reason for hiding this comment

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

Regarding the clippy::needless_bool lint triggering here: You can simplify this to:

Suggested change
if match_def_path(cx, did, &paths::DISPLAY_TRAIT);
then {
true
} else {
false
}
then {
match_def_path(cx, did, &paths::DISPLAY_TRAIT)
} else {
false
}

Clippy is great 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!!

@flip1995
Copy link
Member

Please also address the other two dogfood errors:

warning: unused variable: `parent_node`
  --> clippy_lints/src/to_string_in_display.rs:73:13
   |
73 |         let parent_node = cx.tcx.hir().find(parent_id);
   |             ^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_parent_node`
   |
   = note: `#[warn(unused_variables)]` on by default

error: usage of wildcard import
 --> clippy_lints/src/to_string_in_display.rs:3:5
  |
3 | use rustc_hir::*;
  |     ^^^^^^^^^^^^ help: try: `rustc_hir::{Expr, ExprKind, Item, ItemKind}`
  |
  = note: `-D clippy::wildcard-imports` implied by `-D clippy::pedantic`
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wildcard_imports

@flip1995
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Aug 16, 2020

📌 Commit 8e54997 has been approved by flip1995

bors added a commit that referenced this pull request Aug 16, 2020
@bors
Auto merge of #5831 - chansuke:to_string_in_display, r=flip1995
a2d8331 
Don't use `to_string` in impl Display

fixes #3876

this PR is derived from [Toxyxer's implementation](#5574).
changelog: none
@bors
Copy link
Contributor

bors commented Aug 16, 2020

⌛ Testing commit 8e54997 with merge a2d8331...

@flip1995
Copy link
Member

@bors retry (for changelog)

@bors
Copy link
Contributor

bors commented Aug 16, 2020

⌛ Testing commit 8e54997 with merge e522ca3...

@bors
Copy link
Contributor

bors commented Aug 16, 2020

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

@bors bors merged commit e522ca3 into rust-lang:master Aug 16, 2020
tesuji
tesuji reviewed Aug 17, 2020
View reviewed changes
clippy_lints/src/to_string_in_display.rs
/// ```
pub TO_STRING_IN_DISPLAY,
correctness,
"to_string method used while implementing Display trait"
Copy link
Contributor

Choose a reason for hiding this comment

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

"`to_string` method used while implementing `Display` trait"

clippy_lints/src/to_string_in_display.rs
cx,
TO_STRING_IN_DISPLAY,
expr.span,
"Using to_string in fmt::Display implementation might lead to infinite recursion",
Copy link
Contributor

Choose a reason for hiding this comment

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

"using to_string in fmt::Display implementation might lead to infinite recursion",

@giraffate giraffate mentioned this pull request Aug 19, 2020
Merged
bors added a commit that referenced this pull request Aug 22, 2020
@bors
Auto merge of #5926 - giraffate:improve_lint_message_in_to_string_in_…
b57ef14 
…display, r=yaahc

Improve lint message in `to_string_in_display`

This is a follow-up of #5831.

changelog: none
@HKalbasi HKalbasi mentioned this pull request Jul 29, 2021
Closed
@HKalbasi HKalbasi mentioned this pull request Jul 29, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tesuji tesuji tesuji left review comments

@flip1995 flip1995 flip1995 requested changes

Assignees

@flip1995 flip1995

Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Correctness: no to_string in impl Display
5 participants
@chansuke @rust-highfive @bors @flip1995 @tesuji