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

Limit exported-private-dependencies lints to libraries #13135

Merged
merged 3 commits into from
Dec 11, 2023

Conversation

linyihai
Copy link
Contributor

@linyihai linyihai commented Dec 8, 2023

What does this PR try to resolve?

Completed #13039.

This PR limit exported-private-dependencies lint in libraray Target.

How should we test and review this PR?

Your can checkout out 2348ac2 and run test, it will failed and then it will be passed in the commit 2348ac2

Additional information

@rustbot
Copy link
Collaborator

rustbot commented Dec 8, 2023

r? @epage

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 8, 2023
@@ -246,3 +246,184 @@ fn workspace_dep_made_public() {
.masquerade_as_nightly_cargo(&["public-dependency"])
.run()
}

#[cargo_test(nightly, reason = "exported_private_dependencies lint is unstable")]
fn allow_priv_in_tests() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another one we should cover is build scripts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like proc-macros are lib's but I'm assuming those shouldn't be checked with exported_private_dependencies.

Example libs are an odd case.

I'm more mixed about crate types, like cdylibs.

@ehuss any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another one we should cover is build scripts.

OK, add a test for custom build script

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good if all libs warn just like a normal rlib. Any lib type can expose types within item signatures, and it seems like it would be good hygiene to be explicit about that.

For proc-macros, it is not possible to have public items (other than the proc-macros themselves), so it is not possible to "leak" anything AFAIK. But I wouldn't go out of my way to try to disable it there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll go for that. We can always re-visit.

tests/testsuite/pub_priv.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented Dec 8, 2023

Thanks for doing this! Overall, looks good!

@epage
Copy link
Contributor

epage commented Dec 11, 2023

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Dec 11, 2023

📌 Commit 58c673d has been approved by epage

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2023
@bors
Copy link
Contributor

bors commented Dec 11, 2023

⌛ Testing commit 58c673d with merge 7688d89...

@bors
Copy link
Contributor

bors commented Dec 11, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 7688d89 to master...

@bors bors merged commit 7688d89 into rust-lang:master Dec 11, 2023
20 checks passed
@linyihai linyihai deleted the limit-priv-to-library branch December 12, 2023 07:11
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 12, 2023
Update cargo

20 commits in 9787229614b27854cf73d57ffae430d7c1e6caa4..1aa9df1a5be205cce621f0bc0ea6062a5e22a98c
2023-12-06 02:29:23 +0000 to 2023-12-12 14:52:31 +0000
- crates-io: Add support for other 2xx HTTP status codes (rust-lang/cargo#13158)
- Remove the deleted feature test_2018_feature from the test (rust-lang/cargo#13156)
- refactor(schema): Remove reliance on cargo types (rust-lang/cargo#13154)
- fix(toml)!: Disallow `[lints]` in virtual workspaces (rust-lang/cargo#13155)
- Limit exported-private-dependencies lints to libraries (rust-lang/cargo#13135)
- chore: update to [email protected] (rust-lang/cargo#13148)
- Update curl-sys to bring in curl 8.5.0 (rust-lang/cargo#13147)
- chore: downgrade to openssl v1.1.1 (rust-lang/cargo#13144)
- fix: explicitly remap current dir by using `.` (rust-lang/cargo#13114)
- Don't rely on mtime to test changes (rust-lang/cargo#13143)
- refactor: Pull PackageIdSpec into schema (rust-lang/cargo#13128)
- fix: Print rustc messages colored on wincon (rust-lang/cargo#13140)
- Add a windows manifest file (rust-lang/cargo#13131)
- Avoid writing CACHEDIR.TAG if it already exists (rust-lang/cargo#13132)
- re-enable flaky tests thanks to update to `gix-config`. (rust-lang/cargo#11821) (rust-lang/cargo#13130)
- fix bash completion in directory with spaces (rust-lang/cargo#13126)
- test: re-ignore git auth tests for gitoxide (rust-lang/cargo#13129)
- fix(toml): Disallow inheriting of dependency public status (rust-lang/cargo#13125)
- re-enable previously disabled tests with Windows-specific fix (rust-lang/cargo#13117)
- refactor: Clarify PackageId constructor names (rust-lang/cargo#13123)
@rustbot rustbot added this to the 1.76.0 milestone Dec 12, 2023
@epage epage added the Z-public-dependency Nightly: public-dependency label Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-execution Area: anything dealing with executing the compiler S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. Z-public-dependency Nightly: public-dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants