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

Make private_in_public compatibility lint deny-by-default #34206

Merged
merged 2 commits into from
Aug 14, 2016
Merged

Make private_in_public compatibility lint deny-by-default #34206

merged 2 commits into from
Aug 14, 2016

Conversation

petrochenkov
Copy link
Contributor

In accordance with the plan.

r? @nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 21, 2016

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

@nikomatsakis
Copy link
Contributor

Sorry for the delay in reviewing! I've been traveling but I ought to be around tomorrow. Two quick questions:

  1. What issue does this link to again? I think once we move to error, it'd be nice to link to a good issue.
  2. Should we keep this in "lint but error" mode, or should we just modify the code to emit a hard error? I had generally envisioned the former as a kind of "optional step" but maybe it's a good middle-ground, and it makes it easy to link to an existing issue with more details for the people who will all of a sudden start noticing what's going on.

@petrochenkov
Copy link
Contributor Author

What issue does this link to again? I think once we move to error, it'd be nice to link to a good issue.

This lint is old, it doesn't have a dedicated tracking issue. It "links" to --explain E0446 at the moment.
I'll probably create one, the lint is still going to exist for at least few releases.

Should we keep this in "lint but error" mode, or should we just modify the code to emit a hard error?

I want it to be a deny-by-default lint for at least one cycle, this is a change with large impact after all.
This is supposed to be merged before the upcoming beta, so I will (partially) turn it into error after that.
deny lints can uncover some unexpected cases, for example, nobody reads warnings in tests, so this change has caught a couple of private-in-public errors in our test suite.

private_in_public is a facade for several kinds of errors, the largest and the oldest ones are in rustc_privacy, some more recent ones are in resolve (e.g, extern crate foo; pub use foo;), they will stay lints for some more time, but will benefit from being deny by default too.

@nikomatsakis
Copy link
Contributor

@petrochenkov

I'll probably create one, the lint is still going to exist for at least few releases.

Sounds good. The main reason I didn't r+ right away is that I wanted to see a link to an issue that offers a good explanation of what's going on. Do you want to create such an issue first, or merge this PR first?

@petrochenkov petrochenkov mentioned this pull request Jun 28, 2016
Closed
5 tasks
@petrochenkov
Copy link
Contributor Author

@nikomatsakis
Updated with a reference to #34537

@nikomatsakis
Copy link
Contributor

@petrochenkov ok -- but it seems like you suggested we wait until #34193 is resolved, no?

@petrochenkov
Copy link
Contributor Author

@nikomatsakis
Sorry for the delay, I've updated #34193, now everything should be in place.

@petrochenkov petrochenkov mentioned this pull request Jul 14, 2016
Closed
@nikomatsakis
Copy link
Contributor

r=me, but still waiting until #34193 merges

@bors
Copy link
Contributor

bors commented Aug 11, 2016

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

@petrochenkov
Copy link
Contributor Author

Rebased.
ping @nikomatsakis, #34193 has landed

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 12, 2016

📌 Commit b052dd6 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 12, 2016

⌛ Testing commit b052dd6 with merge d296294...

bors added a commit that referenced this pull request Aug 12, 2016
@bors
Auto merge of #34206 - petrochenkov:pipdeny, r=nikomatsakis
d296294 
Make `private_in_public` compatibility lint deny-by-default

In accordance with the [plan](https://internals.rust-lang.org/t/fcp-for-various-future-compatibility-warnings/3590/5?u=petrochenkov).

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Aug 13, 2016

💔 Test failed - auto-linux-64-debug-opt

@petrochenkov
Copy link
Contributor Author

Spurious failure.

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Aug 13, 2016

⌛ Testing commit b052dd6 with merge 9a68d58...

@bors
Copy link
Contributor

bors commented Aug 14, 2016

💔 Test failed - auto-linux-64-debug-opt

@alexcrichton
Copy link
Member

@bors: retry

On Sat, Aug 13, 2016 at 6:03 PM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-debug-opt
https://buildbot.rust-lang.org/builders/auto-linux-64-debug-opt/builds/3466

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#34206 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95PSaoQKSEGoazCXFHqu1HXGLxQVQks5qfmlPgaJpZM4IzTU7
.

@bors
Copy link
Contributor

bors commented Aug 14, 2016

⌛ Testing commit b052dd6 with merge d927fa4...

bors added a commit that referenced this pull request Aug 14, 2016
@bors
Auto merge of #34206 - petrochenkov:pipdeny, r=nikomatsakis
d927fa4 
Make `private_in_public` compatibility lint deny-by-default

In accordance with the [plan](https://internals.rust-lang.org/t/fcp-for-various-future-compatibility-warnings/3590/5?u=petrochenkov).

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Aug 14, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-no-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-no-mir, auto-win-msvc-64-opt-rustbuild
Approved by: nikomatsakis
Pushing d927fa4 to master...

@bors bors merged commit b052dd6 into rust-lang:master Aug 14, 2016
@petrochenkov petrochenkov deleted the pipdeny branch September 21, 2016 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@petrochenkov @bors @nikomatsakis @alexcrichton