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

(Modules) Tracking issue for the mod.rs changes #53125

Closed
2 of 3 tasks
Centril opened this issue Aug 6, 2018 · 39 comments
Closed
2 of 3 tasks

(Modules) Tracking issue for the mod.rs changes #53125

Centril opened this issue Aug 6, 2018 · 39 comments
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Milestone

Comments

@Centril
Copy link
Contributor

Centril commented Aug 6, 2018

This is a sub-tracking issue for the RFC "Clarify and streamline paths and visibility" (rust-lang/rfcs#2126)
dealing with the mod.rs changes.

Unresolved questions:

None

Summary

A foo.rs and foo/ subdirectory may coexist; mod.rs is no longer needed when placing submodules in a subdirectory.

@Centril Centril added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Aug 6, 2018
@joshtriplett
Copy link
Member

Of the various module changes, this one seems quite clear, uncontroversial, and ready to commit to.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Aug 6, 2018

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 6, 2018
@Centril Centril added this to the Rust 2018 RC milestone Aug 6, 2018
@sanmai-NL
Copy link

sanmai-NL commented Aug 7, 2018

@joshtriplett: gave your post a thumbs down for the record, because you didn’t support your statement that it’s uncontroversial, while I can think of a few questions around this.

Multiple equivalent module configuration mechanisms

So both foo/mod.rs and foo.rs are going to be supported? I wouldn’t look forward to that. From a teachability perspective, the possibility of two ways to do the same thing even if one way is perhaps more straightforward is not an improvement. The newer, easier way will perhaps not be reflected in the ecosystem for some time to come so new programmers will not necessarily be inclined to use the newer, easier way.

Common submodules and module configuration

What is clearer:

A (now)

foo/mod.rs
foo/tests.rs

B (possibility after this change)

foo.rs
foo/tests.rs

A possible limitation here is that directories and files are often sorted separately in IDEs (e.g., IntelliJ IDEA) and filesystem explorers (e.g., Gnome Files) so there may be a large visual gap between foo.rs and its related foo/.

Non-code resources and module configuration

include_bytes! and include_str! look up resources relative to the current module’s directory. This means a mod.rs-like config would perhaps be clearer, similar to example B in my previous point.

@joshtriplett
Copy link
Member

joshtriplett commented Aug 7, 2018

@sanmai-NL I didn't intend to suggest that there was unanimity, just that unlike some of the other module changes, there hasn't been widespread objection or disagreement, such as in the various edition feedback threads. Addressing your points specifically:

Multiple equivalent module configuration mechanisms

The intention is to primarily point people to foo.rs. We want to keep foo/mod.rs for compatibility, so this change doesn't break existing projects. And we can provide appropriate lints (for people who want to write idiomatic Rust 2018 code) that encourage renaming the files accordingly, to help migrate the ecosystem. So I wouldn't expect this to bifurcate the ecosystem.

Common submodules and module configuration

Today, foo::bar could map to either foo/bar/mod.rs or foo/bar.rs depending on whether it has submodules. Similarly, foo::bar::baz maps to foo/bar/baz.rs or foo/bar/baz/mod.rs. That inconsistency makes it more difficult to look at a path in code and map it to the filesystem. So, I'd suggest that it's clearer to look at foo::bar and always know that you just mentally map each :: to / and append .rs.

Non-code resources and module configuration

Depends on your interpretation of where the module lives. While some other languages do have facilities similar to mod.rs, most tend to put code associated with a module named "foo" in a file foo.ext, and when that file includes something it would do so relative to the directory containing that file. This is a subjective difference.

More importantly, because of the inconsistency in the previous point, with today's system if you want to add a submodule to an existing module, you'd have to move the module to mod.rs, which would then change the location that include_bytes! and similar start from. With this change, you don't have to move the file to add submodules, so those paths always consistently start from the same place.

@rfcbot
Copy link

rfcbot commented Aug 10, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Aug 10, 2018
@Nemo157
Copy link
Member

Nemo157 commented Aug 15, 2018

I just want to report some experience trying this out. On a relatively small (4 relevant sub-modules) new crate for futures I used the new form exclusively. As a CLI only programmer I found the interference with tab completion to really break my workflow, enough so that I eventually gave up and reverted to the old form.

@Centril
Copy link
Contributor Author

Centril commented Aug 15, 2018

@Nemo157 Could you elaborate a bit on the problems you experienced with tab completion?

@Nemo157
Copy link
Member

Nemo157 commented Aug 15, 2018

Just that when I have a folder structure including both src/foo.rs and src/foo/bar.rs (and other sub-files) and I type src/f and tab complete it it now stops at the intermediate src/foo. I then need to disambiguate with either . or /b to select the correct file. Previously with src/foo/mod.rs this would go all the way to src/foo/ and I could then disambiguate with just the single characters m or b.

I’m sure the new scheme is something I could learn to deal with better, but after a few days of working on that crate I was still regularly accidentally opening the folder instead of the file I wanted.

@Centril
Copy link
Contributor Author

Centril commented Aug 15, 2018

@Nemo157 Thanks for the data point!

This does sound a bit concerning and I could see a lot of folks (myself included) being slowed down by this. I skimmed the RFC discussion and I couldn't find any mention of this issue, so it seems it wasn't addressed there.

On the other hand, having multiple files named mod.rs also is a poor experience in VSCode et. all already, at least for me.

If the strategy here was to deprecate mod.rs I would probably register a concern, but mod.rs isn't getting deprecated, so I don't feel strongly enough to do so. However, I would ask @joshtriplett or @aturon to address @Nemo157's concern.

@mark-i-m
Copy link
Member

I feel like tab completion is more important to me than the tab titles in vim in my workflow...

@petrochenkov
Copy link
Contributor

Counterpoint: In my workflow I very rarely have to type file paths manually (why would I do that?) but very often look at them in editor tabs.

@cramertj
Copy link
Member

Yeah, this seems like a very editor-dependent experience. It seems like, in this respect at least, the new style can make things easier for some tabbed-IDE users, while making things a bit harder for VIM (etc) folks.

@mark-i-m
Copy link
Member

mark-i-m commented Aug 15, 2018

That seems to suggest that we should keep both and make neither canonical.

@cramertj
Copy link
Member

@mark-i-m AFAIK the plan was always to keep both.

@letheed
Copy link
Contributor

letheed commented Aug 15, 2018

I'm not sure how much vim differs from emacs but the problem of having many mod.rs, as well as having to rename and move modules is as present in emacs as it is in vscode for me. I agree there can be some friction with path completion. It goes away when using project-wide find-file commands and/or fuzzy matching (like helm-projectile-find-file in spacemacs) though.

@sanmai-NL
Copy link

@mark-i-m, @cramertj: Strongly disagree! What about the added complexity of two ways to achieve the same thing? See #53125 (comment). Not that I care so much about how we configure module structure in Rust, but I do care about keeping the language as simple as possible. Having multiple module configuration mechanisms contravenes Rust’s strategic policy (e.g., https://blog.rust-lang.org/2017/03/02/lang-ergonomics.html):

The root issue is instead: how much information do you need to confidently understand what a particular line of code is doing, and how hard is that information to find? Let’s call this the reasoning footprint for a piece of code. The pitfalls above come from the reasoning footprint getting out of hand, rather than implicitness per se.

How to teach to novices: ‘oh, there a mod.rs there, yeah foo.rs here, but never mind ...’?

@sanmai-NL
Copy link

People are mentioning issues with various IDEs/editors, but we are barking up the wrong tree. The Rust refactoring feature set of these tools must be improved then. Not how Rust is designed.

@Nemo157
Copy link
Member

Nemo157 commented Aug 16, 2018

People are mentioning issues with various IDEs/editors, but we are barking up the wrong tree. The Rust refactoring feature set of these tools must be improved then. Not how Rust is designed.

I was very careful not to mention any editor, my issues were mainly around accessing the files directly from a shell (and an editor command buffer with very similar completion to mainstream shells). While it might be possible to integrate RLS into my shell to be able to open files via module-name completion or something, that's not something I'm planning on doing anytime soon.

@vi
Copy link
Contributor

vi commented Aug 16, 2018

What if deprecate mod.rs being the default filename for a module and instead allow to specify it explicitly like

#[path = "mymod/mod.rs"]
pub mod mymod;

if somebody wants to keep all files related to the module in one place?

@Centril
Copy link
Contributor Author

Centril commented Aug 16, 2018

I'm personally quite torn on what I'd use in my own projects given the choice of both.

On one hand, foo.rs would cause problems for me with tab completion when using git to add files to the staging area. On the other hand, I do use VSCode, and I get annoyed when I have two mod.rs files open and the tabs get all long.

My conclusion is that we shouldn't solve this globally for all Rust users because work flows are so different. The addition of foo.rs but non-deprecation of mod.rs seems to me the best choice here.
So let's stick with the plan. :)

@vi
Copy link
Contributor

vi commented Aug 16, 2018

Shall cargo fix migrate foo/mod.rs to foo.rs, gived there are no obstacles like already existing foo.rs or include_bytes!?

@SimonSapin
Copy link
Contributor

I think that foo/mod.rs is not necessarily broken, and so it should not be "fixed" at least by default. This could be an opt-in though.

@rfcbot
Copy link

rfcbot commented Aug 20, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 20, 2018
@Centril
Copy link
Contributor Author

Centril commented Aug 20, 2018

Given that the FCP is now complete; any takers for writing up a stabilization PR?

@sanmai-NL
Copy link

@Centril,
Maybe I’m naive about the process here. How come this has been accepted so quickly, right away before any comment was posted? The concern about teachability hasn’t been addressed by @joshtriplett. Maybe this can be pushed through like that, just want to ask what the process is supposed to be?

@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 22, 2018

Be it good or bad, the normal RFC process kinda goes out of the window with deadlines being close.
Edit: Oh, not so much in this particular case actually, I was talking more about #53130.

@Centril
Copy link
Contributor Author

Centril commented Aug 22, 2018

@sanmai-NL with respect to process, we are, unfortunately, under time constraints to ship the edition so we may sometimes speed up the process more so than normally done. However, this does not mean that decisions should be rushed just because of time constraints.

That said, this has already been extensively discussed on the RFC before it. The main decision to accept this was made there and then, this is simply confirming the decision as part of our double-checking and vetting process.

As for your concern about teachability, where you wrote:

So both foo/mod.rs and foo.rs are going to be supported? I wouldn’t look forward to that. From a teachability perspective, the possibility of two ways to do the same thing even if one way is perhaps more straightforward is not an improvement.

as well as:

What about the added complexity of two ways to achieve the same thing? See #53125 (comment). Not that I care so much about how we configure module structure in Rust, but I do care about keeping the language as simple as possible. Having multiple module configuration mechanisms contravenes Rust’s strategic policy (e.g., https://blog.rust-lang.org/2017/03/02/lang-ergonomics.html)

@joshtriplett said:

The intention is to primarily point people to foo.rs. We want to keep foo/mod.rs for compatibility, so this change doesn't break existing projects. And we can provide appropriate lints (for people who want to write idiomatic Rust 2018 code) that encourage renaming the files accordingly, to help migrate the ecosystem. So I wouldn't expect this to bifurcate the ecosystem.

I believe this addresses your point. At least it suffices for me.

I would also like to add that Rust's language team does not generally adhere to PEP 20 -- The Zen of Python:

There should be one-- and preferably only one --obvious way to do it.

We sometimes do adhere to this, we sometimes don't, depending on the situation. :)

@joshtriplett
Copy link
Member

This was proposed previously in an RFC, the RFC was accepted, the result was implemented, this change was widely discussed as part of the edition, and quite a bit of feedback already happened during that long process. The previous tracking issue incorporated plenty of discussion as well. No part of the process has been bypassed here; if anything, this change has had closer scrutiny and more extensive feedback than usual.

@sanmai-NL
Copy link

@Centril: as for PEP 20-like principles. It seems to me teachability and simplicity haven’t been prioritized in Rust. Please note: not being prioritized does not mean being ignored completely. I deeply regret that. Here is another case where with deliberation and specific attention to teachability and simplicity (two separate but related concerns) would makes us conclude rust-lang/rfcs#2126 is not ‘uncontroversial’ at all.

@sanmai-NL
Copy link

sanmai-NL commented Aug 23, 2018

@Centril, @joshtriplett: thanks, your explanations answer my question. Note that I didn’t claim parts of the process have been bypassed, I just observed how quickly things seemed to go when you do not know the process that it seems to have went through. @joshtriplett: In hindsight you entering the thread stating this ‘seems uncontroversial’ and ‘ready to commit to’, suggested this is not just about implementing what has been firmly decided.

@joshtriplett
Copy link
Member

@sanmai-NL Fair enough. In hindsight, I could have laid out some of the history more explicitly; in fact, I think it makes sense to edit in the explanations of that history near the top of the thread.

By "ready to commit to", I meant "ready to get on a path to stabilization as part of the edition".

@nikomatsakis
Copy link
Contributor

So, as discussed, this is ready for stabilization. This is a great candidate for first PRs! The stabilization guide page on forge gives instructions on how to proceed.

@nikomatsakis nikomatsakis added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Sep 1, 2018
@nikomatsakis
Copy link
Contributor

Also, we should touch base with the @rust-lang/docs team: what is needed around documentation here?

@GuillaumeGomez
Copy link
Member

Changes in the books and in the examples I assume. Bad luck that a few members of the docs team aren't available currently...

@blitzerr
Copy link
Contributor

blitzerr commented Sep 3, 2018

I will take this for my first PR

kennytm added a commit to kennytm/rust that referenced this issue Sep 12, 2018
Stabilization change for mod.rs

This change is in response to rust-lang#53125.
The patch makes the feature accepted and removes the tests that tested the
non-accepted status of the feature.
@Centril
Copy link
Contributor Author

Centril commented Sep 15, 2018

Triage: The only thing remaining now is to update the reference / documentation.

@steveklabnik
Copy link
Member

Done!

@steveklabnik
Copy link
Member

Oops, mis-read. This is ready to stabilize.

@Centril
Copy link
Contributor Author

Centril commented Sep 18, 2018

@steveklabnik I believe this was already stabilized in the compiler :)

Closing this issue; we can track the remaining work in the issues you've listed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. finished-final-comment-period The final comment period is finished for this PR / Issue. 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