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

[RFC 2126] permit foo.rs or foo/mod.rs to support submodules like foo/bar.rs #45385

Closed
nikomatsakis opened this issue Oct 19, 2017 · 5 comments
Closed
Labels
E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 19, 2017

Ignoring "inline" modules like mod foo { }, modules currently come in one of two forms:

  • Leaf modules: a foo.rs file is forced to be a leaf module. In particular, one cannot make a submodule in a file like foo/bar.rs.
  • Directory modules: a foo/mod.rs file is a directory module. One can create submodules in files like foo/bar.rs.

As part of #44660, we plan to support either of those two forms. However, the two forms should not be usable for a single module simultaneously: in other words, you can have foo.rs or foo/mod.rs, but not both. (One is permitted to intermingle the two forms within a single project, however.)

Because the RFC has a number of moving parts, we wanted to feature-gate semi-independent features with distinct names. Therefore, foo.rs files that contain submodules can be gated under the feature name module_flexible_submodules.

(Note: Please limit discussion on this issue strictly to implementation concerns relative to this particular change. Policy discussion as to whether or not to make this change -- or whether to make other changes to the module system -- belong in #44660.)

@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-front labels Oct 19, 2017
@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Oct 19, 2017

Mentoring instructions

Currently, when we encounter a mod foo; statement, we compute the path where to find the module foo in the default_submod_path function. As you can see from inspecting the code, we already report an error if there is both a foo.rs and a foo/mod.rs file present.

The only place where foo.rs and foo/mod.rs sources are treated differently is with respect to their DirectoryOwnership setting. A foo.rs file is tagged as not "owning" its directory. Because of this setting, we will later report an error here if that submodule foo has its own mod bar; within.

Definition: we currently consider a mod.rs file to "own" its directory, same as a lib.rs or bin.rs file -- this basically means that it may declare new submodule files, which will be found within the same directory. The DirectoryOwnership enum is declared here.

This notion of ownership isn't quite what we want for foo.rs files -- they don't own the directory they are in, but they may own a corresponding subdirectory. Thus we have to tweak the setup. I am not 100% sure how to do this; we may want to modify the Owned variant of the DirectoryOwnership enum with a new field, that indicates we are in a foo.rs file. I imagine a field named something like relative_name: Option<ast::Ident>. ast::Ident is the internal type for an identifier: it's an interned string that can be cheaply copied around. We would set this value to Some(id) when we are parsing a non-mod.rs file and set it to None when we have a mod.rs file. We also want to use the same settings when handling explicit #[path] attributes, here.)

Once we have this new notion of directory ownership, we can use it when finding the default path where a mod bar would be located. The relevant code we need to alter is here -- right now, we invoke default_submod_path with the path from the directory field, which stores the location of the current file. I imagine we should also include the "directory ownership" of the current directory. In the case where the new id field is not None, we can construct a path like {directory}/{id}/bar.rs, where {id} is the value of the new id field.

(In the case of our example of foo.rs, {directory} would thus be the directory where foo.rs is found, and {id} would be foo. Thus is foo.rs contains a mod bar;, we would look in foo/bar.rs, as expected.)

The only other change that should be needed is to add a feature-gate somewhere. Ordinarily, I believe the feature-gate information is not available until after parsing -- but we can presumably remember somewhere that we encountered a submodule that would previous have been illegal (e.g., foo/bar.rs with a foo.rs parent). My recommendation would be to add a vector into the ParseSess struct. This vector would store the gated paths, and the span of the mod declaration where they were declared. Then, during the feature-gate checking, we can see if this vector is non-empty and use the gate_feature_post! macro to report an error if needed. An example invocation of the macro can be found here.

(To add a new feature-gate, you can follow the directions at the end of the "So You Want To Implement a Feature" page in the forge).)

@nikomatsakis nikomatsakis added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. and removed E-needs-mentor labels Oct 19, 2017
@slo1
Copy link
Contributor

slo1 commented Oct 21, 2017

Hi, I'm going to try to work on this.

@nikomatsakis
Copy link
Contributor Author

@slo1 great! Let me know how it goes. =) A word of warning, I do try to keep up with GH comments, but faster responses can be had by sending a ping on gitter.

@nikomatsakis
Copy link
Contributor Author

@slo1 just checking in -- everything ok?

@cramertj
Copy link
Member

I'm working on this.

bors added a commit that referenced this issue Dec 10, 2017
Implement non-mod.rs mod statements

Fixes #45385, cc #44660

This will fail tidy right now because it doesn't recognize my UI tests as feature-gate tests. However, I'm not sure if compile-fail will work out either because compile-fail usually requires there to be error patterns in the top-level file, which isn't possible with this feature. What's the recommended way to handle this?
bors added a commit that referenced this issue Dec 21, 2017
Implement non-mod.rs mod statements

Fixes #45385, cc #44660

This will fail tidy right now because it doesn't recognize my UI tests as feature-gate tests. However, I'm not sure if compile-fail will work out either because compile-fail usually requires there to be error patterns in the top-level file, which isn't possible with this feature. What's the recommended way to handle this?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. 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

3 participants