Inject `compiler_builtins` during postprocessing and ensure it is made private by tgross35 · Pull Request #135501 · rust-lang/rust

tgross35 · 2025-01-14T20:06:42Z

Follow up of #135278

Do the following:

Inject compiler_builtins during postprocessing, rather than injecting extern crate compiler_builtins as _ into the AST
Do not make dependencies of std private by default (this was added in Exclude dependencies of std for diagnostics #135278)
Make sure sysroot crates correctly mark their dependencies private/public
Ensure that marking a dependency private makes its dependents private by default as well, unless otherwise specified
Do the compiler_builtins update that has been blocked on this

There 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
try-job: x86_64-msvc-2
try-job: i686-mingw-1
try-job: i686-mingw-2

tgross35 · 2025-01-14T20:08:48Z

The test still fails, and I cannot chase down how gimli is getting into the sysroot as non-private. Locally I am testing with submodules updated too.

r? bjorn3

bjorn3 · 2025-01-20T15:28:00Z

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?

tgross35 · 2025-01-20T19:28:15Z

Do you mean to do that as part of the is_private_dep check?

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.

bjorn3 · 2025-01-23T10:29:08Z

Do you mean to do that as part of the is_private_dep check?

Yes

tgross35 · 2025-01-28T21:26:17Z

It looks like the reason compiler_builtins is not showing up as private is because it shows up as an extern crate item with a no-location span, which gets picked up at

rust/compiler/rustc_resolve/src/build_reduced_graph.rs

Line 1321 in 77a4553

self.build_reduced_graph_for_item(item);

. I'm still trying to figure out where this would come from.

tgross35 · 2025-01-28T21:34:11Z

This looks like a possibility

rust/compiler/rustc_builtin_macros/src/standard_library_imports.rs

Lines 49 to 61 in 77a4553

    
           let item = if name == sym::compiler_builtins { 
        
               // compiler_builtins is a private implementation detail. We only 
        
               // need to insert it into the crate graph for linking and should not 
        
               // expose any of its public API. 
        
               // 
        
               // FIXME(#113634) We should inject this during post-processing like 
        
               // we do for the panic runtime, profiler runtime, etc. 
        
               cx.item( 
        
                   span, 
        
                   Ident::new(kw::Underscore, ident_span), 
        
                   thin_vec![], 
        
                   ast::ItemKind::ExternCrate(Some(name)), 
        
               )

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)

tgross35 · 2025-01-28T23:25:06Z

@bors try

…, 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

bors · 2025-02-20T19:21:42Z

⌛ Trying commit 0827f25 with merge 95f832d...

bors · 2025-02-20T21:59:09Z

☀️ Try build successful - checks-actions
Build commit: 95f832d (95f832deb297cbe86639f86bcc85ba151b687dc5)

tgross35 · 2025-02-20T22:19:15Z

@rustbot ready

bjorn3 · 2025-02-21T09:21:08Z

Looks like you still have the DO NOT MERGE commit.

`compiler_builtins` is currently injected as `extern crate compiler_builtins as _`. This has made gating via diagnostics difficult because it appears in the crate graph as a non-private dependency, and there isn't an easy way to differentiate between the injected AST and user-specified `extern crate compiler_builtins`. Resolve this by injecting `compiler_builtins` during postprocessing rather than early in the AST. Most of the time this isn't even needed because it shows up in `std` or `core`'s crate graph, but injection is still needed to ensure `#![no_core]` works correctly. A similar change was attempted at [1] but this encountered errors building `proc_macro` and `rustc-std-workspace-std`. Similar failures showed up while working on this patch, which were traced back to `compiler_builtins` showing up in the graph twice (once via dependency and once via injection). This is resolved by not injecting if a `#![compiler_builtins]` crate already exists. [1]: rust-lang#113634

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 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)`.

tgross35 · 2025-02-21T17:38:00Z

I kept that in case a retest with further changes was needed. Dropped it now and squashed the last fixup.

bjorn3 · 2025-02-21T18:51:54Z

@bors r+

bors · 2025-02-21T18:51:57Z

📌 Commit 9392580 has been approved by bjorn3

It is now in the queue for this repository.

…te, r=bjorn3 Inject `compiler_builtins` during postprocessing and ensure it is made private Follow up of rust-lang#135278 Do the following: * Inject `compiler_builtins` during postprocessing, rather than injecting `extern crate compiler_builtins as _` into the AST * Do not make dependencies of `std` private by default (this was added in rust-lang#135278) * Make sure sysroot crates correctly mark their dependencies private/public * Ensure that marking a dependency private makes its dependents private by default as well, unless otherwise specified * Do the `compiler_builtins` update that has been blocked on this There is more detail in the commit messages. This includes the changes I was working on in rust-lang#136226. try-job: test-various try-job: x86_64-msvc-1 try-job: x86_64-msvc-2 try-job: i686-mingw-1 try-job: i686-mingw-2

rustbot assigned chenyukang Jan 14, 2025

This comment was marked as outdated.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 14, 2025

tgross35 marked this pull request as draft January 14, 2025 20:07

rustbot assigned bjorn3 and unassigned chenyukang Jan 14, 2025

tgross35 mentioned this pull request Jan 14, 2025

Exclude dependencies of std for diagnostics #135278

Merged

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

tgross35 force-pushed the stdlib-dependencies-private branch from 560b580 to ee49697 Compare January 28, 2025 21:23

This comment has been minimized.

Sign in to view

tgross35 force-pushed the stdlib-dependencies-private branch 2 times, most recently from b32d8d1 to 7a6a6cd Compare January 28, 2025 22:45

This comment has been minimized.

Sign in to view

tgross35 force-pushed the stdlib-dependencies-private branch from 5354933 to 9c9ea48 Compare January 28, 2025 23:24

tgross35 changed the title ~~[WIP] Mark sysroot dependencies private~~ Resolve compiler_builtins not being treated as private; clean up #135278 Jan 28, 2025

This comment was marked as outdated.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 20, 2025

tgross35 added 5 commits February 21, 2025 17:37

Partially revert ed63539 ("Mark dependencies ... private by default")

cb1d076

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`.

Replace some instances of pub with pub(crate)

9392580

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)`.

tgross35 force-pushed the stdlib-dependencies-private branch from 0827f25 to 9392580 Compare February 21, 2025 17:37

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 Feb 21, 2025

matthiaskrgr mentioned this pull request Feb 22, 2025

Rollup of 8 pull requests #137431

Closed

This was referenced Feb 22, 2025

Rollup of 7 pull requests #137433

Closed

Rollup of 7 pull requests #137436

Closed

Rollup of 6 pull requests #137446

Merged

ShoyuVanilla mentioned this pull request Feb 23, 2025

fix: Calculate the privacy of dependencies wrt current local crate #137441

Closed

This was referenced Feb 23, 2025

exported_private_dependencies lint only take effect in innermost dependency #119428

Open

Eliminate the use of public_test_dep! for a third time rust-lang/compiler-builtins#766

Merged

byeongkeunahn mentioned this pull request Mar 1, 2025

compiler_builtins string.h functions are not available when using std targets with #![no_std] #137833

Open

epage mentioned this pull request Mar 17, 2025

Stabilize public/private dependencies rust-lang/rust-project-goals#272

Open

10 tasks

jieyouxu mentioned this pull request May 1, 2025

Suppress suggestions while span is in external library #139316

Closed

tgross35 mentioned this pull request Jun 19, 2025

rustc suggest adding an import of compiler_builtins #142676

Closed

Uh oh!

Conversation

tgross35 commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

tgross35 commented Jan 14, 2025

Uh oh!

This comment has been minimized.

This comment was marked as outdated.

bjorn3 commented Jan 20, 2025

Uh oh!

tgross35 commented Jan 20, 2025

Uh oh!

bjorn3 commented Jan 23, 2025

Uh oh!

tgross35 commented Jan 28, 2025

Uh oh!

tgross35 commented Jan 28, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tgross35 commented Jan 28, 2025

Uh oh!

This comment was marked as outdated.

bors commented Feb 20, 2025

Uh oh!

bors commented Feb 20, 2025

Uh oh!

tgross35 commented Feb 20, 2025

Uh oh!

bjorn3 commented Feb 21, 2025

Uh oh!

tgross35 commented Feb 21, 2025

Uh oh!

bjorn3 commented Feb 21, 2025

Uh oh!

bors commented Feb 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tgross35 commented Jan 14, 2025 •

edited

Loading