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

CI accepted doctests that do not build (due to missing feature gates) #114838

Open
RalfJung opened this issue Aug 15, 2023 · 12 comments
Open

CI accepted doctests that do not build (due to missing feature gates) #114838

RalfJung opened this issue Aug 15, 2023 · 12 comments
Labels
P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

This will stop reproducing when #114837 lands, but right now you can click here, click on "Run", and playground opens and you click on "Run" again, and you get an error. How did CI accept a doctest that does not build?

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 15, 2023
@RalfJung RalfJung changed the title CI accepted doctests that do not build CI accepted doctests that do not build (due to missing feature gates) Aug 15, 2023
@waynr
Copy link
Contributor

waynr commented Aug 15, 2023

Also worth noting, I believe the doctests were passing for me when I ran them locally as well.

@RalfJung
Copy link
Member Author

Yeah I checked and they pass locally for me as well. I just don't understand how that's possible. Either rustdoc is doing something odd or this is caused by the special bootstrap magic. But I can't think of how that would lead to just ignoring a "missing feature gate" error...

It is a somewhat special feature gate since it is about the path, not the item. So maybe something in #95956 is going wrong in combination with how bootstrap sets up the sysroot, or so...? Cc @rust-lang/bootstrap @yaahc

@onur-ozkan
Copy link
Member

Thanks for the ping on bootstrap team, will check it by the coming weekend.

@RalfJung RalfJung added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 15, 2023
@RalfJung
Copy link
Member Author

FWIW when running the tests via https://github.com/rust-lang/miri-test-libstd/, the feature gate error does show up as expected. That's how I noticed this to begin with. That repo runs the tests without bootstrap but there are also some other differences (like setting the miri-test-libstd feature gate which clears out most of the crate -- but not when cfg(doctest) is set). With x.py I don't even know which crate core in the doctest refers to -- does it set up a sysroot and use that, or is this running "without sysroot" like the build of the main core crate?

@saethlin saethlin added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 15, 2023
@onur-ozkan
Copy link
Member

onur-ozkan commented Aug 22, 2023

The main problem behind this issue is that this function

/// Check whether a path is a `use` item that has been marked as unstable.
///
/// See issue #94972 for details on why this is a special case
fn is_unstable_reexport(tcx: TyCtxt<'_>, id: hir::HirId) -> bool {
can't detect unstable attributes which are defined at the top level of the modules when it's the bootstrap environment. cc @yaahc

If you swap

#![unstable(feature = "error_in_core", issue = "103765")]
with error_generic_member_access in this module, then same problem will happen for error_generic_member_access and error_in_core errors will work as expected.

I am not really sure if we should keep the T-bootstrap label for this issue.

@RalfJung
Copy link
Member Author

can't detect unstable attributes which are defined at the top level of the modules when it's the bootstrap environment.

But what is different in bootstrap that makes this happen?

@onur-ozkan
Copy link
Member

But what is different in bootstrap that makes this happen?

This is why..

rust/src/bootstrap/builder.rs

Lines 1725 to 1727 in b60e31b

if !mode.is_tool() {
cargo.env("RUSTC_FORCE_UNSTABLE", "1");
}

sorry for the noise, was debugging/looking on the wrong point:)

@RalfJung
Copy link
Member Author

I still don't get it. Note that all other unstable items are properly detected. E.g., removing the error_generic_member_access feature from the test errors as expected. This only affects some unstable things, like error_in_core -- the ones that are "path unstable" rather than "item unstable".

Why does that env var affect "path unstable" but not "item unstable"?

@onur-ozkan
Copy link
Member

Can you add force-unstable-if-unmarked rustcflag flag on miri-side tests? If you still encounter the errors(related to error_in_core), it is likely that there is another mysterious bootstrap magic that remains to be identified.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 24, 2023

No need to bother Miri. We can just try this file:

#![allow(unused)]
use core::error::{request_ref, Request};

fn main() {}

With rustc +nightly --edition 2018 this errors as expected. But with rustc +nightly --edition 2018 -Zforce-unstable-if-unmarked it builds despite the missing feature!

@RalfJung
Copy link
Member Author

See #97301 (comment) for a bit of discussion on how to best fix this.

@onur-ozkan onur-ozkan added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 25, 2023
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 28, 2023
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

(seems not a regression IIUC)

@rustbot label -I-prioritize +P-high

@rustbot rustbot added the P-high High priority label Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants