Begin fixing all the broken doctests in `compiler/` #96094

Elliot-Roberts · 2022-04-15T23:11:49Z

Begins to fix #95994.
All of them pass now but 24 of them I've marked with ignore HELP (<explanation>) (asking for help) as I'm unsure how to get them to work / if we should leave them as they are.
There are also a few that I marked ignore that could maybe be made to work but seem less important.
Each ignore has a rough "reason" for ignoring after it parentheses, with

(pseudo-rust) meaning "mostly rust-like but contains foreign syntax"
(illustrative) a somewhat catchall for either a fragment of rust that doesn't stand on its own (like a lone type), or abbreviated rust with ellipses and undeclared types that would get too cluttered if made compile-worthy.
(not-rust) stuff that isn't rust but benefits from the syntax highlighting, like MIR.
(internal) uses rustc_* code which would be difficult to make work with the testing setup.

Those reason notes are a bit inconsistently applied and messy though. If that's important I can go through them again and try a more principled approach. When I run rg '```ignore \(' . on the repo, there look to be lots of different conventions other people have used for this sort of thing. I could try unifying them all if that would be helpful.

I'm not sure if there was a better existing way to do this but I wrote my own script to help me run all the doctests and wade through the output. If that would be useful to anyone else, I put it here: https://github.com/Elliot-Roberts/rust_doctest_fixing_tool

rust-highfive · 2022-04-15T23:11:51Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive · 2022-04-15T23:11:52Z

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.

Elliot-Roberts · 2022-04-15T23:13:27Z

r? @jyn514

Elliot-Roberts · 2022-04-15T23:20:00Z

Here are some of the ignores that I thought maybe could be fixed (the line numbers might be slightly off):

compiler/rustc_middle/src/ty/util.rs:970
compiler/rustc_middle/src/ty/walk.rs:37  
compiler/rustc_span/src/hygiene.rs:801  
compiler/rustc_typeck/src/collect/type_of.rs:533        
compiler/rustc_typeck/src/outlives/implicit_infer.rs:239

jyn514

Those reason notes are a bit inconsistently applied and messy though. If that's important I can go through them again and try a more principled approach. When I run rg '```ignore (' . on the repo, there look to be lots of different conventions other people have used for this sort of thing. I could try unifying them all if that would be helpful.

I don't think it's too important to clean these up, the categories you have seem fine 👍 no need to change existing tests.

I left suggestions for most of your HELP comments :)

(internal) uses rustc_* code which would be difficult to make work with the testing setup.

In retrospect, I think I was wrong on the issue and this should work out of the box as long as it only uses crates that have previously been compiled (see also #95445). Maybe try running these and see if they work?

jyn514 · 2022-04-16T05:50:42Z

compiler/rustc_ast_lowering/src/lib.rs

@@ -229,7 +229,7 @@ enum ImplTraitContext<'b, 'a> {
        /// them. This includes lifetimes bound since we entered this context.
        /// For example:
        ///
-        /// ```
+        /// ```ignore HELP (how would you make a stub trait for this?)
        /// type A<'b> = impl for<'a> Trait<'a, Out = impl Sized + 'a>;


hmm, good question - I got as far as

/// #![feature(type_alias_impl_trait)] /// trait Trait<'a, Out> {} /// impl<'a, T: Sized> Trait<'a, T> for i32 {} /// type A<'b> = impl for<'a> Trait<'a, impl Sized + 'a>; /// fn foo() -> A<'static> { 0_i32 }

and then wasn't sure how to fix

error[E0666]: nested `impl Trait` is not allowed --> src/lib.rs:236:37 | 6 | type A<'b> = impl for<'a> Trait<'a, impl Sized + 'a>; | -----------------------^^^^^^^^^^^^^^^- | | | | | nested `impl Trait` here | outer `impl Trait` error: non-defining opaque type use in defining scope --> src/lib.rs:237:26 | 6 | type A<'b> = impl for<'a> Trait<'a, impl Sized + 'a>; | -- cannot use static lifetime; use a bound lifetime instead or remove the lifetime parameter from the opaque type 7 | fn foo() -> A<'static> { 0_i32 } | ^^^^^ error: unconstrained opaque type --> src/lib.rs:236:37 | 6 | type A<'b> = impl for<'a> Trait<'a, impl Sized + 'a>; | ^^^^^^^^^^^^^^^ | = note: `` must be used in combination with a concrete type within the same module

while keeping the original semantics.

Should we ignore this one as just an "illustration" then? The original single line of code gets quite buried when we add all that. If we figure out how to make it compile we could just hide the extra lines, but it would still be harder to read the un-rendered source (is that at all a priority for us? I've been considering it moderately important as my vscode doesn't have built-in doc rendering unless I hover on an item which is tedious and doesn't work for everything, but if we expect most future readers to have rendering then I can see why we wouldn't bother with it.)

I think it's semi important to figure out what the comment is describing in the first place - it's unclear to me now even having read it several times and played around with the example, so it will almost certainly be unclear to whoever reads it next without that context. But I agree making it compile is probably not worth it.

It's unclear to me, too. Igot all the way to

#![feature(type_alias_impl_trait)] trait Trait<'a> { type Out: 'a;} impl<'a> Trait<'a> for i32 { type Out = String;} type A = impl for<'a> Trait<'a, Out = impl Sized + 'a>; fn foo() -> A { 0_i32 }

which is just a bug that it ain't accepted.i'll openable issuefor it And try to figure out the details of what's going on with this field at the same time.

compiler/rustc_builtin_macros/src/deriving/generic/mod.rs

jyn514 · 2022-04-16T05:55:51Z

compiler/rustc_data_structures/src/tagged_ptr.rs

@@ -45,7 +45,8 @@ pub unsafe trait Pointer: Deref {
    /// case you'll need to manually figure out what the right type to pass to
    /// align_of is.
    ///
-    /// ```rust
+    /// ```ignore HELP (should we try to make this runnable? and why does this import not seem to work?)
+    /// # use std::ops::Deref;


The import works fine for me (are you using --stage 0? If so you need to cherry-pick #95993).

and this is a good question - I think using Self instead of a concrete type here was intentional. Not sure the right approach.

I'll try that again. I might have somehow been looking at stale error messages (oops sorry).
I used --stage 1 for the entire thing. Which are the stages we would want to run in CI?

CI always uses --stage 2. At some point I want to test that other stages work, but those will be tests of bootstrap itself, not that the doctests are correct.

compiler/rustc_expand/src/mbe/macro_parser.rs

compiler/rustc_incremental/src/assert_dep_graph.rs

jyn514 · 2022-04-16T06:28:34Z

compiler/rustc_typeck/src/check/mod.rs

-///
-///     f(1, 2);
-///
+/// ```ignore HELP (is there a way to set TupleArgumentsFlag so that this compiles?)


I think this is used for Fn* traits. Not sure how to replicate in user code.

jyn514 · 2022-04-16T06:30:57Z

compiler/rustc_typeck/src/check/regionck.rs

@@ -269,7 +269,7 @@ impl<'a, 'tcx> RegionCtxt<'a, 'tcx> {
    ///
    /// Consider this silly example:
    ///
-    /// ```
+    /// ```ignore HELP (is that @ intentional?)


@ is old syntax for "garbage-collected pointer": https://pcwalton.github.io/2013/06/02/removing-garbage-collection-from-the-rust-language.html. Not sure how to keep the same semantics - maybe Box<i32> is close?

compiler/rustc_typeck/src/check/upvar.rs

Elliot-Roberts · 2022-04-16T09:25:26Z

Thanks for all your help so far, @jyn514! I'm learning lots.
I had hoped to fix more myself and leave less of the problem solving to you but with the iteration times so slow it probably would have taken me a while longer. (Some of them I should have just googled/rtfm for a little more though; sorry about that).

I'll have the straightforward fixes done soon and then we'll just have the handful that need expert input :)

jyn514 · 2022-04-16T09:29:19Z

Thank you for tackling this!

with the iteration times so slow it probably would have taken me a while longer

I highly recommend cherry-picking my fix for --stage 0 so the iteration time is faster. It was taking me about ~10 seconds to check each suggestion I made, I wouldn't have gone through them all one by one if it took me 10 minutes each time.

Elliot-Roberts · 2022-04-16T09:40:32Z

Oh ok wow haha. I luckily only had to recompile everything <10 times by working in batches but that sounds like a way better approach.

Oh and for the ones I marked ignore (internal), I of course only marked them that because they fail and adding the obvious use rustc_...; suggested by the compiler didn't immediately fix the problem. I can go look at them again now. There are 29 of them with 19 identical seeming ones just in compiler/rustc_data_structures/src/owning_ref/mod.rs.

Elliot-Roberts · 2022-04-17T06:36:54Z

Alright, I fixed the HELPs I could and also most of the ignore (internal)s.
Now left to address are:

one ignore LATER I left in by accident with scratch code. I think that one might need to be an ignore (illustrative) because the objects we need to construct to make the example work look big. Are there easier ways than giant struct literals?
one ignore HELP of a new problem (and continuation of the same problem in ignore CONT). Also for that one, I added a dev-dependency which might be undesirable?
seven ignore UNSOLVEDs for previous problems we don't have solutions for yet.

compiler/rustc_builtin_macros/src/deriving/encodable.rs

compiler/rustc_parse/src/parser/item.rs

jyn514 · 2022-04-19T01:34:19Z

one ignore LATER I left in by accident with scratch code. I think that one might need to be an ignore (illustrative) because the objects we need to construct to make the example work look big. Are there easier ways than giant struct literals?

I think it's useful to have at least one example of what TestDescAndFn looks like. No need to have 3 separate struct literals. We should first confirm that's what the compiler actually expands #[test] function to, though.

bors · 2022-04-28T02:10:46Z

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

Elliot-Roberts · 2022-05-01T10:03:27Z

I think I've done most of what I know how to do and incorporated all the feedback given so far (thanks for the help everyone!).
Now there are just 7 ignore UNSOLVEDs left to address. (ones we haven't decided how to fix yet). Each has a small explanation/question in parentheses by it.
I might try to investigate one that's a suspected bug, but at my current level of experience that could take a while haha.
If anyone has time to advise me in working on one, that would be great too. I'm not quite sure where to start.

jyn514 · 2022-05-01T15:51:49Z

I think merging as-is is fine too, no need to do everything at once. We can keep the tracking issue open until we figure out how to get rid of the ignore.

jyn514 · 2022-05-01T15:52:15Z

@compiler-errors I think this is ready for review - I don't have bors permissions.

klensy · 2022-05-01T20:41:15Z

Maybe squash? Some commits add\remove the same things.

Elliot-Roberts · 2022-05-02T04:01:53Z

Right, thanks for the reminder.
On a separate note, should the ignore UNSOLVEDs stay as they are with the comments in parentheses? I had originally written them to be very temporary things.

compiler-errors · 2022-05-05T20:09:28Z

I agree with @jyn514 that this is fine to merge as-is.

@bors r+ rollup=iffy

bors · 2022-05-05T20:09:30Z

📌 Commit 7907385 has been approved by compiler-errors

…iler-errors Begin fixing all the broken doctests in `compiler/` Begins to fix rust-lang#95994. All of them pass now but 24 of them I've marked with `ignore HELP (<explanation>)` (asking for help) as I'm unsure how to get them to work / if we should leave them as they are. There are also a few that I marked `ignore` that could maybe be made to work but seem less important. Each `ignore` has a rough "reason" for ignoring after it parentheses, with - `(pseudo-rust)` meaning "mostly rust-like but contains foreign syntax" - `(illustrative)` a somewhat catchall for either a fragment of rust that doesn't stand on its own (like a lone type), or abbreviated rust with ellipses and undeclared types that would get too cluttered if made compile-worthy. - `(not-rust)` stuff that isn't rust but benefits from the syntax highlighting, like MIR. - `(internal)` uses `rustc_*` code which would be difficult to make work with the testing setup. Those reason notes are a bit inconsistently applied and messy though. If that's important I can go through them again and try a more principled approach. When I run `rg '```ignore \(' .` on the repo, there look to be lots of different conventions other people have used for this sort of thing. I could try unifying them all if that would be helpful. I'm not sure if there was a better existing way to do this but I wrote my own script to help me run all the doctests and wade through the output. If that would be useful to anyone else, I put it here: https://github.com/Elliot-Roberts/rust_doctest_fixing_tool

compiler-errors · 2022-05-06T02:32:02Z

Apparently ignore blocks need to parse as valid Rust code, which is not the case for some of the examples here?

Can you fix these @Elliot-Roberts? Thanks! See: https://github.com/rust-lang-ci/rust/runs/6316344673?check_suite_focus=true

@bors r-
@bors rollup=never

Elliot-Roberts · 2022-05-06T05:34:44Z

Oh ok. I think I'll have to change a few older ignores too. Like this one from 5 years ago:

rust/compiler/rustc_ast/src/ast.rs

Lines 1479 to 1487 in 74cea9f

    
           /// ```ignore (only-for-syntax-highlight) 
        
           /// <Vec<T> as a::b::Trait>::AssociatedItem 
        
           ///  ^~~~~     ~~~~~~~~~~~~~~^ 
        
           ///  ty        position = 3 
        
           /// 
        
           /// <Vec<T>>::AssociatedItem 
        
           ///  ^~~~~    ^ 
        
           ///  ty       position = 0 
        
           /// ```

Elliot-Roberts · 2022-05-06T06:14:56Z

Wait never mind, rustdoc doesn't mind that one.
In the instance identified by CI, it appears the only problem was the un-closed curly brace??

So, given that the error message says

ignore code blocks require valid Rust code for syntax highlighting; mark blocks that do not contain Rust code as text

should I still mark everything that isn't valid rust as text, or should I keep the ones that rustdoc is OK with?

Elliot-Roberts · 2022-05-06T07:26:25Z

Looks like there were just a couple unmatched braces in one file that were causing rustdoc to fail to parse for syntax highlighting 😅.

Elliot-Roberts · 2022-05-06T07:47:42Z

I suppose a number of them would be better as text though. I didn't realize rustdoc shows a warning saying the code isn't tested when you use ignore. That paired with rustdoc's syntax highlighting not actually doing much makes it not very worthwhile.
Here is, for example, some highlighting in my vscode:

versus in rustdoc:

I thought the highlighting was preferable when I saw it in vscode, but in rustdoc it seems worse than ```text. Potentially even misleading?

I'd appreciate others' opinions.

compiler-errors · 2022-05-06T17:39:17Z

should I still mark everything that isn't valid rust as text, or should I keep the ones that rustdoc is OK with?

@Elliot-Roberts: I think you should mark it as text, syntax highlighting may get messed up by stray tokens even if rustdoc doesn't get angry when parsing it, and yeah, that "this example is not tested" string is a bit misleading.

... though I have been less involved in this thread compared to e.g. @jyn514 who may have more of a opinion.

jyn514 · 2022-05-06T19:05:40Z

We should use text unless we're actively looking for syntax highlighting, yeah.

See also #96573 which tries to make this a little less confusing.

Elliot-Roberts · 2022-05-06T21:48:28Z

ok hm. reading #96573 and learning that it's expected to be possible to run doctests marked with ignore changes things a bit. So everying that can't compile should be text? including fragments that only fail for undeclared stuff?

I'll start by just using text for everything that wouldn't Parse. I expect there will be some grey area for even that, though.

jyn514 · 2022-05-06T22:04:23Z

ok hm. reading #96573 and learning that it's expected to be possible to run doctests marked with ignore changes things a bit. So everying that can't compile should be text? including fragments that only fail for undeclared stuff?

Don't spend too much more time on this. If it passes CI and it looks vaguely right in the generated docs, it's fine :) no need to rewrite half your work.

compiler-errors · 2022-05-06T22:06:17Z

+1, don't waste too much time on it -- I can re-approve it as soon as you can locally confirm that the tests don't fail like they do in https://github.com/rust-lang-ci/rust/runs/6316344673?check_suite_focus=true

Elliot-Roberts · 2022-05-06T22:57:46Z

Ok thanks for the reassurance :)

@compiler-errors How do I test that locally? I have at least verified that rusdoc succeeds for everything in compiler/ (647d0b6 was all that was needed)

And I can still go make more of them text as now it's bothering me. I have free time today to work on it.

compiler-errors · 2022-05-07T03:04:21Z

Frankly.... I don't actually know. Anywho, let's try to merge this now.

And I can still go make more of them text as now it's bothering me

Feel free to open a new PR and r? @compiler-errors if you want me to review more follow-up changes.

compiler-errors · 2022-05-07T03:04:26Z

@bors r+

bors · 2022-05-07T03:04:28Z

📌 Commit 647d0b6 has been approved by compiler-errors

bors · 2022-05-07T06:30:33Z

⌛ Testing commit 647d0b6 with merge 574830f...

bors · 2022-05-07T09:11:06Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 574830f to master...

rust-timer · 2022-05-07T10:31:31Z

Finished benchmarking commit (574830f): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

rust-highfive assigned compiler-errors Apr 15, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 15, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 15, 2022

jyn514 reviewed Apr 16, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

Elliot-Roberts force-pushed the fix_doctests branch from 89e1574 to cd7734e Compare April 17, 2022 05:49

oli-obk mentioned this pull request Apr 17, 2022

higher ranked lifetimes not recognized by type alias impl trait #96146

Open

bjorn3 reviewed Apr 17, 2022

View reviewed changes

compiler/rustc_builtin_macros/src/deriving/encodable.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Apr 17, 2022

View reviewed changes

compiler/rustc_parse/src/parser/item.rs Outdated Show resolved Hide resolved

Elliot-Roberts force-pushed the fix_doctests branch from cd7734e to b213ce6 Compare May 1, 2022 03:41

Elliot-Roberts marked this pull request as ready for review May 1, 2022 20:28

fix most compiler/ doctests

7907385

Elliot-Roberts force-pushed the fix_doctests branch from e5a054e to 7907385 Compare May 3, 2022 00:42

compiler-errors mentioned this pull request May 5, 2022

Rollup of 8 pull requests #96749

Closed

compiler-errors mentioned this pull request May 6, 2022

Rollup of 8 pull requests #96753

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 6, 2022

fix unmatched braces

647d0b6

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 7, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label May 7, 2022

bors merged commit 574830f into rust-lang:master May 7, 2022

rustbot added this to the 1.62.0 milestone May 7, 2022

Begin fixing all the broken doctests in compiler/ #96094

Begin fixing all the broken doctests in compiler/ #96094

Conversation

Elliot-Roberts commented Apr 15, 2022

rust-highfive commented Apr 15, 2022

rust-highfive commented Apr 15, 2022

Elliot-Roberts commented Apr 15, 2022

Elliot-Roberts commented Apr 15, 2022

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Elliot-Roberts commented Apr 16, 2022

jyn514 commented Apr 16, 2022

Elliot-Roberts commented Apr 16, 2022

This comment has been minimized.

Elliot-Roberts commented Apr 17, 2022

jyn514 commented Apr 19, 2022

bors commented Apr 28, 2022

Elliot-Roberts commented May 1, 2022

jyn514 commented May 1, 2022

jyn514 commented May 1, 2022

klensy commented May 1, 2022

Elliot-Roberts commented May 2, 2022

compiler-errors commented May 5, 2022

bors commented May 5, 2022

compiler-errors commented May 6, 2022 • edited Loading

Elliot-Roberts commented May 6, 2022

Elliot-Roberts commented May 6, 2022

Elliot-Roberts commented May 6, 2022

Elliot-Roberts commented May 6, 2022

compiler-errors commented May 6, 2022

jyn514 commented May 6, 2022

Elliot-Roberts commented May 6, 2022

jyn514 commented May 6, 2022

compiler-errors commented May 6, 2022

Elliot-Roberts commented May 6, 2022

compiler-errors commented May 7, 2022

compiler-errors commented May 7, 2022

bors commented May 7, 2022

bors commented May 7, 2022

bors commented May 7, 2022

rust-timer commented May 7, 2022

Begin fixing all the broken doctests in `compiler/` #96094

Begin fixing all the broken doctests in `compiler/` #96094

compiler-errors commented May 6, 2022 •

edited

Loading