Revert "Set test flag when rustdoc is running with --test option"

[ollie27]

Reverts #59940.

It caused doctests in this repository to no longer be tested including all of the core crate.

[ollie27]

r? @QuietMisdreavus

This will need backporting to beta as well. cc. @rust-lang/rustdoc

[GuillaumeGomez]

Just to be sure, the test flag will still be passed when running rustdoc in test mode or not?

[ollie27]

Just to be sure, the test flag will still be passed when running rustdoc in test mode or not?

This reverts back to the behaviour before #59940 so cfg(test) will not be set when running rustdoc --test.

[GuillaumeGomez]

Then it's a problem. This is specifically why it was added in the first place. Using cfg(rustdoc) would very certainly fix the case you're talking about.

[ollie27]

Reverting is the quickest way to get the doctests in core running again. Using cfg(rustdoc) would work but I still don't know if any crates outside of std were affected and cfg(rustdoc) won't work for them because it's still unstable.

[QuietMisdreavus]

and cfg(rustdoc) won't work for them because it's still unstable.

So i guess we can block this feature on making cfg(rustdoc) stable, which we wanted to do anyway. I'm not sure how prevalent cfg(not(test)) is outside libcore, but we can at least provide make sure to provide a stable alternative for people.

I'm okay with this PR, and with the backport. Just one extra comment and i think it's fine.

[GuillaumeGomez]

That makes me deeply sad considering how many crates will be impacted by this revert but whatever...

[Mark-Simulacrum]

We should call this out in relnotes since I believe the current behavior of (essentially) --cfg test being passed by rustdoc during doctests is on stable already; this is a backwards compatibility hazard. I'm not actually sure that we should be readily reverting this patch but if rustdoc team thinks it's acceptable then that seems good enough, I suppose.

@rustbot modify labels: relnotes

[RalfJung]

This reverts #59940, right? That one is on beta but not stable (according to git branch --contains 459d677bfffa0adeef75218d1cfa5f686e413f4d).

So, nobody should really be impacted by this revert?

[GuillaumeGomez]

All the crates using doc-comment will be. Also I'm not satisfied at all with this revert but I think the plan is first to stabilize cfg(rustdoc) and then we can stabilize this feature again.

[RalfJung]

But this is a revert before things hit stable, so nothing should change compared to stable...? I am confused.^^

[GuillaumeGomez]

People are already using this feature in nightly (and beta too now). If this remained in nightly, it wouldn't be an issue but we're removing it. Like I already said, I'm strongly against this revert. However an easy fix would be to activate it only on nightly for the moment until cfg(rustdoc) is stabilized.

What do you think of this @QuietMisdreavus ?

[RalfJung]

This is the first time that I hear "people are using it on beta/nightly" as an argument against reverting a change that breaks things. That's what nightly/beta are for -- testing features so that we can take them back if they don't work.

[QuietMisdreavus]

@Mark-Simulacrum As @RalfJung confirmed, this behavior only just hit beta. It's actually a common point of confusion that rustdoc doesn't pass cfg(test) at all to anything; that's something the original feature was intended to correct. However, it appears to have introduced a hole where some tests that were previously running aren't any more, at least in libcore.

@rustbot modify labels: -relnotes

@GuillaumeGomez The reason of "people are already using this feature" is not a good enough excuse for changing it before it hits stable. The compiler and standard library have "un-stabilized" features before hitting stable before for their own reasons - that's the entire purpose of having the release train. In this case we're upholding the behavior that's currently on stable so that test behavior can remain consistent there.

(Also, i'm not seeing where doc-comment forces people to rely on cfg(test) behavior? It doesn't seem to bake it into the macro output, so users would have to add it themselves. I'm not sure how widely publicized the feature was to begin with, so i doubt it's as widespread as you think.)

An alternative would be to only apply cfg(test) on nightly until we stabilize cfg(rustdoc), but i'm hesitant to do that, since the most prominent crate that's affected by this (libcore) is always compiled with nightly features enabled. We would also force users who are already relying on this feature to add #![feature(doc_cfg)] to their project for little reason.

[GuillaumeGomez]

@GuillaumeGomez The reason of "people are already using this feature" is not a good enough excuse for changing it before it hits stable. The compiler and standard library have "un-stabilized" features before hitting stable before for their own reasons - that's the entire purpose of having the release train. In this case we're upholding the behavior that's currently on stable so that test behavior can remain consistent there.

I agree, that's why I suggested to only activate it for nightly for the moment. It'll prevent the "hole" and that'll leave me time to check what's going on. However, we should definitely stabilize cfg(rustdoc).

[bors]

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

[bors]

⌛ Testing commit c77024c with merge 87ceb865a18ef30ec591044776e3539c3f98cdda...

[bors]

💔 Test failed - checks-travis

[rust-highfive]

The job x86_64-gnu-tools of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[01:53:55] 
[01:53:55] failures:
[01:53:55] 
[01:53:55] ---- client_find_definitions stdout ----
[01:53:55] Sending: Object({"id": Number(0), "jsonrpc": String("2.0"), "method": String("initialize"), "params": Object({"capabilities": Object({"window": Object({"progress": Bool(true)})}), "initializationOptions": Object({"settings": Object({"rust": Object({"all_targets": Bool(false), "racer_completion": Bool(false)})})}), "processId": Null, "rootPath": String("/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/rlsit/t11/simple_workspace"), "rootUri": Null})})
[01:53:55] Processing message: Object({"jsonrpc": String("2.0"), "method": String("window/progress"), "params": Object({"done": Null, "id": String("progress_1"), "message": Null, "percentage": Null, "title": String("Building")})})
[01:53:55] Processing message: Object({"jsonrpc": String("2.0"), "method": String("window/progress"), "params": Object({"done": Null, "id": String("progress_1"), "message": String("bar"), "percentage": Null, "title": String("Building")})})
[01:53:55] Processing message: Object({"jsonrpc": String("2.0"), "method": String("window/progress"), "params": Object({"done": Bool(true), "id": String("progress_1"), "message": Null, "percentage": Null, "title": String("Building")})})
[01:53:55] Processing message: Object({"jsonrpc": String("2.0"), "method": String("window/progress"), "params": Object({"done": Null, "id": String("progress_0"), "message": Null, "percentage": Null, "title": String("Indexing")})})
---
[01:53:55] Sending: Object({"id": Number(1202), "jsonrpc": String("2.0"), "method": String("textDocument/definition"), "params": Object({"position": Object({"character": Number(2), "line": Number(12)}), "textDocument": Object({"uri": String("file:///checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/rlsit/t11/simple_workspace/src/main.rs")})})})
[01:53:55] Processing message: Object({"id": Number(1202), "jsonrpc": String("2.0"), "result": Array([])})
[01:53:55] Sending: Object({"id": Number(1203), "jsonrpc": String("2.0"), "method": String("textDocument/definition"), "params": Object({"position": Object({"character": Number(3), "line": Number(12)}), "textDocument": Object({"uri": String("file:///checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/rlsit/t11/simple_workspace/src/main.rs")})})})
[01:53:55] Processing message: Object({"id": Number(1203), "jsonrpc": String("2.0"), "result": Array([])})
[01:53:55] thread 'client_find_definitions' panicked at 'Got different amount of completions than expected: 18 vs. 25: [
[01:53:55]         4,
[01:53:55]         16,
[01:53:55]         [
[01:53:55]             Range {

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[RalfJung]

Looks like one of these spurious RLS failures that we have been seeing for a long while already:

[01:57:39] The state of "rls" has changed from "test-pass" to "test-fail"
[01:57:39] The state of "rls" has regressed from "test-pass" to "test-fail"

The failing test is client_find_definitions.

@bors retry

[bors]

⌛ Testing commit c77024c with merge 5790aabc7da892c527bdfbd71e5b2f35ed722cff...

[Centril]

@bors retry

[bors]

⌛ Testing commit c77024c with merge 9a90d03...

[bors]

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