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

Non-inline module is allowed inside other items #29765

Closed
huonw opened this Issue Nov 11, 2015 · 11 comments

Comments

Projects
None yet
5 participants
@huonw
Copy link
Member

huonw commented Nov 11, 2015

These are all legal, and somewhat strange, since they're not really following the file-system analogy that modules are typically explained with. I expect this behaviour is just whatever happens to fall out of the current implementation strategy, so let's make a more explicit decision one way or another.

static X: u8 = { mod foo; 0 };
fn foo() {
    mod foo;
}
fn main() {
    mod foo;
}

As one might expect, each of these is a distinct instance of foo (which can be verified by placing log_syntax(hello) in foo.rs: the compiler will greet you 3 times).

Noticed in https://users.rust-lang.org/t/are-module-declarations-allowed-inside-functions/3583 .

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Nov 12, 2015

This seems weird to me. I think the intention is that the out-of-line syntax (mod foo;) should only work at the top-level.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Nov 12, 2015

I'd vote for allowing non-inline modules only at the top-level, to retain a clear mapping to the file system.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Nov 12, 2015

(Or nested inside modules.)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Nov 12, 2015

triage: P-medium

Backwards incompatible, but ... kind of a curiousity.

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Nov 14, 2015

Found one more mod aliasing scenario. It involves several files, and is semi officially allowed by the reference.

The layout

src/
├─ bar.rs
├─ foo.rs
├─ inner
│  └── copy.rs
└─ main.rs

The files

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

//foo.rs
mod inner { mod copy; }

//bar.rs
mod inner { mod copy; }

The copy module will be aliased. The problem is that although foo and bar are not directory owners, their inner modules are.

matklad added a commit to intellij-rust/intellij-rust that referenced this issue Nov 14, 2015

(RES): implement child modules resolve
When we see `mod foo;` we should try to find a file corresponding to
`foo`. It is possible only if a parent module is a directory owner.

A module is a directory owner if it is either
  * a crate root
  * a nested inline module
  * called `mod.rs`

It is an error to have `mod foo;` in a non directory owning module.

