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

Allow `foo.rs` to be parent to `foo/bar.rs` #39702

Closed

Conversation

Projects
None yet
7 participants
@withoutboats
Copy link
Contributor

withoutboats commented Feb 9, 2017

Prior to this commit, in order for a module to have submodules, it had
to be at the /foo/mod.rs filepath. After this commit, modules at the
/foo.rs filepath have submodules located in the /foo/ directory.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 9, 2017

r? @pnkfelix

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

@withoutboats

This comment has been minimized.

Copy link
Contributor Author

withoutboats commented Feb 9, 2017

Not sure if this needs an RFC or not.

As written this doesn't work (and doesn't even get past stage 1), because #[path] attributes need to search paths from the directory the file is in, but this causes them to search in a subdirectory.

For example, in src/librustc_borrowck/borrowck/move_data.rs:

#[path="fragments.rs"]
mod fragments;

Needs to search for that file at src/librustc_borrowck/borrowck/fragments.rs, but instead on this branch src/librustc_borrowck/borrowck/mock_data/fragments.rs is searched.

I'll keep iterating on it though.

EDIT: Fixed this.

@withoutboats

This comment has been minimized.

Copy link
Contributor Author

withoutboats commented Feb 9, 2017

Not sure if this needs an RFC or not.

Talked with the lang team, we agreed that this needs an RFC. I'll write one.

@withoutboats withoutboats force-pushed the withoutboats:more_flexible_mods branch 2 times, most recently from 88f82fa to 6b276c7 Feb 10, 2017

@withoutboats

This comment has been minimized.

Copy link
Contributor Author

withoutboats commented Feb 10, 2017

Question for the RFC - Consider this code:

// src/lib.rs
#[path="bar.rs"]
mod foo;
// src/bar.rs (the `foo` module)
mod baz;

Should the baz module be searched for at src/foo/baz.rs or src/bar/baz.rs?

My inclination is to search at src/bar/baz.rs because if you chose #[path="bar/mod.rs"] that is where it would search.

@jmesmon

This comment has been minimized.

Copy link
Contributor

jmesmon commented Feb 10, 2017

On the paths: unless there is an existing example otherwise, it probably makes sense to keep them as being relative to the directory containing the file which contains the #[path="..."] attribute. Otherwise thinking about path gets harder than it really needs to be.

@KalitaAlexey

This comment has been minimized.

Copy link
Contributor

KalitaAlexey commented Feb 10, 2017

I'd like to say against this feature because it adds complexity without any benefits.

@jmesmon

This comment has been minimized.

Copy link
Contributor

jmesmon commented Feb 10, 2017

I don't mind it so much: it could make moving from a single-file module (src/foo.rs) to a multi-file module (src/foo.rs + src/foo/blah.rs) not require a renaming of a file, which makes moving to multi-file modules easier (smaller diff & better vcs logs).

@withoutboats

This comment has been minimized.

Copy link
Contributor Author

withoutboats commented Feb 10, 2017

On the paths: unless there is an existing example otherwise, it probably makes sense to keep them as being relative to the directory containing the file which contains the #[path="..."] attribute. Otherwise thinking about path gets harder than it really needs to be.

The way the path attribute is parsed can't change. The question is whether path'd modules have submodules at the subdirectory matching their module name or their path name.

@jmesmon

This comment has been minimized.

Copy link
Contributor

jmesmon commented Feb 10, 2017

Ah yes, I see what you were getting at. In that case, your suggestion sounds reasonable.

Allow `foo.rs` to be parent to `foo/bar.rs`
Prior to this commit, in order for a module to have submodules, it had
to be at the `/foo/mod.rs` filepath. After this commit, moudles at the
`/foo.rs` filepath have submodules located in the `/foo/` directory.

@withoutboats withoutboats force-pushed the withoutboats:more_flexible_mods branch from 6b276c7 to de2070b Feb 11, 2017

@withoutboats

This comment has been minimized.

Copy link
Contributor Author

withoutboats commented Feb 11, 2017

After considering all the cases, what this implements (and the RFC will propose) is that a path attribute will contain submodules using these rules:

  • foo/mod.rs will have submodules in foo/
  • foo/bar.rs will have submodules in bar if "bar.rs" is valid unicode matching the regex .+\.rs
  • Any other file name (not ending in .rs, not valid unicode, just .rs with no prefix) cannot have submodules, just as non-mod.rs modules cannot have today.

Again this only applies to modules at path attribute locations; I would be surprised if any Rust code in the wild has a module falling under the third category.

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Feb 11, 2017

Question for the RFC - Consider this code:

Take arbitrary stance in the RFC and pose this question in the RFC’s unresolved questions section.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 21, 2017

☔️ The latest upstream changes (presumably #39765) made this pull request unmergeable. Please resolve the merge conflicts.

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Mar 6, 2017

@withoutboats any update on the status of the RFC being drafted? Should I close this PR in the meantime?

@withoutboats

This comment has been minimized.

Copy link
Contributor Author

withoutboats commented Mar 6, 2017

Yea I'll close this for now. Hope to get back to the RFC in the next few weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.