module resolution regression on rustc 1.24.0-nightly (2017-12-21)

[SergioBenitez]

Beginning with rustc 1.24.0-nightly (250b49205 2017-12-21), rustc fails to find the appropriate file for a module when the module root is not mod_name/mod.rs or mod_name.rs (i.e, when using #[path]). This has broken ring, which has the following mod declarations:

In lib.rs:

#[path = "arithmetic/arithmetic.rs"]
mod arithmetic;

In arithmetic/arithmetic.rs:

pub mod montgomery;

And arithmetic/montgomery.rs contains the montgomery module code. This results in the following error:

error[E0583]: file not found for module `montgomery`
  --> $home/.cargo/registry/src/github.com-1ecc6299db9ec823/ring-0.12.1/src/arithmetic/arithmetic.rs:15:9
   |
15 | pub mod montgomery;
   |         ^^^^^^^^^^
   |
   = help: name the file either arithmetic/montgomery.rs or arithmetic/montgomery/mod.rs inside the directory "$home/.cargo/registry/src/github.com-1ecc6299db9ec823/ring-0.12.1/src/arithmetic"

Related issues: briansmith/ring#598, #46531

[SergioBenitez]

cc @cramertj @nikomatsakis

[est31]

Ring has #![allow(legacy_directory_ownership)] inside its lib.rs, cc #37872. Seems that #46531 has turned this into a hard error. Not sure if this was intended, but it didn't refer to the tracking issue #37872 at all. Small reproduction:

src/lib.rs:

#[path="foo/foo.rs"] pub mod foo;

src/foo/foo.rs:

pub mod bar;

src/foo/bar.rs:

// Empty

#34596 strikes again... You shouldn't be able to disable forward compatibility lints.

[cramertj]

Oh, interesting. This is a bit of a weird case-- what's happening here is that, when using #[path="foo/foo.rs"] pub mod foo;, the non_modrs_mods feature will look for an inner pub mod bar; in foo/foo/bar.rs or foo/foo/bar/mod.rs, whereas this legacy feature looks for it in foo/bar.rs or foo/bar/mod.rs.

The new behavior is motivated because foo.rs is not a mod.rs file, so its child modules are nested inside foo/* relative to foo.rs's directory.

I'm not quite sure the best way to solve this. AFAIK there's no way to check the value of feature gates before parsing the files, and it isn't always valid to pull in both because foo.rs might have a legitimate sibling named bar.rs which is unrelated to foo/bar.rs.

Looking at #37872, it seems like this lint has been around for quite some time and was made deny-by-default in #42894. While it wasn't originally my intention, I personally think the best solution here is to move forward with the breakage.

[SergioBenitez]

It sounds like this is intended breakage with a fair warning period that was intentionally ignored by ring. As such, this is a non-issue. Thanks for the input @cramertj, @est31!

[cramertj]

I feel we ought to do some kind of crater run on this change at least, so this might be worth reopening. ring has quite a few transitive dependencies, and I'm worried about breaking all of the ones not using the latest version.

cc @jseyfried, with whom I was discussing this in another thread.

[nikomatsakis]

I'm going to re-open the ticket for discussion. I think I agree that this was intended breakage that ring ignored, but I also agree that we want to minimize the disruption if at all possible -- it ought to be possible to check if the feature-gate is enabled, I suppose, though it's gonna be annoying (and, later, just opt-in with the new epoch).

[nikomatsakis]

OK, chatting on IRC, @eddyb and I came up with a concrete plan of action:

We could add -Zepoch=2018 (which implicitly requires nightly) and enable the new feature only when in that epoch.

Until then, the old behavior would be preserved.

@cramertj, do you think that'd be easy to do? I'm not 100% sure on what path the code was taking before.

[cramertj]

@nikomatsakis Sure, we can just switch on the -Zepoch flag when finding children of a non-mod.rs #[path] file and not report a feature gate error in that case, and we can report an error when we hit a the feature gate in combination with the -Zepoch flag. The bigger question I have is whether we want to wait for the next epoch in order to start supporting mod statements in non-mod.rs files. I think that's probably reasonable, but I don't know what the goal timeline was.

[nikomatsakis]

I guess I think it's ok to wait for the new epoch -- I would really prefer not to break ring. I think this is a grey line case, where we can justify making the change, but I am not happy with the real-world effects, so it may not be worth doing it.

[cramertj]

Agreed. I will make the change.

[daboross]

Sorry if this has already been elaborated on, but could someone with more knowledge clarify this for me?

Why does the mod resolution in the new system depend on where the file actually is, rather than what the mod name is?

I thought foo::bar would always be located at src/foo/bar.rs, regardless of where the foo mod file is located? In the new system, foo could be at src/foo.rs or src/foo/mod.rs. Is there a place where it was decided to treat mod.rs specially in order to get this to work, rather than just ignoring the filename of foo entirely, and only depending on the absolute path of foo::bar for where the file should be located?

If someone could point me to where this was previously discussed, that would be awesome. I'll keep looking around, but I can't find anything directly in the https://github.com/rust-lang/rfcs/blob/8ee535b4fcc8bb22c121ad19a36414f1259397c0/text/2126-path-clarity.md which clarifies this.

[aidanhs]

I've just stumbled across this because rustoto_core hasn't released a new version since removing a dependency on ring. Where is this issue up to? Is it worth backing out the previous PR until a new one with the epoch suggestion is ready?

[cramertj]

@daboross

I thought foo::bar would always be located at src/foo/bar.rs, regardless of where the foo mod file is located?

Your impression is that a file src/whoodliewhatsit/foo.rs containing a mod baz;, introduced by #[path="whoodliewhatsit/foo.rs"] mod foo; in lib.rs, would cause baz to be included from src/foo/baz.rs rather than src/whoodliewhatsit/foo/baz.rs? This seems strange to me personally because I would expect submodules to be paired with their parent modules, but the RFC does not clarify the behavior, and your interpretation would allow the existing code to continue working-ish.

@aidanhs

Where is this issue up to?

I'm currently working on a branch that moves this change into -Zepoch 2018. That change should be backported to beta, which will fix the affected crates.

[daboross]

@cramertj

Your impression is that a file src/whoodliewhatsit/foo.rs containing a mod baz;, introduced by #[path="whoodliewhatsit/foo.rs"] mod foo; in lib.rs, would cause baz to be included from src/foo/baz.rs rather than src/whoodliewhatsit/foo/baz.rs? This seems strange to me personally because I would expect submodules to be paired with their parent modules, but the RFC does not clarify the behavior, and your interpretation would allow the existing code to continue working-ish.

Yes, that would be my interpretation of the behavior, given that the crate root is at src/lib.rs. It seems just like a different interpretation of the "path" attribute: I would interpret #[path="whoodliewhatsit/foo.rs"] mod foo; as "the file for foo itself is here rather than src/foo.rs" not "the foo module and all of its submodules are starting at whoodliewhatsit/foo.rs".

You're right that this is pretty ambiguous, which is why I would prefer the "only this file directly is affected" interpretation over the one currently implemented. Once #37872 happens, all submodules of #[path] marked modules will need #[path] for clarification anyways, so the behavior either way won't last longer than that.