Expansion-driven outline module parsing

[Centril]

After this PR, the parser will not do any conditional compilation or loading of external module files when mod foo; is encountered. Instead, the parser only leaves mod foo; in place in the AST, with no items filled in. Expansion later kicks in and will load the actual files and do the parsing. This entails that the following is now valid:

#[cfg(FALSE)]
mod foo {
    mod bar {
        mod baz; // `foo/bar/baz.rs` doesn't exist, but no error!
    }
}

Fixes #64197.

r? @petrochenkov

[Centril]

cc @matklad

[petrochenkov]

r=me with #69838 (comment) addressed.

[Centril]

So the pretty test (src/test/pretty/issue-12590-a.rs) is failing and I'm not quite sure why it happens.

// exec-env:RUSTC_LOG=rustc_expand=debug
// rustc-env:RUST_BACKTRACE=1
// rustc-env:RUSTC_LOG=rustc_expand=debug
// compile-flags:-Ztreat-err-as-bug
// pp-exact

// The next line should not be expanded

#[path = "issue-12590-b.rs"]
mod issue_12590_b;

fn main() { }

In the same directory, there is a file (src/test/pretty/issue-12590-b.rs) whose contents are unimportant.

Running the test with results in:

---- [pretty] pretty/issue-12590-a.rs stdout ----

error: pretty-printed source does not typecheck
status: exit code: 101
command: "/home/centril/programming/rust/rust-gamma/build/x86_64-unknown-linux-gnu/stage1/bin/rustc" "-" "-Zno-codegen" "--out-dir" "/home/centril/programming/rust/rust-gamma/build/x86_64-unknown-linux-gnu/test/pretty/issue-12590-a/issue-12590-a.pretty-out" "--target=x86_64-unknown-linux-gnu" "-L" "/home/centril/programming/rust/rust-gamma/build/x86_64-unknown-linux-gnu/test/pretty" "-L" "/home/centril/programming/rust/rust-gamma/build/x86_64-unknown-linux-gnu/test/pretty/issue-12590-a/auxiliary.pretty" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/home/centril/programming/rust/rust-gamma/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Ztreat-err-as-bug"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
[DEBUG rustc_expand::expand] expand_crate, module #0 = ModuleData { mod_path: [rust_out#0], directory: "<anon>" }
[DEBUG rustc_expand::expand] expand_crate, module #1 = ModuleData { mod_path: [rust_out#0], directory: "" }
[DEBUG rustc_expand::module] parse_external_mod(id=issue_12590_b#0, span=Span { lo: BytePos(248), hi: BytePos(266), ctxt: 
#0 }, ownership=Owned { relative: None }, path="", attrs=[Attribute { kind: Normal(AttrItem { path: Path { span: Span { lo: BytePos(221), hi: BytePos(225), ctxt: #0 }, segments: [PathSegment { ident: path#0, id: NodeId(4294967040), args: None }] }, args: Eq(Span { lo: BytePos(226), hi: BytePos(227), ctxt: #0 }, TokenStream([(Token(Token { kind: Literal(Lit { kind: Str, symbol: "issue-12590-b.rs", suffix: None }), span: Span { lo: BytePos(228), hi: BytePos(246), ctxt: #0 } }), NonJoint)])) }), id: AttrId(0), style: Outer, span: Span { lo: BytePos(219), hi: BytePos(247), ctxt: #0 } }]
[DEBUG rustc_expand::module] parse_external_mod, mp(ownership=Owned { relative: None }, path="issue-12590-b.rs")
error: couldn't read issue-12590-b.rs: No such file or directory (os error 2)
  --> <anon>:10:1
   |
10 | mod issue_12590_b;
   | ^^^^^^^^^^^^^^^^^^

thread 'rustc' panicked at 'aborting due to `-Z treat-err-as-bug=1`', src/librustc_errors/lib.rs:901:13

and it fails on let mut p0 = new_sub_parser_from_file(sess, &mp.path, Some(id.to_string()), span);.

For debugging purposes I have added:

    log::debug!(
        "parse_external_mod(id={:?}, span={:?}, ownership={:?}, path={:?}, attrs={:?}",
        id,
        span,
        ownership,
        path,
        attrs
    );

on function entry and I have:

        log::debug!("parse_external_mod, mp(ownership={:?}, path={:?})", mp.ownership, mp.path);

        // Actually parse the external file as amodule.
        let mut p0 = new_sub_parser_from_file(sess, &mp.path, Some(id.to_string()), span);

In MacroExpander::expand_crate we have:

    pub fn expand_crate(&mut self, mut krate: ast::Crate) -> ast::Crate {
        let mut module = ModuleData {
            mod_path: vec![Ident::from_str(&self.cx.ecfg.crate_name)],
            directory: match self.cx.source_map().span_to_unmapped_path(krate.span) {
                FileName::Real(path) => path,
                other => PathBuf::from(other.to_string()),
            },
        };
        log::debug!("expand_crate, module #0 = {:?}", module);
        module.directory.pop();
        log::debug!("expand_crate, module #1 = {:?}", module);
        self.cx.root_path = module.directory.clone();
        self.cx.current_expansion.module = Rc::new(module);

I suspect there may be a deficiency in compiletest itself... investigating further.

[Centril]

So run_pretty_test has:

        // Finally, let's make sure it actually appears to remain valid code
        let proc_res = self.typecheck_source(actual);
        if !proc_res.status.success() {
            self.fatal_proc_rec("pretty-printed source does not typecheck", &proc_res);
        }

where actual: String (the source code, not a path). So basically we're sending the stuff to type check via stdin. For some reason (incremental stuff?) it also seems like we never actually try to parse a module when doing the type checking of actual.

[Centril]

Hmm, so the #12590 test is purely about pretty printing. One easy fix which would make the PR green is to use // pretty-compare-only -- but do we think that would be correct? cc also @eddyb

[Centril]

Hmm, so the #12590 test is purely about pretty printing. One easy fix which would make the PR green is to use // pretty-compare-only -- but do we think that would be correct? [...]

@petrochenkov I've done this in 42ab820 but would like feedback on it before r=you-ing.

[eddyb]

Using stdin seems odd, maybe compiletest should set the cwd of the rustc process it runs to the directory of the test, which might make file loading work correctly.

// pretty-compare-only seems like the appropriate workaround, in any case.

[Centril]

// pretty-compare-only seems like the appropriate workaround, in any case.

Alright; then let's do: @bors r=petrochenkov,eddyb

[bors]

📌 Commit 42ab820 has been approved by petrochenkov,eddyb

[bors]

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

[eddyb]

This apparently broke Servo's script crate, or at least the copy on perf.rust-lang.org.
cc @Mark-Simulacrum can you post some details?

IIUC, this can't be fixed locally either, because the mod bar; parsed by include!("directory/included.rs"); will be expanded later, so for expanding mod bar; to load directory/bar.rs (which is apparently the old behavior? how are we not testing for it), we'd need to detect that an include! happened, or something similar.

EDIT: adjusted example to fit @Mark-Simulacrum's reproduction.

[Centril]

cc @petrochenkov

[Mark-Simulacrum]

This is a minimal reproduction:

$ tree .
.
|-- directory
|   |-- bar.rs
|   `-- included.rs
`-- foo.rs

// foo.rs
include!("directory/included.rs");

fn main() {}

// directory/included.rs
mod bar;

// directory/bar.rs
// empty

$ rustc +3c6f982cc908aacc39c3ac97f31c989f81cc213c foo.rs
error[E0583]: file not found for module `bar`
 --> directory/included.rs:1:1
  |
1 | mod bar;
  | ^^^^^^^^
  |
  = help: to create the module `bar`, create file "bar.rs"

error: aborting due to previous error

For more information about this error, try `rustc --explain E0583`.

[petrochenkov]

I've just recalled a related issue - #58818.
Could you check whether this PR fixed it?

[petrochenkov]

we'd need to detect that an include! happened, or something similar.

FWIW, we have Definitions::expansions_that_defined that can link from the mod bar; item to the macro that produced it. Then we have ExpnData::parent that can link from that macro to the macro that produced it (recursively).

Perhaps using these data we can hack up something reproducing the old behavior.

[petrochenkov]

We can only detect include comparing the macro's name in ExpnData with sym::include.
fn expansion_cause used in for a bunch of various stuff also hard-codes the name include like this, so it should be ok for now.

(The proper fix is a flag in struct SyntaxExtension, other file-including built-in macros should have it as well.)

[Centril]

@petrochenkov So I tried two different approaches to set ownership before loading the external module. The first one was adding Incovation::is_current_expansion_macro_named defined like so:

    fn is_current_expansion_macro_named(&self, sym: Symbol) -> bool {
        let mut curr = self.cx.current_expansion.id;
        loop {
            let expn_data = curr.expn_data();
            if expn_data.kind == ExpnKind::Macro(MacroKind::Bang, sym) {
                // Stop going up the backtrace once `sym` is encountered.
                return true;
            }
            if expn_data.is_root() {
                // Reached root, need to bail to ensure termination.
                return false;
            }
            curr = expn_data.call_site.ctxt().outer_expn();
        }
    }

and then using it in flat_map_item before parse_external_mod happens:

                let pushed = &mut false; // Record `parse_external_mod` pushing so we can pop.
                let mut dir = Directory { ownership: orig_ownership, path: module.directory };

                if self.is_current_expansion_macro_named(sym::include) {
                    log::debug!("flat_map_item, is_current_expansion_macro_named = true");
                    dir.ownership = DirectoryOwnership::Owned { relative: None };
                }

The log message here does trigger on @Mark-Simulacrum's regression test, but this doesn't fix the issue.

I also tweaked SyntaxExtension, adding a flag foobar: bool which is used in fully_expand_fragment:

            let (expanded_fragment, new_invocations) = match res {
                InvocationRes::Single(ext) => match self.expand_invoc(invoc, &ext.kind) {
                    ExpandResult::Ready(fragment) => {
                        if ext.foobar {
                            self.cx.current_expansion.directory_ownership =
                                DirectoryOwnership::Owned { relative: None };
                            log::debug!("fully_expand_fragment, using foobar");
                        }
                        self.collect_invocations(fragment, &[])

and which is set in compile_macro:

        if result.is_builtin {
            // The macro was marked with `#[rustc_builtin_macro]`.
            if let Some(ext) = self.builtin_macros.remove(&item.ident.name) {
                // The macro is a built-in, replace its expander function
                // while still taking everything else from the source code.
                result.kind = ext.kind;

                if item.ident.name == sym::include {
                    result.foobar = true;
                    log::debug!("compile_macro, setting foobar");
                }

The log message also triggers here, but also does nothing to fix the issue, as the approach is the same as the other mechanism.

I'm not sure why it doesn't work. :(

[Centril]

Found a fix, PR up at #70184.