Invalid lowering of macro in closure #71820

marmeladema · 2020-05-03T01:26:02Z

When trying to fix #71104 I ended up fighting with some rustdoc test about declarative macro: https://github.com/rust-lang/rust/blob/master/src/test/rustdoc/macro-in-closure.rs

The problem is that the macro is properly lowered to hir because its exported but the parent item is not:

Macro has def id DefId(0:5 ~ macro_in_closure[317d]::main[0]::{{closure}}[0]::m[0])
Closure has def id DefId(0:4 ~ macro_in_closure[317d]::main[0]::{{closure}}[0])

The latter appears nowhere in the hir tree and when https://github.com/rust-lang/rust/blob/master/src/librustc_privacy/lib.rs#L945 tries to access its parent, it returns a DefId that has no associated HirId.

I don't know exactly how lowering of closure works, neither the details of declarative macro visibility, but i think the bug is either:

the closure should have been lowered somehow
the declarative macro should not have been lowered as it is probably not visible outside of the closure

Here is a hir dump of the test case:

warning: unused macro definition
 --> src/test/rustdoc/macro-in-closure.rs:7:9
  |
7 |         macro m() {}
  |         ^^^^^^^^^^^^
  |
  = note: `#[warn(unused_macros)]` on by default

Crate {
    item: CrateItem {
        module: Mod {
            inner: src/test/rustdoc/macro-in-closure.rs:3:1: 9:2,
            item_ids: [
                ItemId {
                    id: HirId {
                        owner: DefId(0:1 ~ macro_in_closure[317d]::{{misc}}[0]),
                        local_id: 0,
                    },
                },
                ItemId {
                    id: HirId {
                        owner: DefId(0:2 ~ macro_in_closure[317d]::std[0]),
                        local_id: 0,
                    },
                },
                ItemId {
                    id: HirId {
                        owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
                        local_id: 0,
                    },
                },
            ],
        },
        attrs: [
            Attribute {
                kind: Normal(
                    AttrItem {
                        path: Path {
                            span: src/test/rustdoc/macro-in-closure.rs:3:4: 3:11,
                            segments: [
                                PathSegment {
                                    ident: feature#0,
                                    id: NodeId(2),
                                    args: None,
                                },
                            ],
                        },
                        args: Delimited(
                            DelimSpan {
                                open: src/test/rustdoc/macro-in-closure.rs:3:11: 3:12,
                                close: src/test/rustdoc/macro-in-closure.rs:3:22: 3:23,
                            },
                            Parenthesis,
                            TokenStream(
                                [
                                    (
                                        Token(
                                            Token {
                                                kind: Ident(
                                                    "decl_macro",
                                                    false,
                                                ),
                                                span: src/test/rustdoc/macro-in-closure.rs:3:12: 3:22,
                                            },
                                        ),
                                        NonJoint,
                                    ),
                                ],
                            ),
                        ),
                    },
                ),
                id: AttrId(0),
                style: Inner,
                span: src/test/rustdoc/macro-in-closure.rs:3:1: 3:24,
            },
        ],
        span: src/test/rustdoc/macro-in-closure.rs:3:1: 9:2,
    },
    exported_macros: [
        MacroDef {
            ident: m#0,
            vis: Spanned {
                node: Inherited,
                span: src/test/rustdoc/macro-in-closure.rs:7:9: 7:9,
            },
            attrs: [],
            hir_id: HirId {
                owner: DefId(0:5 ~ macro_in_closure[317d]::main[0]::{{closure}}[0]::m[0]),
                local_id: 0,
            },
            span: src/test/rustdoc/macro-in-closure.rs:7:9: 7:21,
            ast: MacroDef {
                body: Delimited(
                    DelimSpan {
                        open: src/test/rustdoc/macro-in-closure.rs:7:16: 7:16,
                        close: src/test/rustdoc/macro-in-closure.rs:7:21: 7:21,
                    },
                    Brace,
                    TokenStream(
                        [
                            (
                                Delimited(
                                    DelimSpan {
                                        open: src/test/rustdoc/macro-in-closure.rs:7:16: 7:17,
                                        close: src/test/rustdoc/macro-in-closure.rs:7:17: 7:18,
                                    },
                                    Paren,
                                    TokenStream(
                                        [],
                                    ),
                                ),
                                NonJoint,
                            ),
                            (
                                Token(
                                    Token {
                                        kind: FatArrow,
                                        span: src/test/rustdoc/macro-in-closure.rs:7:18: 7:19,
                                    },
                                ),
                                NonJoint,
                            ),
                            (
                                Delimited(
                                    DelimSpan {
                                        open: src/test/rustdoc/macro-in-closure.rs:7:19: 7:20,
                                        close: src/test/rustdoc/macro-in-closure.rs:7:20: 7:21,
                                    },
                                    Brace,
                                    TokenStream(
                                        [],
                                    ),
                                ),
                                NonJoint,
                            ),
                        ],
                    ),
                ),
                macro_rules: false,
            },
        },
    ],
    non_exported_macro_attrs: [],
    items: {
        HirId {
            owner: DefId(0:1 ~ macro_in_closure[317d]::{{misc}}[0]),
            local_id: 0,
        }: Item {
            ident: #0,
            hir_id: HirId {
                owner: DefId(0:1 ~ macro_in_closure[317d]::{{misc}}[0]),
                local_id: 0,
            },
            attrs: [
                Attribute {
                    kind: Normal(
                        AttrItem {
                            path: Path {
                                span: src/test/rustdoc/macro-in-closure.rs:1:1: 1:1,
                                segments: [
                                    PathSegment {
                                        ident: prelude_import#1,
                                        id: NodeId(4),
                                        args: None,
                                    },
                                ],
                            },
                            args: Empty,
                        },
                    ),
                    id: AttrId(2),
                    style: Outer,
                    span: src/test/rustdoc/macro-in-closure.rs:1:1: 1:1,
                },
            ],
            kind: Use(
                Path {
                    span: src/test/rustdoc/macro-in-closure.rs:1:1: 1:1,
                    res: Err,
                    segments: [
                        PathSegment {
                            ident: {{root}}#1,
                            hir_id: Some(
                                HirId {
                                    owner: DefId(0:1 ~ macro_in_closure[317d]::{{misc}}[0]),
                                    local_id: 1,
                                },
                            ),
                            res: Some(
                                Err,
                            ),
                            args: None,
                            infer_args: false,
                        },
                        PathSegment {
                            ident: std#1,
                            hir_id: Some(
                                HirId {
                                    owner: DefId(0:1 ~ macro_in_closure[317d]::{{misc}}[0]),
                                    local_id: 2,
                                },
                            ),
                            res: Some(
                                Def(
                                    Mod,
                                    DefId(1:0 ~ std[512e]),
                                ),
                            ),
                            args: None,
                            infer_args: false,
                        },
                        PathSegment {
                            ident: prelude#1,
                            hir_id: Some(
                                HirId {
                                    owner: DefId(0:1 ~ macro_in_closure[317d]::{{misc}}[0]),
                                    local_id: 3,
                                },
                            ),
                            res: Some(
                                Def(
                                    Mod,
                                    DefId(1:15 ~ std[512e]::prelude[0]),
                                ),
                            ),
                            args: None,
                            infer_args: false,
                        },
                        PathSegment {
                            ident: v1#1,
                            hir_id: Some(
                                HirId {
                                    owner: DefId(0:1 ~ macro_in_closure[317d]::{{misc}}[0]),
                                    local_id: 4,
                                },
                            ),
                            res: Some(
                                Def(
                                    Mod,
                                    DefId(1:16 ~ std[512e]::prelude[0]::v1[0]),
                                ),
                            ),
                            args: None,
                            infer_args: false,
                        },
                    ],
                },
                Glob,
            ),
            vis: Spanned {
                node: Inherited,
                span: src/test/rustdoc/macro-in-closure.rs:1:1: 1:1,
            },
            span: src/test/rustdoc/macro-in-closure.rs:1:1: 1:1,
        },
        HirId {
            owner: DefId(0:2 ~ macro_in_closure[317d]::std[0]),
            local_id: 0,
        }: Item {
            ident: std#2,
            hir_id: HirId {
                owner: DefId(0:2 ~ macro_in_closure[317d]::std[0]),
                local_id: 0,
            },
            attrs: [
                Attribute {
                    kind: Normal(
                        AttrItem {
                            path: Path {
                                span: src/test/rustdoc/macro-in-closure.rs:1:1: 1:1,
                                segments: [
                                    PathSegment {
                                        ident: macro_use#1,
                                        id: NodeId(10),
                                        args: None,
                                    },
                                ],
                            },
                            args: Empty,
                        },
                    ),
                    id: AttrId(1),
                    style: Outer,
                    span: src/test/rustdoc/macro-in-closure.rs:1:1: 1:1,
                },
            ],
            kind: ExternCrate(
                None,
            ),
            vis: Spanned {
                node: Inherited,
                span: src/test/rustdoc/macro-in-closure.rs:1:1: 1:1,
            },
            span: src/test/rustdoc/macro-in-closure.rs:1:1: 1:1,
        },
        HirId {
            owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
            local_id: 0,
        }: Item {
            ident: main#0,
            hir_id: HirId {
                owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
                local_id: 0,
            },
            attrs: [],
            kind: Fn(
                FnSig {
                    header: FnHeader {
                        unsafety: Normal,
                        constness: NotConst,
                        asyncness: NotAsync,
                        abi: Rust,
                    },
                    decl: FnDecl {
                        inputs: [],
                        output: DefaultReturn(
                            src/test/rustdoc/macro-in-closure.rs:5:11: 5:11,
                        ),
                        c_variadic: false,
                        implicit_self: None,
                    },
                },
                Generics {
                    params: [],
                    where_clause: WhereClause {
                        predicates: [],
                        span: src/test/rustdoc/macro-in-closure.rs:5:10: 5:10,
                    },
                    span: src/test/rustdoc/macro-in-closure.rs:5:8: 5:8,
                },
                BodyId {
                    hir_id: HirId {
                        owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
                        local_id: 6,
                    },
                },
            ),
            vis: Spanned {
                node: Inherited,
                span: src/test/rustdoc/macro-in-closure.rs:5:1: 5:1,
            },
            span: src/test/rustdoc/macro-in-closure.rs:5:1: 9:2,
        },
    },
    trait_items: {},
    impl_items: {},
    bodies: {
        BodyId {
            hir_id: HirId {
                owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
                local_id: 2,
            },
        }: Body {
            params: [],
            value: Expr {
                hir_id: HirId {
                    owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
                    local_id: 2,
                },
                kind: Block(
                    Block {
                        stmts: [],
                        expr: None,
                        hir_id: HirId {
                            owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
                            local_id: 1,
                        },
                        rules: DefaultBlock,
                        span: src/test/rustdoc/macro-in-closure.rs:6:8: 8:6,
                        targeted_by_break: false,
                    },
                    None,
                ),
                attrs: ThinVec(
                    None,
                ),
                span: src/test/rustdoc/macro-in-closure.rs:6:8: 8:6,
            },
            generator_kind: None,
        },
        BodyId {
            hir_id: HirId {
                owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
                local_id: 6,
            },
        }: Body {
            params: [],
            value: Expr {
                hir_id: HirId {
                    owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
                    local_id: 6,
                },
                kind: Block(
                    Block {
                        stmts: [
                            Stmt {
                                hir_id: HirId {
                                    owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
                                    local_id: 4,
                                },
                                kind: Semi(
                                    Expr {
                                        hir_id: HirId {
                                            owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
                                            local_id: 3,
                                        },
                                        kind: Closure(
                                            Ref,
                                            FnDecl {
                                                inputs: [],
                                                output: DefaultReturn(
                                                    src/test/rustdoc/macro-in-closure.rs:6:8: 6:8,
                                                ),
                                                c_variadic: false,
                                                implicit_self: None,
                                            },
                                            BodyId {
                                                hir_id: HirId {
                                                    owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
                                                    local_id: 2,
                                                },
                                            },
                                            src/test/rustdoc/macro-in-closure.rs:6:5: 6:7,
                                            None,
                                        ),
                                        attrs: ThinVec(
                                            None,
                                        ),
                                        span: src/test/rustdoc/macro-in-closure.rs:6:5: 8:6,
                                    },
                                ),
                                span: src/test/rustdoc/macro-in-closure.rs:6:5: 8:7,
                            },
                        ],
                        expr: None,
                        hir_id: HirId {
                            owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
                            local_id: 5,
                        },
                        rules: DefaultBlock,
                        span: src/test/rustdoc/macro-in-closure.rs:5:11: 9:2,
                        targeted_by_break: false,
                    },
                    None,
                ),
                attrs: ThinVec(
                    None,
                ),
                span: src/test/rustdoc/macro-in-closure.rs:5:11: 9:2,
            },
            generator_kind: None,
        },
    },
    trait_impls: {},
    body_ids: [
        BodyId {
            hir_id: HirId {
                owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
                local_id: 6,
            },
        },
        BodyId {
            hir_id: HirId {
                owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
                local_id: 2,
            },
        },
    ],
    modules: {
        HirId {
            owner: DefId(0:0 ~ macro_in_closure[317d]),
            local_id: 0,
        }: ModuleItems {
            items: {
                HirId {
                    owner: DefId(0:1 ~ macro_in_closure[317d]::{{misc}}[0]),
                    local_id: 0,
                },
                HirId {
                    owner: DefId(0:2 ~ macro_in_closure[317d]::std[0]),
                    local_id: 0,
                },
                HirId {
                    owner: DefId(0:3 ~ macro_in_closure[317d]::main[0]),
                    local_id: 0,
                },
            },
            trait_items: {},
            impl_items: {},
        },
    },
    proc_macros: [],
warning: 1 warning emitted
}

