Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Weird filesystem hierarchy with nested modules #55094

Closed
Undin opened this issue Oct 15, 2018 · 15 comments
Closed

Weird filesystem hierarchy with nested modules #55094

Undin opened this issue Oct 15, 2018 · 15 comments
Assignees
Labels
A-resolve Area: Path resolution P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@Undin
Copy link
Contributor

Undin commented Oct 15, 2018

Compiler: rustc 1.31.0-nightly (14f42a7 2018-10-14)

I'm trying to figure out how module system should work in edition 2018 without mod.rs to support it in IntelliJ Rust

I have the following project structure: https://gist.github.com/Undin/eaef0a4765f5a991f6e62f8d07b878db.
And rustc can't compile this code in edition 2015 and 2018 with the following error

error[E0583]: file not found for module `qqq`
 --> src/foo.rs:5:13
  |
5 |     pub mod qqq;
  |             ^^^
  |
  = help: name the file either foo/qqq.rs or foo/qqq/mod.rs inside the directory "src/baz"

error: aborting due to previous error

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

But if I move qqq.rs into src/baz/foo folder rustc will compile it in both editions.

Problems:

  • This code souldn't be compilable in edition 2015.
    At least, rustc 1.29.2 shows the following error
error[E0658]: mod statements in non-mod.rs files are unstable (see issue #44660)
 --> src/foo.rs:5:13
  |
5 |     pub mod qqq;
  |             ^^^
  |
  = help: on stable builds, rename this file to foo/mod.rs

error: aborting due to previous error

For more information about this error, try `rustc --explain E0658`.
  • src/baz/foo/qqq.rs is rather unexpected because baz is child module of foo. I think qqq.rs should be located in src/foo/baz or in src/baz folder but I'm not sure.

It would be great to understand how it should work to implement the corresponding support in the plugin

@estebank estebank added the A-diagnostics Area: Messages for errors, warnings, and lints label Oct 15, 2018
@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated A-resolve Area: Path resolution T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed A-diagnostics Area: Messages for errors, warnings, and lints labels Oct 15, 2018
@nikomatsakis
Copy link
Contributor

I'm nominating this issue. This would seem to be a candidate for RC2 blocker:

Do we expect to be able to create non-inline modules (e.g., mod qqq;) inside of inline ones (mod baz { mod qqq; })? If so, what path do we expect for said modules.

I don't really expect that to be permitted. But I forget precisely what our behavior was historically here, it seems like we ought to test it. (That is, using mod.rs files.)

@Mark-Simulacrum
Copy link
Member

This is, in fact, used inside the compiler I believe -- e.g. syntax does this here: https://github.com/rust-lang/rust/blob/master/src/libsyntax/lib.rs#L113-L145 multiple times, librustc does so as well.

If I'm understanding this issue correctly this is also a 1.30 release blocker essentially -- mod.rs is stabilized in 1.30 AFAIK on both 2015 and 2018 edition(s).

@Mark-Simulacrum Mark-Simulacrum added the P-high High priority label Oct 15, 2018
@Mark-Simulacrum Mark-Simulacrum added this to the 1.30 milestone Oct 15, 2018
@ehuss
Copy link
Contributor

ehuss commented Oct 16, 2018

I was curious, so I did some poking. The fix looks easier than I expected, but of course I'm not very familiar with this code so I'm not sure if I missed anything. I posted a pr at #55108 with what I believe might fix this (running under the assumption that filesystem paths should match the mod paths).

@ehuss
Copy link
Contributor

ehuss commented Oct 18, 2018

Ping. I'm going to guess that there's nothing that can be done for 1.30? If this is fixed in 1.31, is it ok that it will have different behavior than 1.30?

@pnkfelix pnkfelix self-assigned this Oct 18, 2018
@pnkfelix
Copy link
Member

assigning self to take charge on this. I suspect I won't have time to investigate properly before the lang team meeting tonight, but we will see.

@nikomatsakis
Copy link
Contributor

@ehuss thanks for doing poking and opening #55108! I personally feel a bit confused by all the notes. It seems like we need a sort of comprehensive write-up documenting the behavior and trying to make some sort of rationalization for it.

@cramertj
Copy link
Member

I'll take a look at this, too.

@ehuss
Copy link
Contributor

ehuss commented Oct 18, 2018

Sure, yea, I should have summarized it better. I'll try to make it clear.

mod statements inside inline mod statements in non-mod-rs files use the wrong path to load. Example:

main.rs:

mod foo;

fn main() {
    foo::inline::inner::f();
}

foo.rs:

pub mod inline {
    pub mod inner;
}

inline/foo/inner.rs:

pub fn f() {}

In this example, the mod path is foo::inline::inner, but the code requires the filesystem path to be inline/foo/inner.rs, notice the inline and foo are swapped. This is caused by the code push_directory in parser.rs ignores when DirectoryOwnership::Owned{ relative } is set.

The simple fix is to push the relative directory first inside that function, however this runs afoul with #[path] attributes. Since at least rust 1.0, you were able to perform the new non-mod-rs behavior of 1.30 using attributes, but the paths are weird. Here's an example:

main.rs:

mod foo;
fn main() {
    foo::inline::inner::f();
}

foo.rs:

pub mod inline {
    #[path="inner_actual.rs"]
    pub mod inner;
}

inline/inner_actual.rs:

pub fn f () {}

Notice the module path is foo::inline::inner, but the filesystem path is inline/inner_actual.rs, notice the path does not include foo. The general rule with #[path] is that it does not include the id of the file it is in.

So I'm not sure what to do here. There are a variety of choices to address this, but none of them seem good. I listed some options towards the bottom #55108. Another idea is to follow the #[path] naming convention and not include the id of the non-mod-rs file in the filesystem path. I find that unsatisfying because it's hard to explain how module paths map to filesystem paths.

@joshtriplett
Copy link
Member

Quick summary from a discussion (and confusion) in the lang team:

  • You should be able to have foo.rs, or foo/mod.rs, and those should be equivalent.
  • The confusion is this: if you have a foo.rs that has a mod baz { mod qqq; }, do we expect to find qqq.rs in foo/ or foo/baz/? Several of us feel the latter makes far more sense.

@Centril Centril removed I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 18, 2018
@Centril Centril assigned cramertj and unassigned pnkfelix Oct 18, 2018
@Centril
Copy link
Contributor

Centril commented Oct 18, 2018

@cramertj will implement the fix per @joshtriplett's notes.

@cramertj cramertj added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Oct 18, 2018
@cramertj
Copy link
Member

cramertj commented Oct 18, 2018

Tagging as regression from stable to beta because the current incorrect behavior would stabilize in 1.30.

@cramertj
Copy link
Member

This is happening because when we parse mod y { ... } inside x.rs, we amend the current directory path by pushing y onto it. Since we've remembered that we need to append the current module name (x) onto the current path (was /, now is /y), we get a path that starts with all of the in-file modules (/y) and ends with the extension that made paths relative (/x).

This mutation of the current directory path to actually point at a new path that matches the inline mod ... { ... } declarations rather than the actual current file means that is also observable via other path-based functionality such as includes via the #[path = "..."] attribute, so pub mod x { pub mod y { pub mod z { #[path="top.rs"] pub mod a; }}} will look for a file at ./x/y/z/top.rs. This is visible on stable.

@cramertj
Copy link
Member

Anyways, as @ehuss said the current behavior here around #[path] is pretty nonsense, but it's visible from stable. Personally, I think we should probably go ahead with a fix to the non-mod.rs mods case, but we probably can't get away with the breaking change to #[path], so we'll need to track the directory for #[path] separately from the one for nested mod foo; files, which is sort of annoying. I'll open a PR tomorrow with that behavior.

bors pushed a commit that referenced this issue Oct 20, 2018
Flatten relative offset into directory path before adding inline
(mod x { ... }) module names to the current directory path.

Fix #55094
bors added a commit that referenced this issue Oct 20, 2018
Fix ordering of nested modules in non-mod.rs mods

Flatten relative offset into directory path before adding inline
(mod x { ... }) module names to the current directory path.

Fix #55094
@pnkfelix
Copy link
Member

pnkfelix commented Oct 25, 2018

Doing a survey prior to weekly compiler team meeting and summarizing state of P-high issues.

As you can see from reading the discussion on #55192, it was too close to the release for us to land the proposed PR for this cycle. So the plan of record is to remove the stabilization of non-mod.rs mods for the imminent release, and then take care of this problem as part of the next release cycle.

(However, I'm not certain that the plan of record is entirely accurate; i've been given the impression that the point of cutting the new beta was delayed by about a week, which may mean that PR #551912 will could land in time? Not clear.)

bors added a commit that referenced this issue Oct 28, 2018
Fix ordering of nested modules in non-mod.rs mods

Flatten relative offset into directory path before adding inline
(mod x { ... }) module names to the current directory path.

Fix #55094
@Mark-Simulacrum
Copy link
Member

For the record, the feature was destabilized in 1.30 and will be patched in 1.31 (and remains stable on the current beta and nightly).

pietroalbini pushed a commit to alexcrichton/rust that referenced this issue Oct 29, 2018
Flatten relative offset into directory path before adding inline
(mod x { ... }) module names to the current directory path.

Fix rust-lang#55094
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-resolve Area: Path resolution P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants