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

path attribute on modules is useless in nested modules #35016

Open
jethrogb opened this issue Jul 24, 2016 · 15 comments
Open

path attribute on modules is useless in nested modules #35016

jethrogb opened this issue Jul 24, 2016 · 15 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jethrogb
Copy link
Contributor

My crate has some test support data in tests/support/mod.rs. When trying to load this module for unit tests like this:

mod tests {
    #[path="../tests/support/mod.rs"]
    mod support;
}

I get an error like this:

error: couldn't read "src/tests/../tests/support/mod.rs": No such file or directory (os error 2)

In fact, specifying pretty much any path is going to fail, because the src/tests/ directory does not exist, because I'm using a nested module and not a separate file. When using nested modules, I think the path at which relative path resolution should start should be the directory that the actual source file is in.

@Aatch
Copy link
Contributor

Aatch commented Jul 25, 2016

@jethrogb #[path="../../tests/support/mod.rs"] should work.

I guess the issue is that both behaviours are reasonable, it depends on your mental model of how rustc finds module files. The thing is, the current behaviour is, well, what we currently have, meaning that changing it is technically a breaking change. It's also not obvious to me that "relative to the current file" is strictly better than what we have now, or just "different".

@jethrogb
Copy link
Contributor Author

It's also not obvious to me that "relative to the current file" is strictly better than what we have now, or just "different".

Given that one option results in unresolvable paths, I'd say the other option is better. I think it could be non-breaking by testing whether the current way works, and if it doesn't work try the other way.

@jethrogb
Copy link
Contributor Author

Another option is to let rustc, instead of the OS, resolve .., but doing path resolution in userspace is error-prone.

@jseyfried
Copy link
Contributor

The following works for this use case:

#[path = "../tests"]
mod tests {
    #[path = "support/mod.rs"]
    mod support; // expects "../tests/support/mod.rs" (relative to source file directory)

    // --- or just ---

    mod support; // expects "../tests/support/mod.rs" or "../tests/support.rs"
}

If we were designing from scratch, I would probably prefer to have all paths be relative to the source file's directory.

This example is reasonable enough; the paths are relative to the current module's directory, i.e. the directory in which mod foo; would expect to find foo.rs, as opposed to the current file's directory.

However, when mod foo; isn't allowed (c.f. directory ownership restrictions), this definition doesn't make sense and the directory to which the path is relative feels arbitrary, a violation of the spirit of those restrictions.

That being said, I think the status quo here is acceptable enough that falling back to file-relative paths isn't worth the extra language complexity.

@jseyfried jseyfried added A-diagnostics Area: Messages for errors, warnings, and lints E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Feb 8, 2017
@jseyfried
Copy link
Contributor

This issue is common enough that I think it would be worth checking the file-relative path before emitting the "no such file" error. If the path is file-relative, we would give an informative error message, ideally with an error code that can explains #[path] on inline modules (mod foo { ... }).

cc @GuillaumeGomez @jonathandturner

@jseyfried jseyfried added P-low Low priority and removed A-lang labels Feb 8, 2017
@GuillaumeGomez
Copy link
Member

I'm not sure that adding a logic in such a case is a good idea. The error seems pretty explicit for me. However, we could still try to resolve the path instead of having this ".." in the middle.

@jseyfried
Copy link
Contributor

@GuillaumeGomez

However, we could still try to resolve the path instead of having this ".." in the middle.

I'm wary of allowing more paths than we do today, but perhaps we could check if the user is trying to .. their way out of an inline module without a corresponding directory (like the example from #35016 (comment)) and emit the specialized diagnostics then. The idea is to make #[path] on inline modules more discoverable here.

@GuillaumeGomez
Copy link
Member

I'm feeling uneasy with having "hidden" logic in here. It's possible that the user wants the ./something/file.rs and that we deduce that it's some other path. It'd be quite a burden.

@GuillaumeGomez
Copy link
Member

So after explanations, it's not about adding a new mechanism but clarifying the error. Therefore, I completely agree! 👍

@GuillaumeGomez GuillaumeGomez added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Feb 8, 2017
@GuillaumeGomez GuillaumeGomez self-assigned this Feb 8, 2017
@steveklabnik steveklabnik removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 9, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 25, 2017
@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 31, 2018

Any progress here? I am getting this error, and have been trying to work around it for 10 minutes after reading all the comments without any luck.

I don't understand how any workaround for this could even work. AFAICT the problem is that the relative path specified in #[path = ...] is appended to an invalid path, resulting in an invalid path that cannot be resolved. This happens because the path for nested modules contains a directory that does not exist.

Given two files:

  • src/lib.rs
  • src/bar.rs

and writing the following in lib.rs:

mod foo {
    #[path = "bar.rs"]
    mod bar;
}

will try to find bar.rs at src/foo/bar.rs. If I write

mod foo {
    #[path = "../bar.rs"]
    mod bar;
}

instead, rustc will try to find bar.rs at src/foo/../bar.rs.

Note how both paths contain src/foo in it, which is a directory that does not exist. That makes the path invalid (pointing to a non-existing file) in all architectures I've tried (MacOSX and Linux). Is there an X such that:

mod foo {
    #[path = "X/bar.rs"]
    mod bar;
}

will find bar.rs in the previously given directory structure?

If the answer is yes, then this is an error message bug, and the error message should suggest the appropriate path to use.

If the answer is no, this is either a bug in #[path], or a feature-request. Without this feature, #[path]'s doesn't appear to be usable in nested modules. Did the RFC for #[path] if there was ever one, consider this interaction? If not, how does the RFC for modules in Rust 2018 handle it?

@josephlr
Copy link
Contributor

I've run into the problem @gnzlbg described in #35016 (comment) both in once_cell and getrandom. A minimal implementation in rustc could just call Path::canonicalize before attempting to read the file. This would allow ../foo.rs to work as a #[path].

@GuillaumeGomez GuillaumeGomez removed their assignment Apr 23, 2020
@koivunej
Copy link

koivunej commented Apr 5, 2021

Found this issue from a tail end of E-mentor issues, seems like something I could help solve. As far as I understand the situation is illustrated by the original issue description, a file ends up being opened at a non-canonicalized path showing an error like:

error: couldn't read src/tests/../tests/support/mod.rs: No such file or directory (os error 2)
 --> src/main.rs:7:5
  |
7 |     mod support;

I looked into introducing Path::canonicalize at the current implementation. I think that would always cause a readlink or equivalent which might have some TOCTOU issues. TOCTOU because it'd seem that there is currently only an fs::read_to_string call to load the file. The required canonicalization could be done manually at the callsite but the issue of complicating the overall logic is still there. "Done manually" as in pop elements from dir_path for each leading Component::ParentDir in rustc_expand::module::mod_file_path. Not sure if this could be a pandoras box leading to /etc/passwd inclusion somehow.

Regarding the "more original" better error fix the originally suggested hint of using two ../../ to escape the implicit subdirectory given for outer module (tests in the issue description). This doesn't at least (anymore?) work to escape it since #44660. However, looking at the RFC#2126 directly it would seem that it didn't really mean to touch this part of the modules.

Another point raised in the error discussion about generally not being able to escape inline modules with #[path] so using one should just be an error. Currently this case is not covered in the reference, so it would have to be added there as well if not supported. Reference does explain #[path] in the context of outer inline module having #[path]. I'm just starting out the rustc journey, didn't yet look how I could create such "Inline modules cannot be escaped with upward relative #[path] inclusion" error.

Would like to work on getting this fixed, one way or the another but there's a decision to be made.

@koivunej
Copy link

koivunej commented Apr 6, 2021

Continued exploration on this.

It would seem that #[path = "/tmp/this_is_ok.rs"] works at the moment. So would the adding of error be to only give a better error to when escaping inline modules with all or non-navigable relative paths?

Path::canonicalize

This wouldn't work without removing the current behaviour of giving inline modules a directory path. As in the modified original example of:

// in src/lib.rs

mod this_is_inline {
  #[path = "../support.rs"]
  mod support;
}

Here a direct application of std::fs::canonicalize would end up trying to canonicalize src/this_is_inline/../support.rs, which in turn should fail if the src/this_is_inline didn't exist as a directory or symlink to directory (at least on Linux). The real "normalization" or "soft_canonicalization" fix would be to do what I already called "done manually at the callsite", which would allow:

  • assert_eq!(soft_canonicalize("src/this_is_inline", "../support.rs"), "src/support.rs")
  • continue supporting currently supported cases (absolute paths)
  • avoid adding any syscalls before std::fs::read_to_string

@oli-obk oli-obk removed the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Jul 4, 2022
@oli-obk oli-obk added the E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label Jul 4, 2022
@eminence
Copy link
Contributor

eminence commented Nov 4, 2022

Another wrinkle is that the current behavior (as of rust 1.65 today) is not consistent between platforms.

Given this code:

mod foo {
    #[path = "../bar.rs"]
    mod bar;
}

This will work on Windows, but not Linux

@nbaum
Copy link

nbaum commented Mar 17, 2023

If you're looking for a solution, this may suffice:

// In, say, mod.rs
#[path="."]
mod foo {
  #[path="."]
  mod bar {
    mod baz;
  }
}

This will load baz.rs from alongside mod.rs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-low Low priority 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