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

Fix mod from inline mod in non-mod-rs. #55108

Closed
wants to merge 1 commit into from

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Oct 16, 2018

This works under the assumption that filesystem paths should match mod paths.

When a mod is loaded from within an inline mod, it was using the wrong path when
inside a non-mod-rs file. It was not including the path components from the
non-mod-rs file.

This change also includes:

  • Added a test to the run-pass test.
  • Added a test to verify the error message when missing.
  • Remove unused non_modrs_mods for feature gate diagnostics.
  • Fixes the run-pass test, it was accidentally removed during stabilization.
  • Remove the vestiges of the feature gate tests in test/ui.
  • Enable the diagnostic test on windows, there didn't seem to be a reason to
    disable it.

Fixes #55094

This works under the assumption that filesystem paths should match mod paths.

When a mod is loaded from within an inline mod, it was using the wrong path when
inside a non-mod-rs file. It was not including the path components from the
non-mod-rs file.

This change also includes:
- Added a test to the run-pass test.
- Added a test to verify the error message when missing.
- Remove unused `non_modrs_mods` for feature gate diagnostics.
- Fixes the run-pass test, it was accidentally removed during stabilization.
- Remove the vestiges of the feature gate tests in `test/ui`.
- Enable the diagnostic test on windows, there didn't seem to be a reason to
  disable it.
@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 16, 2018
@ehuss
Copy link
Contributor Author

ehuss commented Oct 16, 2018

There is a second commit that fixes it for #[path] attributes as well. However, I think it will break rustfmt. I vaguely recall that tools can't be broken at certain times, so I'm not sure what to do about that.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:48:59] .................................................................................................... 2200/4602
[00:49:03] ...................i................................................................................ 2300/4602
[00:49:06] .................................................................................................... 2400/4602
[00:49:10] .................................................................................................... 2500/4602
[00:49:13] ................................iiiiiiiii........................................................... 2600/4602
[00:49:20] .................................................................................................... 2800/4602
[00:49:24] .................................................................................................... 2900/4602
[00:49:26] ......................................................i............................................. 3000/4602
[00:49:29] .................................................................................................... 3100/4602
---
[00:52:52] .................................................................................................... 1500/2869
[00:53:04] .........................................i.......................................................... 1600/2869
[00:53:18] .................................................................................................... 1700/2869
[00:53:29] .................................................................................................... 1800/2869
[00:53:39] ................................................................F.i................................. 1900/2869
[00:54:11] .................................................................................................... 2100/2869
[00:54:17] ...................................................................................................i 2200/2869
[00:54:33] i.....................................................................i....i........................ 2300/2869
[00:54:47] .............i...................................................................................... 2400/2869
---
[00:55:52] 
[00:55:52] ------------------------------------------
[00:55:52] stderr:
[00:55:52] ------------------------------------------
[00:55:52] {"message":"couldn't read \"/checkout/src/test/run-pass/modules/mod_dir_simple/load_another_mod/test.rs\": No such file or directory (os error 2)","code":null,"level":"error","spans":[{"file_name":"/checkout/src/test/run-pass/modules/mod_dir_simple/load_another_mod.rs","byte_start":507,"byte_end":511,"line_start":13,"line_end":13,"column_start":9,"column_end":13,"is_primary":true,"text":[{"text":"pub mod test;","highlight_start":9,"highlight_end":13}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error: couldn't read \"/checkout/src/test/run-pass/modules/mod_dir_simple/load_another_mod/test.rs\": No such file or directory (os error 2)\n  --> /checkout/src/test/run-pass/modules/mod_dir_simple/load_another_mod.rs:13:9\n   |\nLL | pub mod test;\n   |         ^^^^\n\n"}
[00:55:52] {"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to previous error\n\n"}
[00:55:52] ------------------------------------------
[00:55:52] 
[00:55:52] thread '[run-pass] run-pass/modules/mod_dir_recursive.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3277:9
[00:55:52] note: Run with `RUST_BACKTRACE=1` for a backtrace.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@ehuss
Copy link
Contributor Author

ehuss commented Oct 16, 2018

Well bummer. Apparently non-mod-rs behavior was always possible with #[path] attributes, but the path is never included the path of the root module where the #[path] appears. That really throws a wrench into this fix. 😞

So, stable rust supported this:

main.rs:
    mod foo;
    fn main() {
        foo::inline::inline2::bar::f();
    }

foo.rs:
    pub mod inline {
        pub mod inline2 {
            #[path="baz.rs"]
            pub mod bar;
        }
    }

inline/inline2/baz.rs:
    pub fn f () {}

Notice the distinct lack of foo in the baz.rs path and no mod.rs files.

I can't think of a way, yet, to continue to support this while fixing the original issue. It's getting late here, so I'll need to pick it up tomorrow.

@ehuss
Copy link
Contributor Author

ehuss commented Oct 16, 2018

I've been trying to think of a solution to the above, but I'm running out of ideas. Some (bad) ideas:

  • Replace Directory::path with a stack that can filter out non-mod-rs ids when used with #[path] attributes in inline mods. This sounds awful.
  • Just make a breaking change to #[path] in inline modules in non-mod-rs files. There were no tests for it, and I don't see anyone doing that on crates.io.

And just to be clear, I would propose leaving original #[path] behavior for all other cases (does not include the id of a non-mod-rs module). This may be confusing, but I don't think it can be changed without breakage. I think the simplicity of module path = directory path is beneficial for the 99% of users who never use #[path].

Let me know if anyone has any ideas.

@pnkfelix
Copy link
Member

sorry for not getting around to revewing this yet

@pnkfelix
Copy link
Member

@ehuss I see that you've started comparing this PR to PR #55192. Are you okay with me closing this and letting the future activity be focused on #55192?

@ehuss
Copy link
Contributor Author

ehuss commented Oct 22, 2018

Yea, that's fine. However, some of the fixes here should not get lost.

@ehuss ehuss closed this Oct 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants