Remove a few `actually_rustdoc` uses #107289

oli-obk · 2023-01-25T10:51:29Z

r? @compiler-errors because it removes the hack from

Delay ambiguity span bug in normalize query iff not rustdoc #103368

cc @jyn514 this PR removes a bunch of your FIXME comments from the compiler.

Notable change in this PR is that rustdoc now ignores delay_span_bug in general, which I think is reasonable, considering rustdoc compiles invalid Rust code (multiple items with the same name, but from different cfgs).

oli-obk · 2023-01-25T10:51:38Z

@bors try @rust-timer queue

rustbot · 2023-01-25T10:51:38Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @compiler-errors (or someone else) soon.

Please see the contribution instructions for more information.

bors · 2023-01-25T10:51:46Z

⌛ Trying commit a331842812fd8111853827ac5fc158b18de4b021 with merge 7f7032a99cafd0b1752b8d0f1ffca8ebfb14e9ab...

src/librustdoc/core.rs

rustbot · 2023-01-25T11:05:21Z

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

tests/rustdoc/issue-96381.rs

jyn514 · 2023-01-25T11:18:59Z

compiler/rustc_hir_analysis/src/check/check.rs

-    // Since rustdoc doesn't care about the concrete type behind `impl Trait`, just don't look at it!
-    // See https://github.com/rust-lang/rust/issues/75100
-    if tcx.sess.opts.actually_rustdoc {
-        return;


Are there tests for this code path? If not, can you please double check that documenting async-std still works after this change? Make sure to enable all the features enabled on doc's.rs (you can use doc-like-docs.rs in @Nemo157 's dotfiles).

Yes, there are tests. I'll run the try build on async-std once it finishes. Seems like rustup toolchain link on local builds doesn't work for rustdoc as cargo +linked_thingy doc can't find libcore and libstd

That should work - try x build rustdoc library, or just x build without arguments.

hmm weird, not sure what I was doing wrong before.

It passes fine now with --all-features

src/librustdoc/lib.rs

oli-obk · 2023-01-25T11:21:58Z

grml... x test rustdoc does not run all other rustdoc tests... do I need to specify them one by one or is there some placeholder syntax?

jyn514 · 2023-01-25T11:26:33Z

src/librustdoc/core.rs

@@ -317,6 +318,12 @@ pub(crate) fn create_config(
                }
                (rustc_interface::DEFAULT_QUERY_PROVIDERS.type_of)(tcx, def_id)
            };
+            providers.codegen_fn_attrs = |_, did| {
+                assert!(did.is_local());
+                CodegenFnAttrs::new()


Is this right? Won't it end up in us failing to include the target features in the generated docs?

cc @Amanieu , I'm not sure if this has tests.

compiler/rustc_interface/src/passes.rs

tests/rustdoc-ui/check-cfg.stderr

jyn514 · 2023-01-25T11:31:48Z

I am not sure this is the right approach. I worry that we are allowing code to be even more broken than before, in ways that are not tested or guaranteed, but people will still depend on.

oli-obk · 2023-01-25T11:34:29Z

I am not sure this is the right approach. I worry that we are allowing code to be even more broken than before, in ways that are not tested or guaranteed, but people will still depend on.

Either rustdoc runs all checks, or rustdoc expects the crate to already pass rustc. This PR just takes the second variant further to its extreme.

jyn514 · 2023-01-25T11:39:30Z

Yes, that in itself seems ok to me - my worry is that the broken code we newly accept isn't tested and we'll only know if it regresses once someone complains. See #75100 for an example.

oli-obk · 2023-01-25T11:42:31Z

my worry is that the broken code we newly accept isn't tested and we'll only know if it regresses once someone complains

Oh yea, that's definitely a problem. Do you expect this to be worse due to this PR? If the fix is adding an actually_rustdoc check to avoid a delay_span_bug, then we haven't lost anything with this PR and are just getting fewer issues opened. If the fixes are commonly changing something in rustdoc, then yes, we'll lose some of that signal and will just accept the code and produce possibly broken documentation.

jyn514 · 2023-01-25T11:45:19Z

compiler/rustc_errors/src/lib.rs

+    /// of the same function in scope at the same time, which isn't legal Rust otherwise. See
+    /// https://doc.rust-lang.org/beta/rustdoc/advanced-features.html#interactions-between-platform-specific-docs
+    /// for details
+    pub fn reset_delayed_bugs(&self) {


Suggested change

pub fn reset_delayed_bugs(&self) {

pub fn reset_delayed_bugs(&self) {

assert!(self.opts.actually_rustdoc);

😆 I want to remove that flag at some point

jyn514 · 2023-01-25T11:48:41Z

Do you expect this to be worse due to this PR?

Yes - before checks had to explicitly special case rustdoc in order to be ignored, now all checks are blanket ignored. So we are accepting strictly more code than before, and in ways that are not predictable and therefore hard to test.

I suspect #79461 (and the other linked issues) will compile successfully instead of ICEing after this change.

oli-obk · 2023-01-25T11:58:02Z

I suspect #79461 (and the other linked issues) will compile successfully instead of ICEing after this change.

And from looking at what they do, I suspect the fix I would consider least invasive is to add an actually_rustdoc check. Yes we lose signal, but we also get lots fewer ICE reports I hope :)

jyn514 · 2023-01-25T12:03:54Z

I am confused. I thought the goal of this PR was to get rid of actually_rustdoc checks, not to accept more invalid code (I don't particularly care about those linked ICEs for the reasons stated in #79496 (comment)).

oli-obk · 2023-01-25T12:53:00Z

@rust-timer build 7f7032a99cafd0b1752b8d0f1ffca8ebfb14e9ab

rust-timer · 2023-01-25T14:20:13Z

Finished benchmarking commit (7f7032a99cafd0b1752b8d0f1ffca8ebfb14e9ab): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.5%	[-19.0%, -0.3%]	17
Improvements ✅ (secondary)	-11.0%	[-38.4%, -0.5%]	9
All ❌✅ (primary)	-4.5%	[-19.0%, -0.3%]	17

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.9%	[1.0%, 3.1%]	4
Improvements ✅ (primary)	-2.9%	[-6.8%, -1.4%]	10
Improvements ✅ (secondary)	-6.6%	[-14.2%, -1.5%]	5
All ❌✅ (primary)	-2.9%	[-6.8%, -1.4%]	10

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.7%	[-17.5%, -1.9%]	13
Improvements ✅ (secondary)	-12.6%	[-41.3%, -1.7%]	8
All ❌✅ (primary)	-6.0%	[-17.5%, 2.3%]	14

oli-obk · 2023-01-25T14:22:25Z

src/librustdoc/core.rs

-    tcx.sess.time("item_types_checking", || {
-        tcx.hir().for_each_module(|module| tcx.ensure().check_mod_item_types(module))
-    });


The performance improvement is almost entirely due to this change. We stop checking item signatures.

GuillaumeGomez · 2023-01-26T16:29:43Z

Looks great, thanks!

bjorn3 · 2023-01-26T17:20:20Z

compiler/rustc_errors/src/lib.rs

+    /// of the same function in scope at the same time, which isn't legal Rust otherwise. See
+    /// <https://doc.rust-lang.org/beta/rustdoc/advanced-features.html#interactions-between-platform-specific-docs>
+    /// for details
+    pub fn reset_delayed_bugs(&self) {


Would it be possible to still keep a record of the fact that there was a delayed bug and if so ICE when using sess.compile_error() but ignore this flag when using a new sess.compile_error_allow_reset_delay_bugs() to be used in rustdoc? Or maybe even have that later method be the method that is actually responsible for ignoring delayed bugs?

bjorn3 · 2023-01-26T17:23:25Z

tests/rustdoc-ui/issue-79494.rs


 #![feature(const_transmute)]

-const ZST: &[u8] = unsafe { std::mem::transmute(1usize) }; //~ ERROR cannot transmute between types of different sizes, or dependently-sized types
+const ZST: &[u8] = unsafe { std::mem::transmute(1usize) };


Maybe make this public to force it to be evaluated? Otherwise #79494 could regress for documented items.

bjorn3 · 2023-01-26T17:24:04Z

tests/rustdoc-ui/issue-96381.rs

 #![allow(unused)]

+// check-pass
+
 trait Foo<T>: Sized {


Maybe this needs to be public too?

bjorn3 · 2023-01-26T17:25:28Z

tests/rustdoc-ui/track-diagnostics.rs

-// Normalize the emitted location so this doesn't need
-// updating everytime someone adds or removes a line.
-// normalize-stderr-test ".rs:\d+:\d+" -> ".rs:LL:CC"
+// check-pass


This no longer tests -Ztrack-diagnostics.

bors · 2023-01-31T18:10:28Z

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

apiraino · 2023-02-02T11:47:27Z

Discussed during T-compiler meeting (Zulip notes). Will need another thought in a separate conversation.

@rustbot label -I-compiler-nominated

compiler-errors · 2023-03-01T05:09:29Z

Gonna mark this as blocked then 🤔 lemme know if I misinterpreted the current status

@rustbot blocked

oli-obk · 2023-05-11T16:25:52Z

That makes sense to me and seems like a reasonable direction for rustdoc to go, but I think we should get sign off from the rustdoc team they actually want to do that.

As a summary, this ignores delay_span_bug in rustdoc, which has the follow-on effects that:
* More invalid code is accepted

* We can remove `actually_rustdoc` checks from many parts of the compiler

* There is a higher likelihood that bugs in rustdoc or the compiler itself will not be found or reported (and conversely, fewer bug reports about ICEs in rustdoc that we don't care about).

* It is no longer possible to test the majority of the broken code that is accepted; we do not have an exhaustive list of checks that are being ignored. This makes it hard to guarantee we don't regress which broken code is accepted.

do you want to kick off a rustdoc FCP?

jyn514 · 2023-05-11T18:34:38Z

I am no longer on the rustdoc team.

GuillaumeGomez · 2023-05-11T19:11:13Z

I'm a bit shared on this: it does improve performance but on the other hand, some code that was supposed to fail isn't failing anymore. Really not sure what to think about it...

oli-obk · 2023-05-12T07:46:41Z

it's more of a general question, not specifically about this PR:

Is rustdoc supposed to work with broken Rust code, because you should only run rustdoc on code that works with rustc?

yes: then let's remove more checks from rustdoc
no: let's fix rustdoc to make it stop compiling broken Rust code

I don't think the current middle ground is a good place to be in, and I'd rather find a way to get to one of the extremes. Or is there another option that I'm not thinking about?

jyn514 · 2023-05-12T12:59:58Z

@oli-obk not so much a third option, but lots of delay_span_bug calls are for errors in the caller, not in the code. rustdoc already misuses the API and i think that will get worse if we silence delayed bugs.

oli-obk · 2023-05-12T14:34:09Z

That's true, but if the alternative is to wrap all delay_span_bugs or bug!s in if !actually_rustdoc, then we have gained nothing either.

jyn514 · 2023-05-12T17:15:05Z

I don't think that's what would happen. There are many many delay_span_bug calls and only a small fraction special case rustdoc, usually around function bodies and impl trait. This would turn off the checking for the trait system as well, which is very likely to cause real bugs in collect_blanket_impls.

oli-obk · 2023-05-12T20:00:24Z

I agree, but that's because I'm team "only allow legal Rust as Rustdoc input.". We can't keep Rustdoc from ICEing otherwise.

So in my opinion that's the options we have: make Rustdoc even weirder by hiding as many failures as we can, or fix it to stop allowing broken code.

I would like to move away from the current situation in either direction.

GuillaumeGomez · 2023-06-14T11:16:27Z

Sorry, forgot about this. Let's see what the @rust-lang/rustdoc team thinks about this.

notriddle · 2023-06-14T14:59:07Z

This would turn off the checking for the trait system as well

That doesn’t seem okay.

Is rustdoc supposed to work with broken Rust code, because you should only run rustdoc on code that works with rustc?

yes: then let's remove more checks from rustdoc

no: let's fix rustdoc to make it stop compiling broken Rust code

The way it’s supposed to work is that rustdoc does type checking on items — but not on function bodies — because the function’s type signature completely describes its type. That’s what the book says, so that’s what rustdoc promised, but it can’t be true because the function body can change the auto traits of an async generator or RPIT.

This would turn off the checking for the trait system as well, which is very likely to cause real bugs in collect_blanket_impls.

The spot where I’m really worried is running collect_auto_traits on a TAIT. It can’t be done without type checking function bodies, and not doing it (even transitively, if a struct contains a TAIT) would be annoying for anybody who isn’t writing crates with platform-specific code using the cfg(doc) hack. Not running auto trait checks on TAIT seems like bailing on a feature at the point where it would be most useful.

In short, I’m on team stop-accepting-broken-code too, because it seems like the language is changing in a direction that makes the original assumption around skipping function bodies invalid.

jyn514 · 2023-07-17T16:37:44Z

In short, I’m on team stop-accepting-broken-code too, because it seems like the language is changing in a direction that makes the original assumption around skipping function bodies invalid.

started a discussion at https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/stop.20accepting.20broken.20code about what it would take to start doing this.

for now i'm going to close this pr since it seems like the team doesn't want to move in the direction of accepting more broken code.

Remove a dummy use of LazyLock

393564b

rustbot assigned compiler-errors Jan 25, 2023

This comment has been minimized.

Sign in to view

Noratrieb reviewed Jan 25, 2023

View reviewed changes

src/librustdoc/core.rs Outdated Show resolved Hide resolved