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

Add format_in_format_args and to_string_in_format_args lints #7743

Merged
merged 3 commits into from
Oct 15, 2021
Merged

Add format_in_format_args and to_string_in_format_args lints #7743

merged 3 commits into from
Oct 15, 2021

Conversation

smoelius
Copy link
Contributor

Fixes #7667 and #7729

I put these in perf since that was one of @jplatte's suggestions, and redundant_clone (which I consider to be similar) lives there as well.

However, I am open to changing the category or anything else.

r? @camsteffen

changelog: Add format_in_format_args and to_string_in_format_args lints

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 30, 2021
@smoelius
Copy link
Contributor Author

Sorry for the late addition. I added two more test cases.

camsteffen
camsteffen reviewed Sep 30, 2021
View reviewed changes
clippy_utils/src/format.rs Outdated Show resolved Hide resolved
clippy_lints/src/format_args.rs Outdated Show resolved Hide resolved
@smoelius
Copy link
Contributor Author

smoelius commented Oct 2, 2021

I pushed the changes using bitflags.

bitflags 1.2 is already in rust's lockfile, so hopefully this won't be a problem: https://github.com/rust-lang/rust/blob/a8387aef8c378a771686878062e544af4d5e2245/Cargo.lock#L125

Please let me know what other changes are needed.

@bors
Copy link
Collaborator

bors commented Oct 3, 2021

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

camsteffen
camsteffen suggested changes Oct 4, 2021
View reviewed changes
clippy_lints/src/format_args.rs Outdated Show resolved Hide resolved
clippy_lints/src/format_args.rs Outdated Show resolved Hide resolved
clippy_lints/src/format_args.rs Outdated Show resolved Hide resolved
clippy_lints/src/format_args.rs Outdated Show resolved Hide resolved
clippy_utils/src/higher.rs Outdated Show resolved Hide resolved
clippy_utils/src/higher.rs Outdated Show resolved Hide resolved
clippy_lints/src/format_args.rs Show resolved Hide resolved
clippy_utils/src/higher.rs Outdated Show resolved Hide resolved
clippy_utils/src/higher.rs Outdated Show resolved Hide resolved
tests/ui/format_args.rs Show resolved Hide resolved
@camsteffen camsteffen 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 Oct 5, 2021
@bors
Copy link
Collaborator

bors commented Oct 6, 2021

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

@smoelius
Copy link
Contributor Author

smoelius commented Oct 7, 2021

@camsteffen I apologize for the delay. The Deref case was trickier than I expected. I think the most recent push addresses all of your comments up to this point.

@bors
Copy link
Collaborator

bors commented Oct 8, 2021

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

camsteffen
camsteffen suggested changes Oct 8, 2021
View reviewed changes
Copy link
Contributor

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

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

No problem at all!

Okay, I have another round of feedback. We're definitely getting closer. This may be the most complex macro processing that Clippy has even seen so really great work!

clippy_lints/src/format.rs Outdated Show resolved Hide resolved
clippy_utils/src/higher.rs Outdated Show resolved Hide resolved
clippy_utils/src/higher.rs Outdated Show resolved Hide resolved
clippy_lints/src/format_args.rs Outdated Show resolved Hide resolved
clippy_lints/src/format_args.rs Outdated Show resolved Hide resolved
clippy_lints/src/format_args.rs Outdated Show resolved Hide resolved
clippy_lints/src/format_args.rs Outdated Show resolved Hide resolved
tests/ui/format_args_unfixable.stderr Outdated Show resolved Hide resolved
tests/ui/format_args_unfixable.stderr Outdated Show resolved Hide resolved
tests/ui/format_args_unfixable.rs Outdated Show resolved Hide resolved
@smoelius
Copy link
Contributor Author

I think I'm ready for the next round of feedback, @camsteffen. Thanks for doing this.

camsteffen
camsteffen suggested changes Oct 12, 2021
View reviewed changes
Copy link
Contributor

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

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

I think we're nearing the end now. Thanks for sticking with it.

clippy_lints/src/format_args.rs Outdated Show resolved Hide resolved
clippy_lints/src/format_args.rs Outdated Show resolved Hide resolved
tests/ui/format_args.rs Show resolved Hide resolved
clippy_lints/src/format_args.rs Outdated Show resolved Hide resolved
clippy_lints/src/format_args.rs Show resolved Hide resolved
clippy_utils/src/higher.rs Show resolved Hide resolved
clippy_lints/src/format_args.rs Show resolved Hide resolved
clippy_lints/src/format_args.rs Outdated Show resolved Hide resolved
@bors
Copy link
Collaborator

bors commented Oct 12, 2021

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

@smoelius
Copy link
Contributor Author

@camsteffen The paths I added to path.rs appear to be the only ones that refer to macros. I am guessing this is why INVALID_PATHS is failing. Assuming this is right, do you have any suggestions on how to fix it?

@camsteffen
Copy link
Contributor

@camsteffen The paths I added to path.rs appear to be the only ones that refer to macros. I am guessing this is why INVALID_PATHS is failing. Assuming this is right, do you have any suggestions on how to fix it?

@smoelius I'd probably have to debug the invalid_paths lint to figure it out. But the paths should be migrated to diagnostic items anyways so it's kinda not worth the effort. So it's okay to use paths for now and allow the lint on those items with a todo. Or, if you feel inclined, you can add the diagnostic items in rustc and wait for them to be synced in the following Rustup PR.

@smoelius
Copy link
Contributor Author

it's okay to use paths for now and allow the lint on those items with a todo.

OK, that's what I did.

I also did some optimistic squashing. But I can undo it if it's a problem.

camsteffen
camsteffen approved these changes Oct 15, 2021
View reviewed changes
Copy link
Contributor

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

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

Please squash the last commit and this is ready to merge!

@smoelius
Copy link
Contributor Author

Thanks very much for your patience, @camsteffen.

@camsteffen
Copy link
Contributor

You bet. Thanks for the PR!

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 15, 2021

📌 Commit 75e9f8c has been approved by camsteffen

@bors
Copy link
Collaborator

bors commented Oct 15, 2021

⌛ Testing commit 75e9f8c with merge e1871ba...

@bors
Copy link
Collaborator

bors commented Oct 15, 2021

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

@bors bors merged commit e1871ba into rust-lang:master Oct 15, 2021
This was referenced Oct 15, 2021
Closed
Merged
This was referenced Nov 12, 2021
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camsteffen camsteffen camsteffen approved these changes

Assignees

@camsteffen camsteffen

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.

Lint suggestion: nested format!
4 participants
@smoelius @bors @camsteffen @rust-highfive