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

Don't merge cfg and doc(cfg) attributes for re-exports #113091

Merged
merged 4 commits into from
Dec 15, 2023

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jun 27, 2023

Fixes #112881.

Explanations

When re-exporting things with different cfgs there are two things that can happen:

  • The re-export uses a subset of cfgs, this subset is sufficient so that the item will appear exactly with the subset
  • The re-export uses a non-subset of cfgs (e.g. like the example I posted just above where the re-export is ungated), if the non-subset cfgs are active (e.g. compiling that example on windows) then this will be a compile error as the item doesn't exist to re-export, if the subset cfgs are active it behaves like 1.

Glob re-exports?

This only applies to non-glob inlined re-exports. For glob re-exports the item may or may not exist to be re-exported (potentially the cfgs on the path up until the glob can be removed, and only cfgs on the globbed item itself matter), for non-inlined re-exports see #85043.

cc @Nemo157
r? @notriddle

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 27, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Nemo157
Copy link
Member

Nemo157 commented Jun 27, 2023

The test failures are expected, my argument in the issue is that those tests are wrong. (I didn't go looking to see whether there's other tests that I'd expect to fail but didn't).

@GuillaumeGomez
Copy link
Member Author

Sorry should have precised: just realized that this wasn't ready yet. I'm finishing another PR first then coming back to this one.

@GuillaumeGomez
Copy link
Member Author

It's now ready. So in any case, this will need to go through an FCP. But I'm personally not a big fan of this change:

  • deprecated/unstable and equivalents are "inherited" by re-exports, so it's not coherent with this change.
  • Unlike other items, re-exports are a bit special since it's a same item with potentially extra conditions, extra documentation and another name. I think all such information should be available on the end re-export. Even if a missing cfg will trigger a compilation failure, I think it'd be a better user experience to show all needed requirements directly on the re-export instead of discovering them one by one through potentially multiple failures.

Anyway, that was my two cents on this. We'll see what the others think about it.

@est31
Copy link
Member

est31 commented Jun 28, 2023

Even if a missing cfg will trigger a compilation failure

The compilation failure will be triggered at the reexport site, or in other words, I'd consider it a bug if the item is not provided.

Another thing that speaks in favour of this PR is that there is often patterns like:

#[cfg(target_os = "linux")]
mod impl {
pub fn foo() { /* impl for linux */ }
}

#[cfg(target_os = "windows")]
mod impl {
pub fn foo() { /* impl for linux */ }
}

use impl::foo;

Here, there should ideally be no cfg documented as there is implementations for all operating systems.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Jun 28, 2023