cc #39412

The text was updated successfully, but these errors were encountered:

marmeladema · 2020-05-04T22:23:17Z

I managed to reproduce the bug with a regular macro rules:

#![feature(rustc_attrs)]

fn main() {
    || {
        #[macro_export] #[rustc_macro_transparency = "opaque"] macro_rules! m { () => {} }
    };
}

The problem is more broader that I originally thought but I still don't exactly know how to solve it quite yet.

…arent` Fixes rust-lang#71820

marmeladema · 2020-05-10T22:19:22Z

The issue is due to an AST simplification pass: ReplaceBodyWithLoop.
Before this pass, the name resolution pass creates a DefId for the macro with the closure expression as parent:
fn main -> block -> closure -> block -> macrodef
But later on, during ReplaceBodyWithLoop, the closure expression layer is removed and the AST ends up looking like:
fn main -> block -> block -> macrodef

Then, the macro is lowered but not its parent which explains why the closure DefId does not have an associated HirId.

As far as i know, this simplification pass is only used by rustdoc.

Fixes rust-lang#71820

@ghost

…op, r=pnkfelix Preserve `Expr`s that have `DefId`s in `ReplaceBodyWithLoop` This PR fixes rust-lang#71820 as well as the last part of rust-lang#71104 by preserving expressions that are assigned their own `DefId`s (closures and `async` blocks) when passing them to `rustdoc`. This avoids having a `DefId` without a corresponding `HirId`. The first commit in this PR makes `-Zunpretty=everybody_loops` actually work again, and the subsequent two are miscellaneous cleanup. They should probably get merged regardless of what we end up doing here. Sample input: ```rust fn foo() -> Box<i32> { let x = |a: i64| { const FOO: i64 = 1; }; let a = 4; Box::new(a) } ``` Sample output: ```rust fn foo() -> Box<i32> { || -> ! { const FOO: i64 = 1; loop { } }; loop { } } ``` r? @ghost

@ghost

…op, r=pnkfelix Preserve `Expr`s that have `DefId`s in `ReplaceBodyWithLoop` This PR fixes rust-lang#71820 as well as the last part of rust-lang#71104 by preserving expressions that are assigned their own `DefId`s (closures and `async` blocks) when passing them to `rustdoc`. This avoids having a `DefId` without a corresponding `HirId`. The first commit in this PR makes `-Zunpretty=everybody_loops` actually work again, and the subsequent two are miscellaneous cleanup. They should probably get merged regardless of what we end up doing here. Sample input: ```rust fn foo() -> Box<i32> { let x = |a: i64| { const FOO: i64 = 1; }; let a = 4; Box::new(a) } ``` Sample output: ```rust fn foo() -> Box<i32> { || -> ! { const FOO: i64 = 1; loop { } }; loop { } } ``` r? @ghost

@ghost

…op, r=pnkfelix Preserve `Expr`s that have `DefId`s in `ReplaceBodyWithLoop` This PR fixes rust-lang#71820 as well as the last part of rust-lang#71104 by preserving expressions that are assigned their own `DefId`s (closures and `async` blocks) when passing them to `rustdoc`. This avoids having a `DefId` without a corresponding `HirId`. The first commit in this PR makes `-Zunpretty=everybody_loops` actually work again, and the subsequent two are miscellaneous cleanup. They should probably get merged regardless of what we end up doing here. Sample input: ```rust fn foo() -> Box<i32> { let x = |a: i64| { const FOO: i64 = 1; }; let a = 4; Box::new(a) } ``` Sample output: ```rust fn foo() -> Box<i32> { || -> ! { const FOO: i64 = 1; loop { } }; loop { } } ``` r? @ghost

@ghost

…op, r=pnkfelix Preserve `Expr`s that have `DefId`s in `ReplaceBodyWithLoop` This PR fixes rust-lang#71820 as well as the last part of rust-lang#71104 by preserving expressions that are assigned their own `DefId`s (closures and `async` blocks) when passing them to `rustdoc`. This avoids having a `DefId` without a corresponding `HirId`. The first commit in this PR makes `-Zunpretty=everybody_loops` actually work again, and the subsequent two are miscellaneous cleanup. They should probably get merged regardless of what we end up doing here. Sample input: ```rust fn foo() -> Box<i32> { let x = |a: i64| { const FOO: i64 = 1; }; let a = 4; Box::new(a) } ``` Sample output: ```rust fn foo() -> Box<i32> { || -> ! { const FOO: i64 = 1; loop { } }; loop { } } ``` r? @ghost

@eddyb

Don't run `everybody_loops` for rustdoc; instead ignore resolution errors r? @eddyb cc @petrochenkov, @GuillaumeGomez, @Manishearth, @ecstatic-morse, @marmeladema ~~Blocked on rust-lang#73743 Merged. ~~Blocked on crater run.~~ Crater popped up some ICEs ([now fixed](rust-lang#73566 (comment))). See [crater run](https://crater-reports.s3.amazonaws.com/pr-73566/index.html), [ICEs](rust-lang#73566 (comment)). ~~Blocked on rust-lang#74070 so that we don't make typeck_tables_of public when it shouldn't be.~~ Merged. Closes rust-lang#71820, closes rust-lang#71104, closes rust-lang#65863. ## What is the motivation for this change? As seen from a lengthy trail of PRs and issues (rust-lang#73532, rust-lang#73103, rust-lang#71820, rust-lang#71104), `everybody_loops` is causing bugs in rustdoc. The main issue is that it does not preserve the validity of the `DefId` tree, meaning that operations on DefIds may unexpectedly fail when called later. This is blocking intra-doc links (see rust-lang#73101). This PR starts by removing `everybody_loops`, fixing rust-lang#71104 and rust-lang#71820. However, that brings back the bugs seen originally in rust-lang#43348: Since libstd documents items for all platforms, the function bodies sometimes do not type check. Here are the errors from documenting `libstd` with `everybody_loops` disabled and no other changes: ```rust error[E0433]: failed to resolve: could not find `handle` in `sys` --> src/libstd/sys/windows/ext/process.rs:13:27 | 13 | let handle = sys::handle::Handle::new(handle as *mut _); | ^^^^^^ could not find `handle` in `sys` error[E0425]: cannot find function `symlink_inner` in module `sys::fs` --> src/libstd/sys/windows/ext/fs.rs:544:14 | 544 | sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), false) | ^^^^^^^^^^^^^ not found in `sys::fs` error[E0425]: cannot find function `symlink_inner` in module `sys::fs` --> src/libstd/sys/windows/ext/fs.rs:564:14 | 564 | sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), true) | ^^^^^^^^^^^^^ not found in `sys::fs` ``` ## Why does this need changes to `rustc_resolve`? Normally, this could be avoided by simply not calling the `typeck_item_bodies` pass. However, the errors above happen before type checking, in name resolution itself. Since name resolution is intermingled with macro expansion, and rustdoc needs expansion to happen before it knows all items to be documented, there needs to be someway to ignore _resolution_ errors in function bodies. An alternative solution suggested by @petrochenkov was to not run `everybody_loops` on anything containing a nested `DefId`. This would solve some of the immediate issues, but isn't bullet-proof: the following functions still could not be documented if the items in the body failed to resolve: - Functions containing a nested `DefId` (rust-lang#71104) - ~~Functions returning `impl Trait` (rust-lang#43878 These ended up not resolving anyway with this PR. - ~~`const fn`, because `loop {}` in `const fn` is unstable (rust-lang#43636 `const_loop` was just stabilized. This also isn't exactly what rustdoc wants, which is to avoid looking at function bodies in the first place. ## What changes were made? The hack implemented in this PR is to add an option to ignore all resolution errors in function bodies. This is enabled only for rustdoc. Since resolution errors are ignored, the MIR generated will be invalid, as can be seen in the following ICE: ```rust error: internal compiler error: broken MIR in DefId(0:11 ~ doc_cfg[8787]::uses_target_feature[0]) ("return type"): bad type [type error] --> /home/joshua/src/rust/src/test/rustdoc/doc-cfg.rs:51:1 | 51 | / pub unsafe fn uses_target_feature() { 52 | | content::should::be::irrelevant(); 53 | | } | |_^ ``` Fortunately, rustdoc does not need to access MIR in order to generate documentation. Therefore this also removes the call to `analyze()` in `rustdoc::run_core`. This has the side effect of not generating all lints by default. Most lints are safe to ignore (does rustdoc really need to run liveness analysis?) but `missing_docs` in particular is disabled when it should not be. Re-running `missing_docs` specifically does not help, because it causes the typechecking pass to be run, bringing back the errors from rust-lang#24658: ``` error[E0599]: no method named `into_handle` found for struct `sys::unix::pipe::AnonPipe` in the current scope --> src/libstd/sys/windows/ext/process.rs:71:27 | 71 | self.into_inner().into_handle().into_raw() as *mut _ | ^^^^^^^^^^^ method not found in `sys::unix::pipe::AnonPipe` | ``` Because of rust-lang#73743, we only run typeck on demand. So this only causes an issue for functions returning `impl Trait`, which were already special cased by `ReplaceFunctionWithBody`. However, it now considers `async fn f() -> T` to be considered `impl Future<Output = T>`, where before it was considered to have a concrete `T` type. ## How will this affect future changes to rustdoc? - Any new changes to rustdoc will not be able to perform type checking without bringing back resolution errors in function bodies. + As a corollary, any new lints cannot require or perform type checking. In some cases this may require refactoring other parts of the compiler to perform type-checking only on-demand, see for example rust-lang#73743. + As a corollary, rustdoc can never again call `tcx.analysis()` unless this PR is reverted altogether. ## Current status - ~~I am not yet sure how to bring back `missing_docs` without running typeck. @eddyb suggested allowing lints to opt-out of type-checking, which would probably be another rabbit hole.~~ The opt-out was implemented in rust-lang#73743. However, of the rustc lints, now _only_ missing_docs is run and no other lints: rust-lang#73566 (comment). We need a team decision on whether that's an acceptable tradeoff. Note that all rustdoc lints are still run (`intra_doc_link_resolution_failure`, etc). **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment) - ~~The implementation of optional errors in `rustc_resolve` is very brute force, it should probably be moved from `LateResolver` to `Resolver` to avoid duplicating the logic in many places.~~ I'm mostly happy with it now. - This no longer allows errors in `async fn f() -> T`. This caused breakage in 50 crates out of a full crater run, all of which (that I looked at) didn't compile when run with rustc directly. In other words, it used to be that they could not be compiled but could still be documented; now they can't be documented either. This needs a decision from the rustdoc team on whether this is acceptable breakage. **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment) - ~~This makes `fn typeck_tables_of` in `rustc_typeck` public. This is not desired behavior, but needs the changes from rust-lang#74070 in order to be fixed.~~ Reverted.

@eddyb

Don't run `everybody_loops` for rustdoc; instead ignore resolution errors r? @eddyb cc @petrochenkov, @GuillaumeGomez, @Manishearth, @ecstatic-morse, @marmeladema ~~Blocked on rust-lang#73743 Merged. ~~Blocked on crater run.~~ Crater popped up some ICEs ([now fixed](rust-lang#73566 (comment))). See [crater run](https://crater-reports.s3.amazonaws.com/pr-73566/index.html), [ICEs](rust-lang#73566 (comment)). ~~Blocked on rust-lang#74070 so that we don't make typeck_tables_of public when it shouldn't be.~~ Merged. Closes rust-lang#71820, closes rust-lang#71104, closes rust-lang#65863. ## What is the motivation for this change? As seen from a lengthy trail of PRs and issues (rust-lang#73532, rust-lang#73103, rust-lang#71820, rust-lang#71104), `everybody_loops` is causing bugs in rustdoc. The main issue is that it does not preserve the validity of the `DefId` tree, meaning that operations on DefIds may unexpectedly fail when called later. This is blocking intra-doc links (see rust-lang#73101). This PR starts by removing `everybody_loops`, fixing rust-lang#71104 and rust-lang#71820. However, that brings back the bugs seen originally in rust-lang#43348: Since libstd documents items for all platforms, the function bodies sometimes do not type check. Here are the errors from documenting `libstd` with `everybody_loops` disabled and no other changes: ```rust error[E0433]: failed to resolve: could not find `handle` in `sys` --> src/libstd/sys/windows/ext/process.rs:13:27 | 13 | let handle = sys::handle::Handle::new(handle as *mut _); | ^^^^^^ could not find `handle` in `sys` error[E0425]: cannot find function `symlink_inner` in module `sys::fs` --> src/libstd/sys/windows/ext/fs.rs:544:14 | 544 | sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), false) | ^^^^^^^^^^^^^ not found in `sys::fs` error[E0425]: cannot find function `symlink_inner` in module `sys::fs` --> src/libstd/sys/windows/ext/fs.rs:564:14 | 564 | sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), true) | ^^^^^^^^^^^^^ not found in `sys::fs` ``` ## Why does this need changes to `rustc_resolve`? Normally, this could be avoided by simply not calling the `typeck_item_bodies` pass. However, the errors above happen before type checking, in name resolution itself. Since name resolution is intermingled with macro expansion, and rustdoc needs expansion to happen before it knows all items to be documented, there needs to be someway to ignore _resolution_ errors in function bodies. An alternative solution suggested by @petrochenkov was to not run `everybody_loops` on anything containing a nested `DefId`. This would solve some of the immediate issues, but isn't bullet-proof: the following functions still could not be documented if the items in the body failed to resolve: - Functions containing a nested `DefId` (rust-lang#71104) - ~~Functions returning `impl Trait` (rust-lang#43878 These ended up not resolving anyway with this PR. - ~~`const fn`, because `loop {}` in `const fn` is unstable (rust-lang#43636 `const_loop` was just stabilized. This also isn't exactly what rustdoc wants, which is to avoid looking at function bodies in the first place. ## What changes were made? The hack implemented in this PR is to add an option to ignore all resolution errors in function bodies. This is enabled only for rustdoc. Since resolution errors are ignored, the MIR generated will be invalid, as can be seen in the following ICE: ```rust error: internal compiler error: broken MIR in DefId(0:11 ~ doc_cfg[8787]::uses_target_feature[0]) ("return type"): bad type [type error] --> /home/joshua/src/rust/src/test/rustdoc/doc-cfg.rs:51:1 | 51 | / pub unsafe fn uses_target_feature() { 52 | | content::should::be::irrelevant(); 53 | | } | |_^ ``` Fortunately, rustdoc does not need to access MIR in order to generate documentation. Therefore this also removes the call to `analyze()` in `rustdoc::run_core`. This has the side effect of not generating all lints by default. Most lints are safe to ignore (does rustdoc really need to run liveness analysis?) but `missing_docs` in particular is disabled when it should not be. Re-running `missing_docs` specifically does not help, because it causes the typechecking pass to be run, bringing back the errors from rust-lang#24658: ``` error[E0599]: no method named `into_handle` found for struct `sys::unix::pipe::AnonPipe` in the current scope --> src/libstd/sys/windows/ext/process.rs:71:27 | 71 | self.into_inner().into_handle().into_raw() as *mut _ | ^^^^^^^^^^^ method not found in `sys::unix::pipe::AnonPipe` | ``` Because of rust-lang#73743, we only run typeck on demand. So this only causes an issue for functions returning `impl Trait`, which were already special cased by `ReplaceFunctionWithBody`. However, it now considers `async fn f() -> T` to be considered `impl Future<Output = T>`, where before it was considered to have a concrete `T` type. ## How will this affect future changes to rustdoc? - Any new changes to rustdoc will not be able to perform type checking without bringing back resolution errors in function bodies. + As a corollary, any new lints cannot require or perform type checking. In some cases this may require refactoring other parts of the compiler to perform type-checking only on-demand, see for example rust-lang#73743. + As a corollary, rustdoc can never again call `tcx.analysis()` unless this PR is reverted altogether. ## Current status - ~~I am not yet sure how to bring back `missing_docs` without running typeck. @eddyb suggested allowing lints to opt-out of type-checking, which would probably be another rabbit hole.~~ The opt-out was implemented in rust-lang#73743. However, of the rustc lints, now _only_ missing_docs is run and no other lints: rust-lang#73566 (comment). We need a team decision on whether that's an acceptable tradeoff. Note that all rustdoc lints are still run (`intra_doc_link_resolution_failure`, etc). **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment) - ~~The implementation of optional errors in `rustc_resolve` is very brute force, it should probably be moved from `LateResolver` to `Resolver` to avoid duplicating the logic in many places.~~ I'm mostly happy with it now. - This no longer allows errors in `async fn f() -> T`. This caused breakage in 50 crates out of a full crater run, all of which (that I looked at) didn't compile when run with rustc directly. In other words, it used to be that they could not be compiled but could still be documented; now they can't be documented either. This needs a decision from the rustdoc team on whether this is acceptable breakage. **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment) - ~~This makes `fn typeck_tables_of` in `rustc_typeck` public. This is not desired behavior, but needs the changes from rust-lang#74070 in order to be fixed.~~ Reverted.

@eddyb

Don't run `everybody_loops` for rustdoc; instead ignore resolution errors r? @eddyb cc @petrochenkov, @GuillaumeGomez, @Manishearth, @ecstatic-morse, @marmeladema ~~Blocked on rust-lang#73743 Merged. ~~Blocked on crater run.~~ Crater popped up some ICEs ([now fixed](rust-lang#73566 (comment))). See [crater run](https://crater-reports.s3.amazonaws.com/pr-73566/index.html), [ICEs](rust-lang#73566 (comment)). ~~Blocked on rust-lang#74070 so that we don't make typeck_tables_of public when it shouldn't be.~~ Merged. Closes rust-lang#71820, closes rust-lang#71104, closes rust-lang#65863. ## What is the motivation for this change? As seen from a lengthy trail of PRs and issues (rust-lang#73532, rust-lang#73103, rust-lang#71820, rust-lang#71104), `everybody_loops` is causing bugs in rustdoc. The main issue is that it does not preserve the validity of the `DefId` tree, meaning that operations on DefIds may unexpectedly fail when called later. This is blocking intra-doc links (see rust-lang#73101). This PR starts by removing `everybody_loops`, fixing rust-lang#71104 and rust-lang#71820. However, that brings back the bugs seen originally in rust-lang#43348: Since libstd documents items for all platforms, the function bodies sometimes do not type check. Here are the errors from documenting `libstd` with `everybody_loops` disabled and no other changes: ```rust error[E0433]: failed to resolve: could not find `handle` in `sys` --> src/libstd/sys/windows/ext/process.rs:13:27 | 13 | let handle = sys::handle::Handle::new(handle as *mut _); | ^^^^^^ could not find `handle` in `sys` error[E0425]: cannot find function `symlink_inner` in module `sys::fs` --> src/libstd/sys/windows/ext/fs.rs:544:14 | 544 | sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), false) | ^^^^^^^^^^^^^ not found in `sys::fs` error[E0425]: cannot find function `symlink_inner` in module `sys::fs` --> src/libstd/sys/windows/ext/fs.rs:564:14 | 564 | sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), true) | ^^^^^^^^^^^^^ not found in `sys::fs` ``` ## Why does this need changes to `rustc_resolve`? Normally, this could be avoided by simply not calling the `typeck_item_bodies` pass. However, the errors above happen before type checking, in name resolution itself. Since name resolution is intermingled with macro expansion, and rustdoc needs expansion to happen before it knows all items to be documented, there needs to be someway to ignore _resolution_ errors in function bodies. An alternative solution suggested by @petrochenkov was to not run `everybody_loops` on anything containing a nested `DefId`. This would solve some of the immediate issues, but isn't bullet-proof: the following functions still could not be documented if the items in the body failed to resolve: - Functions containing a nested `DefId` (rust-lang#71104) - ~~Functions returning `impl Trait` (rust-lang#43878 These ended up not resolving anyway with this PR. - ~~`const fn`, because `loop {}` in `const fn` is unstable (rust-lang#43636 `const_loop` was just stabilized. This also isn't exactly what rustdoc wants, which is to avoid looking at function bodies in the first place. ## What changes were made? The hack implemented in this PR is to add an option to ignore all resolution errors in function bodies. This is enabled only for rustdoc. Since resolution errors are ignored, the MIR generated will be invalid, as can be seen in the following ICE: ```rust error: internal compiler error: broken MIR in DefId(0:11 ~ doc_cfg[8787]::uses_target_feature[0]) ("return type"): bad type [type error] --> /home/joshua/src/rust/src/test/rustdoc/doc-cfg.rs:51:1 | 51 | / pub unsafe fn uses_target_feature() { 52 | | content::should::be::irrelevant(); 53 | | } | |_^ ``` Fortunately, rustdoc does not need to access MIR in order to generate documentation. Therefore this also removes the call to `analyze()` in `rustdoc::run_core`. This has the side effect of not generating all lints by default. Most lints are safe to ignore (does rustdoc really need to run liveness analysis?) but `missing_docs` in particular is disabled when it should not be. Re-running `missing_docs` specifically does not help, because it causes the typechecking pass to be run, bringing back the errors from rust-lang#24658: ``` error[E0599]: no method named `into_handle` found for struct `sys::unix::pipe::AnonPipe` in the current scope --> src/libstd/sys/windows/ext/process.rs:71:27 | 71 | self.into_inner().into_handle().into_raw() as *mut _ | ^^^^^^^^^^^ method not found in `sys::unix::pipe::AnonPipe` | ``` Because of rust-lang#73743, we only run typeck on demand. So this only causes an issue for functions returning `impl Trait`, which were already special cased by `ReplaceFunctionWithBody`. However, it now considers `async fn f() -> T` to be considered `impl Future<Output = T>`, where before it was considered to have a concrete `T` type. ## How will this affect future changes to rustdoc? - Any new changes to rustdoc will not be able to perform type checking without bringing back resolution errors in function bodies. + As a corollary, any new lints cannot require or perform type checking. In some cases this may require refactoring other parts of the compiler to perform type-checking only on-demand, see for example rust-lang#73743. + As a corollary, rustdoc can never again call `tcx.analysis()` unless this PR is reverted altogether. ## Current status - ~~I am not yet sure how to bring back `missing_docs` without running typeck. @eddyb suggested allowing lints to opt-out of type-checking, which would probably be another rabbit hole.~~ The opt-out was implemented in rust-lang#73743. However, of the rustc lints, now _only_ missing_docs is run and no other lints: rust-lang#73566 (comment). We need a team decision on whether that's an acceptable tradeoff. Note that all rustdoc lints are still run (`intra_doc_link_resolution_failure`, etc). **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment) - ~~The implementation of optional errors in `rustc_resolve` is very brute force, it should probably be moved from `LateResolver` to `Resolver` to avoid duplicating the logic in many places.~~ I'm mostly happy with it now. - This no longer allows errors in `async fn f() -> T`. This caused breakage in 50 crates out of a full crater run, all of which (that I looked at) didn't compile when run with rustc directly. In other words, it used to be that they could not be compiled but could still be documented; now they can't be documented either. This needs a decision from the rustdoc team on whether this is acceptable breakage. **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment) - ~~This makes `fn typeck_tables_of` in `rustc_typeck` public. This is not desired behavior, but needs the changes from rust-lang#74070 in order to be fixed.~~ Reverted.

jonas-schievink added A-hir Area: The high-level intermediate representation (HIR) A-macros-2.0 Area: Declarative macros 2.0 (#39412) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 3, 2020

marmeladema changed the title ~~Invalid lowering of declarative macro in closure~~ Invalid lowering of macro in closure May 4, 2020

marmeladema mentioned this issue May 9, 2020

Use get_parent_did to access parent hir_id instead of DefIdTree::parent #72039

Closed

marmeladema added a commit to marmeladema/rust that referenced this issue May 9, 2020

Use get_parent_did to access parent hir_id instead of `DefIdTree::p…