The file name for `foo` is (modulo #[path = ] attribute) a directory
name of the parent module plus either `/foo.rs` or `/foo/mod.rs`.

Example 1

```Rust
// file src/main.rs, a crate root

mod foo; // src/foo.rs

mod inner {
    mod foo; // src/inner/foo.rs
}
```

Example 2

```Rust
// file src/foo.rs

mod bar; // forbidden

mod inner {
    mod foo; // src/inner/foo.rs
}
```

The semantics of `mod foo;` nested inside a function is not fully
specified at the moment: rust-lang/rust#29765

matklad added a commit to intellij-rust/intellij-rust that referenced this issue Nov 15, 2015

(RES): implement child modules resolve
When we see `mod foo;` we should try to find a file corresponding to
`foo`. It is possible only if a parent module is a directory owner.

A module is a directory owner if it is either
  * a crate root
  * a nested inline module
  * called `mod.rs`

It is an error to have `mod foo;` in a non directory owning module.

The file name for `foo` is (modulo #[path = ] attribute) a directory
name of the parent module plus either `/foo.rs` or `/foo/mod.rs`.

Example 1

```Rust
// file src/main.rs, a crate root

mod foo; // src/foo.rs

mod inner {
    mod foo; // src/inner/foo.rs
}
```

Example 2

```Rust
// file src/foo.rs

mod bar; // forbidden

mod inner {
    mod foo; // src/inner/foo.rs
}
```

The semantics of `mod foo;` nested inside a function is not fully
specified at the moment: rust-lang/rust#29765

matklad added a commit to intellij-rust/intellij-rust that referenced this issue Nov 16, 2015

(RES): implement child modules resolve
When we see `mod foo;` we should try to find a file corresponding to
`foo`. It is possible only if a parent module is a directory owner.

A module is a directory owner if it is either
  * a crate root
  * a nested inline module
  * called `mod.rs`

It is an error to have `mod foo;` in a non directory owning module.

The file name for `foo` is (modulo #[path = ] attribute) a directory
name of the parent module plus either `/foo.rs` or `/foo/mod.rs`.

Example 1

```Rust
// file src/main.rs, a crate root

mod foo; // src/foo.rs

mod inner {
    mod foo; // src/inner/foo.rs
}
```

Example 2

```Rust
// file src/foo.rs

mod bar; // forbidden

mod inner {
    mod foo; // src/inner/foo.rs
}
```

The semantics of `mod foo;` nested inside a function is not fully
specified at the moment: rust-lang/rust#29765
@matklad

This comment has been minimized.

Copy link
Member

matklad commented Nov 16, 2015

Upon reflecting more about this, I think that there are two questions here.

  • should non inline modules be allowed (syntactically) inside items?
  • should module aliasing (the same file representing two modules in the crate) be allowed?

The questions are independent because you can use #[path = ] attribute to create module aliasing manually.

I would prefer if the answer to the second question is not. I'm contributing to the Rust plugin for IntelliJ IDEA, and one of the tasks there is mapping files to modules (for example, to go to the parent module). I would much prefer just to highlight module aliasing as an error, rather then to deal with non one-to-one mapping between files and modules.

As a side note, if a user does want aliasing for some reason, she can always use symbolic link for the same effect.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Nov 16, 2015

@matklad

I tend to believe it is fine to have aliasing if you write the #[path] yourself (you asked for it...), and that it would be ok to put a non-inline module inside an non-module, if you specify the path yourself, but that we should error out otherwise. However, there has definitely been confusion in the past about the role of mod as a declaration and not a use, and disallowing aliasing would prevent that confusion from leading to incorrect trees.

Your example however is disconcerting, and suggests that perhaps the code has a slightly different model in mind than I do in my head. :) That is, I strongly expect mod inner { mod copy; } to (by default) be found at foo/inner/copy. What happens if you don't have the mod inner, but just place mod copy in foo?

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Nov 16, 2015

What happens if you don't have the mod inner, but just place mod copy in foo?

It is forbidden because foo is not a directory owner.

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Nov 16, 2015

Just in case here is a repository with the example: https://github.com/matklad/nested-mod

I tend to believe it is fine to have aliasing if you write the #[path] yourself (you asked for it...)

I don't see a reason to allow user to shoot himself in the leg. What if you accidentelly have identical path attributes (copy paste problem)?

My main concern though is that non injective file to module mapping may complicate the implementation of some rust tools. Here is another hypothetical example of this. Suppose you implement incremental compilation for rustc. You will need a mapping from files to modules. If mod aliasing is alowed, then you will need a multimapping. So you end up with more complex code which is used only in a tiny fraction of projects and because of this probably contains some bugs.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 1, 2015

@matklad

It is forbidden because foo is not a directory owner.

Ah, I see. So it seems to me that the problem is that mod inner { mod copy; } should not be allowed in contexts where mod copy; would be illegal. In other words, an inline module should only be considered a "directory owner" if it is located within a directory owner.

I don't see a reason to allow user to shoot himself in the leg. What if you accidentelly have identical path attributes (copy paste problem)?

A compromise might be Yet Another Lint Warning. ;) I agree with you about preventing people from shooting themselves in the leg, but I guess I think people don't use #[path] lightly or frequently, so this seems unlikely to be a major source of confusion.

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Dec 1, 2015

so this seems unlikely to be a major source of confusion.

I totally agree that this curiosity is insignificant for users. But I still worry more about the implementation side of the issue, and a lint warning does not help here.

jseyfried added a commit to jseyfried/rust that referenced this issue Feb 10, 2016

jseyfried added a commit to jseyfried/rust that referenced this issue Feb 10, 2016

bors added a commit that referenced this issue Feb 16, 2016

Auto merge of #31534 - jseyfried:restrict_noninline_mod, r=nikomatsakis
This PR disallows non-inline modules without path annotations that are either in a block or in an inline module whose containing file is not a directory owner (fixes #29765).
This is a [breaking-change].
r? @nikomatsakis

bors added a commit that referenced this issue Feb 16, 2016

Auto merge of #31534 - jseyfried:restrict_noninline_mod, r=nikomatsakis
This PR disallows non-inline modules without path annotations that are either in a block or in an inline module whose containing file is not a directory owner (fixes #29765).
This is a [breaking-change].
r? @nikomatsakis

@bors bors closed this in #31534 Feb 16, 2016

steveklabnik added a commit to steveklabnik/rust that referenced this issue Apr 14, 2016

Rollup merge of rust-lang#32885 - rkjnsn:patch-2, r=alexcrichton
Fix conflicting link identifiers

Caused "Errors for non-exhaustive match patterns now list up to 3 missing variants while also indicating the total number of missing variants if more than 3." to link to "libsyntax: Restrict where non-inline modules can appear (fixes rust-lang#29765)"

steveklabnik added a commit to steveklabnik/rust that referenced this issue Apr 14, 2016

Rollup merge of rust-lang#32885 - rkjnsn:patch-2, r=alexcrichton
Fix conflicting link identifiers

Caused "Errors for non-exhaustive match patterns now list up to 3 missing variants while also indicating the total number of missing variants if more than 3." to link to "libsyntax: Restrict where non-inline modules can appear (fixes rust-lang#29765)"

bors added a commit that referenced this issue Apr 14, 2016

Auto merge of #32885 - rkjnsn:patch-2, r=alexcrichton
Fix conflicting link identifiers

Caused "Errors for non-exhaustive match patterns now list up to 3 missing variants while also indicating the total number of missing variants if more than 3." to link to "libsyntax: Restrict where non-inline modules can appear (fixes #29765)"
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.