But in this case, there is no cfg on foo directly, so nothing will be displayed, even currently. The equivalent would be (even if it doesn't compile but to get an idea):

#[cfg(target_os = "linux")]
pub fn foo() { /* impl for linux */ }

#[cfg(target_os = "windows")]
pub fn foo() { /* impl for windows */ }

pub use self::foo as reexport;

@Nemo157
Copy link
Member

Nemo157 commented Jun 28, 2023

But in this case, there is no cfg on foo directly, so nothing will be displayed, even currently.

No, cfg's are merged down onto the item before the re-export is inlined

#![feature(doc_auto_cfg)]

#[cfg(target_os = "linux")]
mod impl_ {
    pub fn foo() { /* impl for linux */ }
}

#[cfg(target_os = "macos")]
mod impl_ {
    pub fn foo() { /* impl for darwin */ }
}

pub use impl_::foo;
image

That doesn't really matter though, your example showing the cfg on the inlined re-export would also be wrong, since the public item is not gated on any platform the crate compiles on. (Except it's not inlined since the re-exported items are pub so my arguments (and hopefully this change) don't apply).

@Nemo157
Copy link
Member

Nemo157 commented Jun 28, 2023

(I would suggest maybe doing FCP in the issue since that's where I documented my rationale for the change).

@GuillaumeGomez
Copy link
Member Author

You could also start it here and add an explanation text too. As you prefer.

@Nemo157
Copy link
Member

Nemo157 commented Jun 28, 2023

@rfcbot pr merge

My reasoning for this change from the issue:

When re-exporting things with different cfgs there are two things that can happen:

  1. The re-export uses a subset of cfgs, this subset is sufficient so that the item will appear exactly with the subset
  2. The re-export uses a non-subset of cfgs (e.g. like the example I posted just above where the re-export is ungated), if the non-subset cfgs are active (e.g. compiling that example on windows) then this will be a compile error as the item doesn't exist to re-export, if the subset cfgs are active it behaves like 1.

This only applies to non-glob inlined re-exports, for glob re-exports the item may or may not exist to be re-exported (potentially the cfgs on the path up until the glob can be removed, and only cfgs on the globbed item itself matter, but I haven't thought through all the details), for non-inlined re-exports see #85043.

@rfcbot
Copy link

rfcbot commented Jun 28, 2023

Team member @Nemo157 has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 28, 2023
@bors
Copy link
Contributor

bors commented Jul 28, 2023

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

@GuillaumeGomez
Copy link
Member Author

Fixed merge conflict.

@bors
Copy link
Contributor

bors commented Nov 16, 2023

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

@GuillaumeGomez
Copy link
Member Author

Fixed merged conflict.

@rfcbot
Copy link

rfcbot commented Dec 4, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 14, 2023
@rfcbot
Copy link

rfcbot commented Dec 14, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@GuillaumeGomez
Copy link
Member Author

@bors r=rustdoc

@bors
Copy link
Contributor

bors commented Dec 15, 2023

📌 Commit 823148f has been approved by rustdoc

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 15, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 15, 2023
…llaumeGomez

Rollup of 4 pull requests

Successful merges:

 - rust-lang#113091 (Don't merge cfg and doc(cfg) attributes for re-exports)
 - rust-lang#115660 (rustdoc: allow resizing the sidebar / hiding the top bar)
 - rust-lang#118863 (rustc_mir_build: Enforce `rustc::potential_query_instability` lint)
 - rust-lang#118909 (Some cleanup and improvement for invalid ref casting impl)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ec0008a into rust-lang:master Dec 15, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 15, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 15, 2023
Rollup merge of rust-lang#113091 - GuillaumeGomez:prevent-cfg-merge-reexport, r=rustdoc

Don't merge cfg and doc(cfg) attributes for re-exports

Fixes rust-lang#112881.

## Explanations

When re-exporting things with different `cfg`s there are two things that can happen:

 * The re-export uses a subset of `cfg`s, this subset is sufficient so that the item will appear exactly with the subset
 * The re-export uses a non-subset of `cfg`s (e.g. like the example I posted just above where the re-export is ungated), if the non-subset `cfg`s are active (e.g. compiling that example on windows) then this will be a compile error as the item doesn't exist to re-export, if the subset `cfg`s are active it behaves like 1.

### Glob re-exports?

**This only applies to non-glob inlined re-exports.** For glob re-exports the item may or may not exist to be re-exported (potentially the `cfg`s on the path up until the glob can be removed, and only `cfg`s on the globbed item itself matter), for non-inlined re-exports see rust-lang#85043.

cc `@Nemo157`
r? `@notriddle`
@bors
Copy link
Contributor

bors commented Dec 15, 2023

⌛ Testing commit 823148f with merge 4d1bd0d...

@GuillaumeGomez GuillaumeGomez deleted the prevent-cfg-merge-reexport branch December 15, 2023 14:52
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 28, 2023
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 30, 2024
…merge-bugfix, r=notriddle

rustdoc: Correctly handle attribute merge if this is a glob reexport

Fixes rust-lang#120487.

The regression was introduced in rust-lang#113091. Only non-glob reexports should have been impacted.

cc `@Nemo157`
r? `@notriddle`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 30, 2024
…merge-bugfix, r=notriddle

rustdoc: Correctly handle attribute merge if this is a glob reexport

Fixes rust-lang#120487.

The regression was introduced in rust-lang#113091. Only non-glob reexports should have been impacted.

cc ``@Nemo157``
r? ``@notriddle``
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Jan 31, 2024
…merge-bugfix, r=notriddle

rustdoc: Correctly handle attribute merge if this is a glob reexport

Fixes rust-lang#120487.

The regression was introduced in rust-lang#113091. Only non-glob reexports should have been impacted.

cc ```@Nemo157```
r? ```@notriddle```
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Jan 31, 2024
…merge-bugfix, r=notriddle

rustdoc: Correctly handle attribute merge if this is a glob reexport

Fixes rust-lang#120487.

The regression was introduced in rust-lang#113091. Only non-glob reexports should have been impacted.

cc ````@Nemo157````
r? ````@notriddle````
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Jan 31, 2024
…merge-bugfix, r=notriddle

rustdoc: Correctly handle attribute merge if this is a glob reexport

Fixes rust-lang#120487.

The regression was introduced in rust-lang#113091. Only non-glob reexports should have been impacted.

cc `````@Nemo157`````
r? `````@notriddle`````
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2024
Rollup merge of rust-lang#120501 - GuillaumeGomez:glob-reexport-attr-merge-bugfix, r=notriddle

rustdoc: Correctly handle attribute merge if this is a glob reexport

Fixes rust-lang#120487.

The regression was introduced in rust-lang#113091. Only non-glob reexports should have been impacted.

cc `````@Nemo157`````
r? `````@notriddle`````
wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request Feb 18, 2024
Pkgsrc changes:
 * Adapt checksums and patches.

Upstream chnages:

Version 1.76.0 (2024-02-08)
==========================

Language
--------
- [Document Rust ABI compatibility between various types]
  (rust-lang/rust#115476)
- [Also: guarantee that char and u32 are ABI-compatible]
  (rust-lang/rust#118032)
- [Warn against ambiguous wide pointer comparisons]
  (rust-lang/rust#117758)

Compiler
--------
- [Lint pinned `#[must_use]` pointers (in particular, `Box<T>`
  where `T` is `#[must_use]`) in `unused_must_use`.]
  (rust-lang/rust#118054)
- [Soundness fix: fix computing the offset of an unsized field in
  a packed struct]
  (rust-lang/rust#118540)
- [Soundness fix: fix dynamic size/align computation logic for
  packed types with dyn Trait tail]
  (rust-lang/rust#118538)
- [Add `$message_type` field to distinguish json diagnostic outputs]
  (rust-lang/rust#115691)
- [Enable Rust to use the EHCont security feature of Windows]
  (rust-lang/rust#118013)
- [Add tier 3 {x86_64,i686}-win7-windows-msvc targets]
  (rust-lang/rust#118150)
- [Add tier 3 aarch64-apple-watchos target]
  (rust-lang/rust#119074)
- [Add tier 3 arm64e-apple-ios & arm64e-apple-darwin targets]
  (rust-lang/rust#115526)

Refer to Rust's [platform support page][platform-support-doc]
for more information on Rust's tiered platform support.

Libraries
---------
- [Add a column number to `dbg!()`]
  (rust-lang/rust#114962)
- [Add `std::hash::{DefaultHasher, RandomState}` exports]
  (rust-lang/rust#115694)
- [Fix rounding issue with exponents in fmt]
  (rust-lang/rust#116301)
- [Add T: ?Sized to `RwLockReadGuard` and `RwLockWriteGuard`'s Debug impls.]
  (rust-lang/rust#117138)
- [Windows: Allow `File::create` to work on hidden files]
  (rust-lang/rust#116438)

Stabilized APIs
---------------
- [`Arc::unwrap_or_clone`]
  (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.unwrap_or_clone)
- [`Rc::unwrap_or_clone`]
  (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.unwrap_or_clone)
- [`Result::inspect`]
  (https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.inspect)
- [`Result::inspect_err`]
  (https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.inspect_err)
- [`Option::inspect`]
  (https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.inspect)
- [`type_name_of_val`]
  (https://doc.rust-lang.org/stable/std/any/fn.type_name_of_val.html)
- [`std::hash::{DefaultHasher, RandomState}`]
  (https://doc.rust-lang.org/stable/std/hash/index.html#structs)
  These were previously available only through `std::collections::hash_map`.
- [`ptr::{from_ref, from_mut}`]
  (https://doc.rust-lang.org/stable/std/ptr/fn.from_ref.html)
- [`ptr::addr_eq`](https://doc.rust-lang.org/stable/std/ptr/fn.addr_eq.html)

Cargo
-----

See [Cargo release notes]
(https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-176-2024-02-08).

Rustdoc
-------
- [Don't merge cfg and doc(cfg) attributes for re-exports]
  (rust-lang/rust#113091)
- [rustdoc: allow resizing the sidebar / hiding the top bar]
  (rust-lang/rust#115660)
- [rustdoc-search: add support for traits and associated types]
  (rust-lang/rust#116085)
- [rustdoc: Add highlighting for comments in items declaration]
  (rust-lang/rust#117869)

Compatibility Notes
-------------------
- [Add allow-by-default lint for unit bindings]
  (rust-lang/rust#112380)
  This is expected to be upgraded to a warning by default in a future Rust
  release. Some macros emit bindings with type `()` with user-provided spans,
  which means that this lint will warn for user code.
- [Remove x86_64-sun-solaris target.]
  (rust-lang/rust#118091)
- [Remove asmjs-unknown-emscripten target]
  (rust-lang/rust#117338)
- [Report errors in jobserver inherited through environment variables]
  (rust-lang/rust#113730)
  This [may warn](rust-lang/rust#120515)
  on benign problems too.
- [Update the minimum external LLVM to 16.]
  (rust-lang/rust#117947)
- [Improve `print_tts`](rust-lang/rust#114571)
  This change can break some naive manual parsing of token trees
  in proc macro code which expect a particular structure after
  `.to_string()`, rather than just arbitrary Rust code.
- [Make `IMPLIED_BOUNDS_ENTAILMENT` into a hard error from a lint]
  (rust-lang/rust#117984)
- [Vec's allocation behavior was changed when collecting some iterators]
  (rust-lang/rust#110353)
  Allocation behavior is currently not specified, nevertheless
  changes can be surprising.
  See [`impl FromIterator for Vec`]
  (https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#impl-FromIterator%3CT%3E-for-Vec%3CT%3E)
  for more details.
- [Properly reject `default` on free const items]
  (rust-lang/rust#117818)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doc_cfg should not merge cfgs on non-glob inlined reexports
9 participants