0d7cae3

…arent` Fixes rust-lang#71820

marmeladema mentioned this issue May 10, 2020

Do not remove closures during ReplaceBodyWithLoop pass #72088

Closed

jonas-schievink added the A-closures Area: closures (`|args| { .. }`) label May 10, 2020

marmeladema added a commit to marmeladema/rust that referenced this issue May 11, 2020

Do not remove closures during ReplaceBodyWithLoop pass

1941a43

Fixes rust-lang#71820

marmeladema mentioned this issue May 22, 2020

Remove macro_defs map #72284

Merged

jyn514 mentioned this issue Jun 7, 2020

Resolve items for cross-crate imports relative to the original module #73101

Merged

marmeladema mentioned this issue Jun 8, 2020

Preserve Exprs that have DefIds in ReplaceBodyWithLoop #73103

Closed

This was referenced Jun 20, 2020

Don't generate fake IDs in ReplaceBodyWithLoop #73532

Closed

Don't run everybody_loops for rustdoc; instead ignore resolution errors #73566

Merged

bors closed this as completed in c23f045 Jul 16, 2020

jyn514 mentioned this issue Aug 7, 2020

async-std docs no longer build on recent nightlies #75100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invalid lowering of macro in closure #71820

Invalid lowering of macro in closure #71820

marmeladema commented May 3, 2020

marmeladema commented May 4, 2020

marmeladema commented May 10, 2020

Invalid lowering of macro in closure #71820

Invalid lowering of macro in closure #71820

Comments

marmeladema commented May 3, 2020

marmeladema commented May 4, 2020

marmeladema commented May 10, 2020