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

Fix ordering of nested modules in non-mod.rs mods #55192

Merged
merged 1 commit into from
Oct 28, 2018

Conversation

cramertj
Copy link
Member

Flatten relative offset into directory path before adding inline
(mod x { ... }) module names to the current directory path.

Fix #55094

Flatten relative offset into directory path before adding inline
(mod x { ... }) module names to the current directory path.

Fix rust-lang#55094
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 19, 2018
@cramertj cramertj added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 19, 2018
@cramertj
Copy link
Member Author

Beta-nominating as this changes the behavior of functionality that would become stable in the current beta.

@Mark-Simulacrum
Copy link
Member

@bors p=10 coming up on release week

@ehuss
Copy link
Contributor

ehuss commented Oct 19, 2018

Sorry for being pestering, but I feel like I'm not communicating well, since I proposed this exact change. Is it OK that this breaks behavior for #[path] attributes in inline modules?

@cramertj
Copy link
Member Author

@ehuss Ah, my apologies! I didn't realize there was already a change open for this. Yeah, it does affect the behavior of #[path], which is visible on stable, but only in the case of #[path] inside nested modules in a non-mod.rs file. We could separately track the file ordering for inline mod files for use in #[path] separately from uses in nested mod foo; declarations, which is sort of annoying, but I don't have other great ideas.

@ehuss
Copy link
Contributor

ehuss commented Oct 19, 2018

If you want to, you can grab the fixes for the non-mod-rs tests from my PR. In-tree tests are pretty broken right now. src/test/run-pass/non_modrs_mods/non_modrs_mods.rs got accidentally deleted during stabilization, so all tests are currently disabled. And the entire src/test/ui/non_modrs_mods tree can be deleted, it was only for feature gating, and was not deleted properly. There are a few other fixes in my PR, but they are not immediately important.

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 19, 2018

I can almost guarantee that all changes done in this area after #37602 were subtly buggy, and you can't fix any bug in directory ownership / non-inline modules without introducing new bugs, unless you have detailed understanding of the whole picture.

