-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Restrict sysroot crate imports to those defined in this repo. #143548
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
Conversation
Some changes occurred in compiler/rustc_codegen_ssa
Some changes occurred in compiler/rustc_codegen_cranelift
cc @bjorn3
Some changes occurred in src/tools/clippy
cc @rust-lang/clippy
Some changes occurred in src/tools/compiletest
cc @jieyouxu
Some changes occurred in src/tools/rustfmt
cc @rust-lang/rustfmt
Some changes occurred in compiler/rustc_attr_data_structures
Some changes occurred in compiler/rustc_attr_parsing
Some changes occurred in compiler/rustc_codegen_gcc
The Miri subtree was changed
cc @rust-lang/miri
Some changes occurred in exhaustiveness checking
cc @Nadrieril
Some changes occurred in compiler/rustc_sanitizers
cc @rcvalle
rustc_error_messages was changed
8e8dd82 to
c613bce
Compare
The Miri subtree was changed
cc @rust-lang/miri
Some changes occurred in src/tools/clippy
cc @rust-lang/clippy
Some changes occurred in compiler/rustc_codegen_cranelift
cc @bjorn3
Some changes occurred in src/tools/rustfmt
cc @rust-lang/rustfmt
Some changes occurred in compiler/rustc_codegen_gcc
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Miri changes LGTM, except the ones in the vendored file where only the header at the top should be edited.
I'm fine with having these Miri changes here, seems easier to do this centrally than coordinate half a dozen PRs.
Do you think it is feasible to add an internal lint that rejects extern crate items where the crate name doesn't start with rustc_ or sth like that?
@oli-obk I'll look into this. How does testing work for sub-trees? Would each sub-tree need its own copy of the lint, or do the subtree repos have some way to run rust-wide lints?
"internal" lints live in rustc like normal lints, but are gated behind a nightly-only flag. Miri enables them in their CI, I don't know if ./x check miri also enables them.
We have a list of internal lints that rustc has and enables everywhere. We'd have to limit it to crates with #![allow(internal_features)] or sth like it. Most other lints are obviously limited to that as they look for e.g. TyKind .
Not sure what you mean about restricting it, the lints need -Zunstable-options -Wrustc::internal to be enabled, right?
Ah right, my bad
6dd804b to
8561f09
Compare
This comment has been minimized.
This comment has been minimized.
8561f09 to
2235afc
Compare
This comment has been minimized.
This comment has been minimized.
2235afc to
8dd8124
Compare
This comment has been minimized.
This comment has been minimized.
8dd8124 to
a79cac5
Compare
This comment has been minimized.
This comment has been minimized.
a79cac5 to
12fe444
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extern crate self is not a thing, or is it...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is. You can for example do extern crate self as foo;.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah core uses extern crate self as core
It's common to import dependencies from the sysroot via `extern crate` rather than use an explicit cargo dependency, when it's necessary to use the same dependency version as used by rustc itself. However, this is dangerous for crates.io crates, since rustc may not pull in the dependency on some targets, or may pull in multiple versions. In both cases, the `extern crate` fails to resolve. To address this, re-export all such dependencies from the appropriate `rustc_*` crates, and use this alias from crates which would otherwise need to use `extern crate`.
12fe444 to
3565a59
Compare
I've updated the PR and added a lint, so I think this is ready to go.
There is one problem which is that the lint is an internal rustc lint and so is not automatically applied to all of the tool crates which it should be. It will be necessary to opt-in to rustc internal lints in each of the tools. Clippy already opts in, and possibly Miri? But other tool crates will have to opt-in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cg_gcc part looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cg_clif changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Miri changes LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"dangerous" is not very descriptive. some ideas:
- conflict_prone_extern_crate
- implicit_sysroot_crate_import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh oh!
There was an error while loading. Please reload this page.
It's common to import dependencies from the sysroot via
extern craterather than use an explicit cargo dependency, when it's necessary to use the same dependency version as used by rustc itself. However, this is dangerous for crates.io crates, since rustc may not pull in the dependency on some targets, or may pull in multiple versions. In both cases, theextern cratefails to resolve.To address this, re-export all such dependencies from the appropriate
rustc_*crates, and use this alias from crates which would otherwise need to useextern crate.See #143492 for an example of the kind of issue that can occur.