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 avoid_breaking_exported_api config option #7187

Merged
merged 10 commits into from
May 27, 2021
Merged

Add avoid_breaking_exported_api config option #7187

merged 10 commits into from
May 27, 2021

Conversation

camsteffen
Copy link
Contributor

@camsteffen camsteffen commented May 7, 2021 •
edited
Loading

changelog: Add avoid_breaking_exported_api config option for [enum_variant_names], [large_types_passed_by_value], [trivially_copy_pass_by_ref], [unnecessary_wraps], [upper_case_acronyms] and [wrong_self_convention].

changelog: Deprecates [pub_enum_variant_names] and [wrong_pub_self_convention] as the non-pub variants are now configurable.

changelog: Fix various false negatives for pub items that are not exported from the crate.

A couple changes to late passes in order to use cx.access_levels.is_exported rather than item.vis.kind.is_pub.

I'm not sure how to better document the config option or lints that are (not) affected (see comments in #6806). Suggestions are welcome. cc @rust-lang/clippy

I added /clippy.toml to use the config internally and /tests/clippy.toml to maintain a default config in ui tests.

Closes #6806
Closes #4504

@rust-highfive
Copy link

r? @phansch

(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 May 7, 2021
@camsteffen camsteffen force-pushed the avoid-break-exported branch 2 times, most recently from 9a8dacd to 7895c50 Compare May 7, 2021 16:11
flip1995
flip1995 reviewed May 7, 2021
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.

Haven't reviewed it in-depth (just reviewed the "infra"-impact), but this looks like a great improvement of Clippy. Thanks!

clippy.toml Show resolved Hide resolved
tests/clippy.toml Outdated Show resolved Hide resolved
clippy_lints/src/deprecated_lints.rs Show resolved Hide resolved
@camsteffen
Copy link
Contributor Author

I added some cleanup to the compiletest to call remove_var after using set_var if the variable was previously unset. This uncovered a problem with the config file lookup logic which I just fixed.

@camsteffen camsteffen force-pushed the avoid-break-exported branch 2 times, most recently from e705584 to 01a6fdb Compare May 7, 2021 17:22
@bors
Copy link
Collaborator

bors commented May 13, 2021

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

@camsteffen camsteffen force-pushed the avoid-break-exported branch 2 times, most recently from 91ddf0e to 7e0714b Compare May 15, 2021 16:06
phansch
phansch approved these changes May 18, 2021
View reviewed changes
Copy link
Member

@phansch phansch left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! The implementation looks good to me. My only concern is that we don't have tests for when the option is set to false. I think this might be a general problem though, and I wouldn't object to merging the PR anyway. What do you think?

clippy_lints/src/lib.rs Outdated Show resolved Hide resolved
@phansch phansch 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 May 18, 2021
@bors
Copy link
Collaborator

bors commented May 20, 2021

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

@camsteffen
Copy link
Contributor Author

Rebased with your comment.

You're right that false is not tested. Though it is proven out with the dogfood test. I'm okay with it if you are :)

@phansch
Copy link
Member

phansch commented May 27, 2021

Thanks!
@bors r=flip1995,phansch

@bors
Copy link
Collaborator

bors commented May 27, 2021

📌 Commit 6eea598 has been approved by flip1995,phansch

@bors
Copy link
Collaborator

bors commented May 27, 2021

⌛ Testing commit 6eea598 with merge 9c4651f...

@bors
Copy link
Collaborator

bors commented May 27, 2021

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

@bors bors merged commit 9c4651f into rust-lang:master May 27, 2021
@camsteffen camsteffen deleted the avoid-break-exported branch July 8, 2021 21:47
@xFrednet xFrednet mentioned this pull request Jul 26, 2021
Closed
@xFrednet xFrednet mentioned this pull request Aug 12, 2021
Closed
@tamaroning tamaroning mentioned this pull request Jul 1, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 left review comments

@phansch phansch phansch approved these changes

Assignees

@phansch phansch

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.

Config for public API changes Option to disable trivially_copy_pass_by_ref in public functions
5 participants
@camsteffen @rust-highfive @bors @phansch @flip1995