@cramertj and @ehuss
Do you have detailed understanding of the whole picture?
What is directory ownership, which modules own directories and which don't and why, how absolute path to any non-inline module is determined.
(I don't, so I don't understand full implications of this change and can't review it in isolation.)

@petrochenkov
Copy link
Contributor

What is directory ownership, which modules own directories and which don't and why, how absolute path to any non-inline module is determined.

Also, precise spec with motivational notes would be greatly helpful for this part of the language and compiler.

@cramertj
Copy link
Member Author

cramertj commented Oct 19, 2018

@petrochenkov So, several of the rules laid out in #37602 are no longer true, especially since #47298. I'll try and lay out the current rules, and what we want to change. I'm not going to use the phrase "directory owner" here, since as we've discovered, we might want the semantics for what directory is "owned" by the current module to be different based on whether the ownership is being used to determine what location #[path] is relevant to, vs. determining what location mod x; is relevant to. Instead, I'll refer to modules as "path-owning" and "mod-owning" a directory.

Some definitions:

  • module X path-owns directory Y: if #[path = "z.rs"] appears in X, it is valid and corresponds to Y/z.rs
  • module X mod-owns directory Y: if mod z; appears in X, it is valid and corresponds to Y/z.rs
  • out-of-line module: top-level module of a file, either crate root file or included via mod x; or #[path = ...] mod x;.
  • inline module: module that appears directly in the source of a file, e.g. mod x { ... }

The rules as currently implemented (post non-mod.rs mods stabilization):

  • an out-of-line module without a #[path] attribute (e.g. mod foo;) is allowed iff its parent module/block (whichever is nearer) mod-owns a directory
  • an out-of-line module always mod-owns a directory. It owns the current directory if the file name is "mod.rs" or if it was included via a #[path = ...] invocation, or if it is the crate root. If neither of these are true, it mod-owns the neighboring subdirectory whose name matches the mod <...>; that caused the file to be included (should be the same as the name of the file, <...>.rs).
  • an inline module path-owns the directory of the current file followed by an offset for each nested inline module. For example, mod y { mod z { ... } } in /x/foo.rs path-owns /x/y/z. This is visible from stable and cannot be changed. This is somewhat in contradiction to the desired path-owning behavior that one might want given non-mod.rs modes, where mod y { mod z { ... } } inside /x/foo.rs would path and mod-own /x/foo/y/z. The currently-implemented mod-ownership for these nested modules inside non-mod.rs/crate-root files is even messier, since it tries to append the name of the current out-of-line mod to the current path-owned directory, making mod y { mod z { ... } } in /x.foo.rs mod-own x/y/z/foo, which is unintuitive to absolutely everyone.

I see two options to fixing this:

  • Make nested modules such as mod y { mod z { ... } } in /x/foo.rs path-own /x/y/z (as they do today, we can't break this) and mod-own x/foo/y/z (as makes sense given the typical behavior of non-mod.rs mods). Complicates the implementation somewhat since two directory paths have to be tracked when calculating the ownership of inline modules.
  • Make these nested modules mod-own x/y/z just as they path-own it (basically, forget the foo). This is unintuitive given the normal behavior of non-mod.rs mods which offsets mod-ownership by the name of the current module, but matches the path ownership. This make the ownership behavior here more uniform and easier to implement, but introduces conflicts in the case of e.g. mod x; mod y; in /lib.rs, mod y { mod z; } in /x.rs (which would reference /y/z.rs) and mod z; in /y/mod.rs (which would also reference /y/z.rs). It also adds strange behavior (file path doesn't match mod ownership lineup) to the nested-mods-in-non-mod.rs case, rather than just relegating it to usages of the more obscure #[path=...] syntax.

I think the first option is probably the best, but it's the one with the most complication and the least consistency between the behavior of #[path] and mod.

TL;DR: mod y { #[path = "z.rs" } in x.rs references y/z.rs (no x). The missing x means this doesn't match with the normal behavior of non-mod.rs mod-owners.

Edit: there's also the third option of breaking the current path behavior inside nested modules, which is what this PR currently implements. I think this is probably too big of a breaking change, but it might be worth a crater.

@petrochenkov
Copy link
Contributor

@bors try
(For crater.)

@bors
Copy link
Contributor

bors commented Oct 20, 2018

⌛ Trying commit ca35ca8 with merge 19f01b9...

bors added a commit that referenced this pull request Oct 20, 2018
Fix ordering of nested modules in non-mod.rs mods

Flatten relative offset into directory path before adding inline
(mod x { ... }) module names to the current directory path.

Fix #55094
@petrochenkov petrochenkov added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 20, 2018
@petrochenkov
Copy link
Contributor

The "path-owning" vs "mod-owning" separations seems strange to me, unless we have to introduce it to explain the existing implementation.
It would be great to specify everything in terms of #[path = "..."] mod name;, and then say that mod name; is simply a sugar for #[path = "name.rs"] mod name;.

I'll re-read #55192 (comment) again tomorrow because right now I seem to forget previous paragraphs faster than I read the new ones.

@bors
Copy link
Contributor

bors commented Oct 21, 2018

☀️ Test successful - status-travis
State: approved= try=True

@petrochenkov
Copy link
Contributor

@craterbot run start=master#155510e377ae2a8d8ee0dad1a5f809c9062a5526 end=try#ca35ca839582d74f045a4a1eb9e1a82e29f0e032 mode=check-only

@craterbot
Copy link
Collaborator

👌 Experiment pr-55192 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🚧 Experiment pr-55192 is now running on agent aws-1.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@pietroalbini
Copy link
Member

cc @rust-lang/release and @rust-lang/compiler for the backport to beta this close to the stable release

@craterbot
Copy link
Collaborator

🎉 Experiment pr-55192 is completed!
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 22, 2018
@pietroalbini
Copy link
Member

Wut, Crater broke itself again... :(

@Mark-Simulacrum
Copy link
Member

This needs to land by Wednesday morning at the absolute latest if it is to make it for the release in beta; otherwise I think the likely course is to back out stabilization. Part of me says we should just do that, though -- is there some reason we can't destabilize this feature instead?

@craterbot craterbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 26, 2018
@craterbot
Copy link
Collaborator

🚧 Experiment pr-55192-2 is now running on agent aws-2.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Oct 26, 2018
@alexcrichton alexcrichton added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 27, 2018
@alexcrichton
Copy link
Member

I'm retagging this as beta-nominated as since this was destabilized in 1.30.0 this now affects 1.31.0 and if we want to continue to leave this as stable it'll presumably need this fix!

@craterbot
Copy link
Collaborator

🎉 Experiment pr-55192-2 is completed!
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 28, 2018
@pietroalbini
Copy link
Member

No regressions.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 28, 2018

📌 Commit ca35ca8 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 28, 2018
@bors
Copy link
Contributor

bors commented Oct 28, 2018

⌛ Testing commit ca35ca8 with merge 4f5cfa6...

bors added a commit that referenced this pull request Oct 28, 2018
Fix ordering of nested modules in non-mod.rs mods

Flatten relative offset into directory path before adding inline
(mod x { ... }) module names to the current directory path.

Fix #55094
@bors
Copy link
Contributor

bors commented Oct 28, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 4f5cfa6 to master...

@bors bors merged commit ca35ca8 into rust-lang:master Oct 28, 2018
@Mark-Simulacrum Mark-Simulacrum added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 28, 2018
@pietroalbini pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 28, 2018
@cramertj
Copy link
Member Author

I don't think this should have been merged without more discussion on #55094. cc @rust-lang/lang this changed the behavior of mod y { #[path = "z.rs"] mod z; } in x.rs to x/y/z.rs rather than the old behavior of y/z.rs. Note that #[path = "z.rs"] mod z; inside x.rs still points to z.rs (rather than x/z.rs). I don't think we can get away with changing the last point-- there's almost certainly code in the wild that uses #[path = "..."] from a file named something other than mod.rs. Are we okay with this inconsistency, or should we try one of the alternatives above that preserves the old #[path] behavior? (old behavior: #[path] is relative to the current directory, unless it's in an inline mod ... { .... } which causes it to nest in subdirs of the current directory)

bors added a commit that referenced this pull request Oct 29, 2018
[beta]: Prepare the 1.31.0 beta release

* Update to Cargo's branched 1.31.0 branch
* Update the channel to `beta`

Rolled up beta-accepted PRs:

* #55362: Remove `cargo new --edition` from release notes.
* #55325: Fix link to macros chapter
* #55358: Remove redundant clone (2)
* #55346: Shrink `Statement`.
* #55274: Handle bindings in substructure of patterns with type ascriptions
* #55297: Partial implementation of uniform paths 2.0 to land before beta
* #55192: Fix ordering of nested modules in non-mod.rs mods
* #55185: path suggestions in Rust 2018 should point out the change in semantics
* #55423: back out bogus `Ok`-wrapping suggestion on `?` arm type mismatch

Note that **this does not update the bootstrap compiler** due to #55404
@petrochenkov
Copy link
Contributor

FWIW, I don't even care about precise rules anymore.
We should just land something more or less acceptable, then wait for Rust 2021 and redesign everything from scratch.

@cramertj cramertj deleted the nested-mod branch October 29, 2018 19:47
@cramertj
Copy link
Member Author

@petrochenkov If we're willing and able to land this breaking change, then we can probably also land a breaking change that would make #[path] always relative to the current file's directory, which would be hugely preferable in my opinion. Both of these break #[path] only when used from a nested module, but the always-current-file's-directory-relative plan breaks #[path] when used in nested modules inside a file named mod.rs, which this change does not. That seems like a pretty minor difference to me.

@petrochenkov
Copy link
Contributor

@cramertj

If we're willing and able to land this breaking change, then we can probably also land a breaking change that would make #[path] always relative to the current file's directory, which would be hugely preferable in my opinion.

Quite possible.
Let's pass it through crater, then it will be clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants