-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Inject compiler_builtins
during postprocessing and ensure it is made private
#135501
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
The test still fails, and I cannot chase down how r? bjorn3 |
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Maybe you can make the code that checks if a crate is private for diagnostics check if all paths to reach a dependency are private and if so consider the dependency to be private even if the direct dependent crate doesn't mark it as private? |
Do you mean to do that as part of the I am apparently missing some path since exposing more traits in builtins seems to leak into diagnostics still #135560. Haven't looked into it yet, but I suppose that might fix it. |
Yes |
560b580
to
ee49697
Compare
It looks like the reason
|
This looks like a possibility rust/compiler/rustc_builtin_macros/src/standard_library_imports.rs Lines 49 to 61 in 77a4553
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
b32d8d1
to
7a6a6cd
Compare
This comment has been minimized.
This comment has been minimized.
Currently, marking a dependency private does not automatically make all its child dependencies private. Resolve this by making its children private by default as well. [1]: rust-lang#135501 (comment)
Currently, marking a dependency private does not automatically make all its child dependencies private. Resolve this by making its children private by default as well. [1]: rust-lang#135501 (comment)
5354933
to
9c9ea48
Compare
@bors try |
compiler_builtins
not being treated as private; clean up #135278
…, r=<try> Resolve `compiler_builtins` not being treated as private; clean up rust-lang#135278 Follow up of rust-lang#135278 try-job: test-various
This comment was marked as outdated.
This comment was marked as outdated.
Remove the portion of ed63539 that automatically sets crates private based on whether they are dependencies of `std`. Instead, this is controlled by dependency configuration in `Cargo.toml`.
In [1], most dependencies of `std` and other sysroot crates were marked private, but this did not happen for `alloc` and `test`. Update these here, marking public standard library crates as the only non-private dependencies. [1]: rust-lang#111076
Currently, marking a dependency private does not automatically make all its child dependencies private. Resolve this by making its children private by default as well. This also resolves some FIXMEs for tests that are intended to fail but previously passed. [1]: rust-lang#135501 (comment)
The change between 0.1.143 and 0.1.144 includes refactoring that was in compiler-builtins before, but had to be reverted before landing in rust-lang/rust because the traits were leaking into diagnostics [1]. Recently a fix for this issue was merged [2] so the cleanup is reapplied here. This also acts as a regression test for [2]. [1]: rust-lang#128691 (comment) [2]: rust-lang#135278
The recent fixes to private dependencies exposed some cases in the UEFI module where private dependencies are exposed in a public interface. These do not need to be crate-public, so change them to `pub(crate)`.
1db60e8
to
530eaf1
Compare
@bors try |
…, r=<try> Resolve `compiler_builtins` not being treated as private; clean up rust-lang#135278 Follow up of rust-lang#135278 Do the following (one per commit): * Do not make dependencies of `std` private by default * Update remaining sysroot crates to use `public-dependencies` * Ensure that marking a dependency private makes its dependents private by default as well * Do the `compiler_builtins` update that has been blocked on this Based on top of rust-lang#136226 so there are a few preceding commits. try-job: test-various try-job: x86_64-msvc-1
compiler_builtins
not being treated as private; clean up #135278compiler_builtins
as private; clean up #135278
☀️ Try build successful - checks-actions |
compiler_builtins
as private; clean up #135278compiler_builtins
during postprocessing and ensure it is made private
Update to LLVM 20 LLVM 20 GA is scheduled for March 11th. Rust 1.86 will be stable on April 3rd. * [x] rust-lang#135764 * [x] rust-lang#136134 * [x] rust-lang/compiler-builtins#752 * [ ] llvm/llvm-project#125287 * [ ] Update compiler-builtins (blocked on rust-lang#135501) Tested: host-x86_64, host-aarch64, apple, mingw try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: dist-aarch64-msvc try-job: x86_64-msvc-ext1 try-job: x86_64-msvc-ext2 try-job: x86_64-msvc-ext3
[1] has not gone forward, so this needs to be reverted in order to unblock a compiler-builtins upgrade that is is blocking the LLVM 20 upgrade. This reverts commit a2494f1. [1]: rust-lang/rust#135501
[1] has not gone forward, so this needs to be reverted again in order to unblock a compiler-builtins upgrade that is necessary for the LLVM 20 upgrade. This reverts commit a2494f1. [1]: rust-lang/rust#135501
[1] has not gone forward, so this needs to be reverted again in order to unblock a compiler-builtins upgrade that is necessary for the LLVM 20 upgrade. This reverts commit a2494f1. [1]: rust-lang/rust#135501
} | ||
|
||
// If a `#![compiler_builtins]` crate already exists, avoid injecting it twice. This is | ||
// the common case since usually it appears as a dependency of `std` or `core`. |
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.
of std
and alloc
. core
doesn't depend on compiler_builtins
. That is why we need this injection code in the first place.
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.
This just needs a comment update rather than behavior change, correct?
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.
Correct
) else { | ||
info!("`compiler_builtins` not resolved"); | ||
return; | ||
}; |
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.
Why has this been changed from a hard error to silently omitting the compiler-builtins dependency?
@@ -414,10 +451,14 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> { | |||
|
|||
let extern_private = self.sess.opts.externs.get(name.as_str()).map(|e| e.is_private_dep); | |||
|
|||
if matches!(origin, CrateOrigin::Injected) { | |||
return true; | |||
} |
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.
Maybe put this check above the let extern_private = ...
?
/// Part of the extern prelude. | ||
ExternPrelude, | ||
/// Provided by `extern crate foo`. | ||
AstExtern, |
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.
Do we actually need to distinguish between ForcedExtern
, ExternPrelude
and AstExtern
? If not I did prefer merging them into a single variant.
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.
They don’t have any behavior difference, so I’ll merge them
#[derive(Clone, Copy)] | ||
enum CrateOrigin<'a> { | ||
/// This crate was a dependency of another crate. | ||
Dependency { |
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.
Maybe IndirectDependency
? All variants of this enum are for dependencies of the current crate.
Follow up of #135278
Do the following:
compiler_builtins
during postprocessing, rather than injectingextern crate compiler_builtins as _
into the ASTstd
private by default (this was added in Exclude dependencies ofstd
for diagnostics #135278)compiler_builtins
update that has been blocked on thisThere is more detail in the commit messages. This includes the changes I was working on in #136226.
try-job: test-various
try-job: x86_64-msvc-1
r? @bjorn3