possible regression around paths and macro-generated modules

[durka]

I have some code that builds on stable and does not build on beta. I am not sure whether it is exploiting a compiler bug or not, but the changes seem related to #37602 (cc @jseyfried).

Here is the code: https://github.com/durka/mod-dir-regr

Note that if you pretty=expanded the code on stable, then use the beta compiler to compile it, it does work. So there is something going on with the macro.

[durka]

Oh yeah, and the error:

seas:mod-dir-regr alex$ cargo +beta build
   Compiling mod-dir-regr v0.1.0 (file:///Users/alex/mod-dir-regr)
error: file not found for module `wrapper`
 --> src/lib.rs:8:9
  |
8 |     mod wrapper;
  |         ^^^^^^^
  |
  = help: name the file either wrapper.rs or wrapper/mod.rs inside the directory ""

error: Could not compile `mod-dir-regr`.

To learn more, run the command again with --verbose.

[jseyfried]

This was unintentionally introduced in #37569.

Investigating further, it appears that non-inline modules parsed into item fragments were already buggy before #37569. For example, wrapping mod-dir-regr's group_attr! { ... } invocation in an inline module mod foo { group_attr! { ... } } didn't change wrapper's path, but wrapping the invocation in a non-inline module mod foo; at foo/mod.rs did.

I think the best way forward is to fix this regression and the pre-existing bug at once and backport to beta (I'll submit a PR by tomorrow). Fixing the pre-existing bug technically a breaking change, but I believe it won't break anything not already broken by #37569, and I haven't seen any breakage from #37602 in the weekly regression reports.

[durka]

Well this did break my code, I was using a non-stripped-down version of group_attr! in production. But if you tell me I was relying on a bug before, I'd believe you.

[durka]

Another weird part of the regression has to do with #[path]. The path given to the attribute is supposed to be relative to the current file. But once you are inside this strange path-less module created by group_attr!, it becomes relative to the directory where rustc is invoked. Try replacing mod wrapper; with #[path="wrapper/mod.rs"] mod wrapper;, and cargo build from the crate root doesn't work. But go into the src directory, then it does work. Crazy.

[jseyfried]

@durka
Does your code have a relevant group_attr! invocation in an inline module? That's the only way the pre-existing bug could apply.

But once you are inside this strange path-less module created by group_attr!, it becomes relative to the directory where rustc is invoked

Right, that's the regression that #37569 introduced. We've never set the directory of the fragment parser, so the parser constructor picks a default directory, the directory of the file containing the span of the first token, or an empty path if there is no such file (e.g. the span is DUMMY_SP).

When I lifted parser construction out of a loop, the initial token changed so that its span became DUMMY_SP, causing the directory path to be empty -- semantically, is the current directory.

Even when the first token's span has a file, its directory isn't always the correct choice since it doesn't take into account inline modules in the file (this is the preexisting bug).

[durka]

My invocations of group_attr! are all at crate roots.

[durka]

What I'm not clear about is if mod __internal { mod wrapper; } should ever have worked at all (loading "./wrapper/mod.rs") or whether it should really be looking for "./__internal/wrapper/mod.rs".

[jseyfried]

We load the module's file when we parse mod wrapper; as an item fragment, so we don't know about mod __internal yet.

[jseyfried]

Ideally, we would load the file for a non-inline module more lazily, so that we knew exactly where will end up in the module hierarchy before we look for the file. We might be able to implement this today with negligible breakage.

[jseyfried]

Well ideally the directory would be hygienic, so that

mod foo {
    pub macro m($i:item, $id:ident) {
        mod bar {
            mod baz; // `foo/bar/baz.rs` or `foo/bar/baz/mod.rs`
            $i; // `baz/quux.rs` or `baz/quux/mod.rs`
            mod $id; // `baz/bar/quux.rs` or `baz/bar/quux/mod.rs` (?)
        }
    }
}

mod baz {
    ::foo::m!(mod quux;, quux);
}

[jseyfried]

cc @nrc

[durka]

Oh man, so the interpretation of a mod statement in a macro input depends on the macro pattern, e.g.

macro_rules! m { ($i:item) => { mod x { $i } } }

m! { mod y; } // error: no file y.rs or y/mod.rs

macro_rules! as_item { ($i:item) => { $i } }
macro_rules! m { ($($t:tt)*) => { as_item!(mod x { $($t)* }); } }

m! { mod y; } // error: no file x/y.rs or x/y/mod.rs

Fun! I guess this likely can't be changed due to backcompat.

[jseyfried]

I think changing to #38190 (comment) wouldn't cause breakage in practice. If it does, we'll have to wait for macros 2.0.