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

Should modules with path attribute become directory owners automatically? #32401

Closed
matklad opened this issue Mar 21, 2016 · 13 comments
Closed
Assignees
Labels
A-parser Area: The parsing of Rust source code to an AST. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. P-medium Medium priority T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@matklad
Copy link
Member

matklad commented Mar 21, 2016

Consider this layout

// main.rs
mod foo;
fn main() {}

// foo.rs
mod bar;

// bar.rs
// empty

It generates cannot declare a new module at this location error for mod bar;, which is good.

However, If I explicitly specify #[path = ] for mod foo; it compiles:

// main.rs
#[path = "foo.rs"]
mod foo;
fn main() {}

// foo.rs
mod bar;

// bar.rs
// empty

I don't know if this is expected behavior or not, but it looks suspicious to me .

@pnkfelix
Copy link
Member

No matter what we decide here, I think its important that the following variation continue to work:

// main.rs
#[path = "foo.rs"]
mod foo;
fn main() {}

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

// bar.rs
// empty

That is, if the developer cares enough about their existing directory layout that they are willing to add #[path] attributes even for modules that are merely referenced from non-standard places, then we should support that.


(I don't have a strong opinion about disallowing the second form in the issue description.)

@pnkfelix pnkfelix added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Mar 21, 2016
@pnkfelix
Copy link
Member

triage: I-nominated

@KalitaAlexey
Copy link
Contributor

Why #[path] is allowed?
https://github.com/rust-lang/rust/blob/master/src/doc/style/features/modules.md suggests to not use #[path].

@nikomatsakis
Copy link
Contributor

I think the answer is that we should not accept this. If a module has a path attribute, it should be a directory owner if the path is a mod.rs path (e.g. foo/mod.rs), but not otherwise.

@nikomatsakis
Copy link
Contributor

Discussed in @rust-lang/lang. Note that if we stop allowing out-of-line submodules from arbitrary named files, a warning period would be required.

@nikomatsakis
Copy link
Contributor

triage: P-medium

This doesn't feel super urgent. If we end up keeping the current behavior, we can live with it.

@rust-highfive rust-highfive added P-medium Medium priority and removed I-nominated labels Mar 24, 2016
@Aatch
Copy link
Contributor

Aatch commented Mar 26, 2016

@KalitaAlexey while it should probably be avoided if possible, #[path] is useful and shouldn't be removed (not that we can remove it at this point anyway).

@KalitaAlexey
Copy link
Contributor

@Aatch Could you please describe situation when #[path] is single way?

@matklad
Copy link
Member Author

matklad commented Mar 26, 2016

@KalitaAlexey for example stdlib uses path in conjunction with cfg attributes to select among several implementations: https://github.com/rust-lang/rust/blob/master/src/libstd/lib.rs#L439-L443

@brson brson added I-needs-decision Issue: In need of a decision. A-parser Area: The parsing of Rust source code to an AST. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. labels Nov 3, 2016
@brson
Copy link
Contributor

brson commented Nov 3, 2016

Help wanted. Just change the parser to disallow the behavior in the OP, add a compile-fail test. We will need to run it through crater to see if anybody depends on this, and maybe have a deprecation period. If people do depend on it, let's just add a test to the test suite and call it expected behavior.

@brson
Copy link
Contributor

brson commented Nov 3, 2016

@jseyfried nice papercut

@brson brson added I-papercut and removed I-needs-decision Issue: In need of a decision. labels Nov 3, 2016
@jseyfried jseyfried self-assigned this Nov 3, 2016
@jseyfried
Copy link
Contributor

Fixed in #37602.

bors added a commit that referenced this issue Nov 22, 2016
parser: simplify directory ownership semantics

This PR simplifies the semantics of "directory ownership". After this PR,
 - a non-inline module without a `#[path]` attribute (e.g. `mod foo;`) is allowed iff its parent module/block (whichever is nearer) is a directory owner,
 - an non-inline module is a directory owner iff its corresponding file is named `mod.rs` (c.f. [comment](#32401 (comment))),
 - a block is never a directory owner (c.f. #31534), and
 - an inline module is a directory owner iff either
   - its parent module/block is a directory owner (again, c.f. #31534), or
   - it has a `#[path]` attribute (c.f. #36789).

These semantics differ from today's in three orthogonal ways:
 - `#[path = "foo.rs"] mod foo;` is no longer a directory owner. This is a [breaking-change].
 - #36789 is generalized to apply to modules that are not directory owners in addition to blocks.
 - A macro-expanded non-inline module is only allowed where an ordinary non-inline module would be allowed. Today, we incorrectly allow macro-expanded non-inline modules in modules that are not directory owners (but not in blocks). This is a [breaking-change].

Fixes #32401.
r? @nikomatsakis
@briansmith
Copy link
Contributor

I think the answer is that we should not accept this. If a module has a path attribute, it should be a directory owner if the path is a mod.rs path (e.g. foo/mod.rs), but not otherwise.

Now how do you create a module in a file that isn't named mod.rs that is a directory owner?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parser Area: The parsing of Rust source code to an AST. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. P-medium Medium priority T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants