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 for module redesign. #2108

Closed
wants to merge 3 commits into
base: master
from

Conversation

@withoutboats
Copy link
Contributor

withoutboats commented Aug 14, 2017

This RFC describes a proposal to change the Rust module system. Its the culmination of a long, iterative cycle of design and feedback. We, the authors of this RFC (@aturon and myself), believe it is a significant clarification of our current system, making it easier to use and easier to learn.

Because it is a relatively large RFC, I have broken the text into multiple files. You can find the rendered root of the RFC here.

The overview document gives a high level overview of the proposed new system and how it differs from the current system.


For people who have been involved in the iteration process on the internals thread, a brief summary of how it differs from the most recent proposals:

  • Loading Files documents how files will be loaded without mod statements; the exact mechanism has been refined & improved from the previous iteration.
  • Paths documents changes to paths, we've chosen to prefix crate-local paths with crate:: rather than some of the more radical syntaxes proposed in recent iterations.
  • Visibility & Re-exporting contains the most significant material that wasn't in the previous iterations. Please read & digest :-)

For people joining the conversation about modules for the first time, welcome! There has been a lot of iteration on the internals forum which you can read if you want, and there are blog posts (referenced in the RFC) which describe past stages of our thinking about modules. Please don't feel that you do not have enough context to participate in this thread, your feedback matters!


Based on past experience, I predict this RFC will be high traffic. So a couple of reminders:

  • If you find yourself in a conversation with a small number of other users "workshopping" a particular topic, consider finding an out-of-band medium (such as IRC) to hash out the details, and bringing the substantial proposal back to the thread afterward.
  • If you've staked out a position, consider when making a new comment whether you're restating the same position or if your thoughts have been clarified or evolved. Unless your prior post was significantly misunderstood, the conversation doesn't progress much by the same arguments being repeated in cycles.
  • When the conversation grows long, its unreasonable to expect someone coming into it to have read every prior comment. But if you have an objection that seems very obvious to you, there's a good chance its been discussed previously. Consider searching the thread to see if maybe that subject has already reached an endpoint.

Lastly, if you want to leave a comment on the RFC but for any reason at all don't want to leave it publicly, I receive email addressed to woboats@gmail.com. I can't promise I'll respond, but I will read it.

build layer, instead of the source layer. This has some disadvantages, in that
users may prefer to not have to open the build configuration either.

This will require migrating users away from `mod` statements toward the new

This comment has been minimized.

@arielb1

arielb1 Aug 14, 2017

Contributor

This will also make building "modern" Rust programs much harder without cargo - you can no longer hardcode the list of dependencies in a variable and just call rustc on lib.rs.

Also, might run against the command-line limit for huge crates, but I'm not sure how much of a concern is that.

This comment has been minimized.

@withoutboats

withoutboats Aug 14, 2017

Contributor

This will also make building "modern" Rust programs much harder without cargo - you can no longer hardcode the list of dependencies in a variable and just call rustc on lib.rs.

I'd be eager to get user reports from people actually not passing --extern arguments today. We can also support an analogous modules path env var.

This comment has been minimized.

@arielb1

arielb1 Aug 14, 2017

Contributor

At least I often collect the --extern arguments in an env var and call rustc explicitly when debugging (that includes running rustc under gdb and running rustc under valgrind, which aren't easy from cargo).

This comment has been minimized.

@arielb1

arielb1 Aug 14, 2017

Contributor

While I don't expect normal users to be debugging rustc, and you can always copy-paste the arguments using cargo build --verbose, that's one annoyance.

This comment has been minimized.

@arielb1

arielb1 Aug 14, 2017

Contributor

Also, running cargo build --verbose, editing the argument line, then rebuilding. This will get much much more annoying if argument lines contain a hundred files.

This comment has been minimized.

@AndrewBrinker

AndrewBrinker Aug 14, 2017

It seems like having the option to use a file or an environment variable to specify external dependencies may resolve these concerns.

This comment has been minimized.

@arielb1

arielb1 Aug 15, 2017

Contributor

Doesn't cargo support a way to change how it invokes rustc? Why doesn't that work for running rustc under a debugger?

Because with gdb you have to run it as

$ gdb $RUSTC
gdb> set args ARG1 ARG2
gdb> run
$

Also, sometimes editing the command-line to remove some flags, which is sometimes needed when poking around.

This comment has been minimized.

@eddyb

eddyb Aug 15, 2017

Member

Doesn't gdb --args $RUSTC ARG1 ARG2 work?

This comment has been minimized.

@arielb1

arielb1 Aug 15, 2017

Contributor

maybe it does?

This comment has been minimized.

@gnzlbg

gnzlbg Aug 15, 2017

Contributor

@arielb1 If rustc would also allow you to pass a path to a file listing modules to include, would that appease your concerns?

This is how C++ modules work. There one has a modules.modulemap file, that maps files and sets of files to modules. The compiler can take many of these files, and will then include all modules listed in those files (but they can also include specific files directly).

Since I don't like having to include modules in the Cargo.toml file (I find this "unclean"), maybe we could adopt a similar approach to C++, where we have a canonical modules.modulesmap file in the crate directory (just like build.rs), and if it's there cargo detects it and uses it, and if not, cargo will generate one automatically for you from the filesystem structure in the temporary build directory [*]. That way, you would be able to copy-paste the command of cargo build --verbose, those wanting to specify the module file hierarchy differently can still write their own .modulemap files, and my OCD is happy as well because Cargo.toml remains clean.

[*] we can also allow specifying paths to multiple module map files in Cargo.toml, but experience with build.rs has shown that we probably should try if having a canonical location for it is enough. Since modules.modulemap files can specify paths to other modules.modulemap files, worst case users would need to add a dummy file that points somewhere else, and we don't need any Cargo.toml property for this at all.


EDIT: the module.modulemaps file in C++ allow also specifying things like item-level visibility and stuff like that. It might be worth to explore if allowing something like this in Rust could be used to remove boilerplate. As syntax we can use JSON or TOML to just specify a map of relative file paths to modules or modulemap files. But these things are details.

`exported`.

If a user wants to make one of their file submodules a part of their API, they
can do so using the `export keyword (no `use`), followed by the name of the

This comment has been minimized.

@tomaka

tomaka Aug 14, 2017

Missing a closing quote after export

crates which are both managed by cargo, as when cargo detects that it will
instead pass no `--module` arguments.

### Using `export` changes semantics

This comment has been minimized.

@arielb1

arielb1 Aug 14, 2017

Contributor

This sounds like a way to annoy people during the transition period. Is there a reason we can't have:

export pub pub(crate)
today error global crate-local
checkpoint 1 global global (deprecated) crate-local
checkpoint 2 global crate-local crate-local (deprecated)
checkpoint 3 global crate-local error

The disadvantage is that in checkpoint 1 people will have to mark all of their pub imports as either pub(crate) or export, and they might be annoyed by pub(crate) being ugly. I think that's better than having pub mean export in all "old" crates and pub(crate) in all "new" crates, with no way to incrementally make the change.

This comment has been minimized.

@arielb1

arielb1 Aug 14, 2017

Contributor

Also, it might be nice to have the private-export lint only start in Ckp2, so that in Ckp1 you could just change all pub to export and not have to go over them one-by-one.

This comment has been minimized.

@aturon

aturon Aug 14, 2017

Member

@withoutboats and I talked about such a transition strategy, but:

  • It means that code must undergo multiple periods of churn (changing to pub(crate) and then to pub)
  • That's particularly painful for apps, where the distinction doesn't matter
  • It greatly prolongs the overall transition

On the other hand:

  • we can provide a rustfix to automatically convert to the new pub/export model, to make it one painless jump
  • I think in practice it will be highly obvious when you encounter code that is using the old pub semantics, and that with rustfix the transition can and will happen pretty quickly.

Another option, as @withoutboats mentioned, is to move to a different keyword altogether, like public. I for one would be sad to see pub go :-)

This comment has been minimized.

@withoutboats

withoutboats Aug 14, 2017

Contributor

I wouldn't mind seeing pub go, but I do think its very valuable that if a stack overflow example happens to have pub in it, it will still compile - in cases like that (e.g. documentation), the semantic change here hopefully doesn't matter very often (that is, I hope you aren't copying your external API from old stack overflow answers 😉).

In other words, even though this "change semantics if we ever encounter extern" sounds like the most disruptive mechanism possible, when I played out the scenarios in my head, I found that it seemed like the least disruptive thing we could do.

This comment has been minimized.

@cramertj

cramertj Aug 14, 2017

Member

There's some existing precedent here with pub(restricted). Using pub(restricted) anywhere made the "public in private" warning a hard error (see here).

This comment has been minimized.

@arielb1

arielb1 Aug 15, 2017

Contributor

I don't think it's a breaker, just annoying when you have a project of 20 crates, half of them use the old semantics, and half use the new semantics.

This comment has been minimized.

@xfix

xfix Aug 15, 2017

Contributor

I wonder if it's possible to use crate keyword (currently only used in extern crate) instead of pub. This would avoid issues caused by changing well established meaning of pub keyword. Say...

crate fn hello() {
    println!("Hello, world!");
}

crate struct Example {
    crate a: i32,
    b: i64,
}
This item is expected to be an array of strings, which are relative paths from
the cargo manifest directory to files containing Rust source code. If this item
is specified, cargo will canonicalize these paths and pass them to rustc as the
`--module` argument when building this target.

This comment has been minimized.

@cramertj

cramertj Aug 14, 2017

Member

Just to clarify-- if any module list is passed here, Cargo won't do any directory searching, right?

This comment has been minimized.

@withoutboats
@tomaka

This comment has been minimized.

Copy link

tomaka commented Aug 14, 2017

One advantage of the crate:: prefix that isn't mentioned in the "Paths" section is that it is consistent with $crate:: in macros.

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Aug 14, 2017

My position is pretty much the same - I'm for the relative path changes, somewhat for the visibility overhaul (it makes the right thing prettier, but requires an ugly long checkpoint period), and against automod (the mod keyword just does not annoy me and is sometimes very nice - so overall a positive).

// This is an error: you have exported something which is only marked `pub`
export foo::Foo;
```

This comment has been minimized.

@cramertj

cramertj Aug 14, 2017

Member

To clarify, this should work:

mod foo {
    export struct Foo;
}
export foo::Foo;

but removing export foo::Foo; would cause a hard error. Is that correct?

This comment has been minimized.

@withoutboats

This comment has been minimized.

@atmorgan

atmorgan Aug 15, 2017

Does this still apply if the mod foo is in a separate file? This seems confusing to me, that I have to declare Foo as 'exportable' before I can really export it, unless it's being declared at the path where it's being exported, in which case declaring it as exportable is the same as exporting it.

Also in actually writing this code, I wouldn't be able to add 'export' to the 'struct Foo' declaration without also adding the export foo::Foo, and vice-versa. This seems bad for someone who's trying to add in the right export to make their library compile.

Allowing "re-exports" to export items that aren't already exported, and requiring that export statements only appear within the top-level module or explicitly-exported submodules, would take care of both of those confusions. Using export in say a struct or function definition would be shorthand for declaring the struct with some appropriate visibility modifier, and then re-exporting it. Otherwise export just defines the public API of the crate. (And in order to avoid spaghetti code, re-exports don't make symbols that can be used internally by the crate.)

We could make the error on a non-exposed `export` item a warning instead of a
hard error.

[migration]: detailed-design/migration.md

This comment has been minimized.

@newpavlov

newpavlov Aug 14, 2017

Incorrect link, it should be [migration]: migration.md.

@stevenblenkinsop

This comment has been minimized.

Copy link

stevenblenkinsop commented Aug 14, 2017

Something I'm not clear on is whether, with this RFC, use is no longer an item, or if it will become a synonym for pub(self) ::.

@phaylon

This comment has been minimized.

Copy link

phaylon commented Aug 14, 2017

An unmentioned alternative for file loading is a glob specifier (mod *). It's explicit but doesn't require specifying everything, it provides a place for default visibility, and it has symmetry with imports.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Aug 14, 2017

Something I'm not clear on is whether, with this RFC, use is no longer an item, or if it will become a synonym for pub(self) ::.

This is an unresolved question for me! I still don't think it was the right choice, but I'm not sure (especially since the use io; example will no longer work) if its worth the breakage cost.

One advantage of the crate:: prefix that isn't mentioned in the "Paths" section is that it is consistent with $crate:: in macros.

Possible in macros 2.0 we could even replace $crate entirely with just interpreting crate paths in a particular way inside of macro bodies.

@newpavlov

This comment has been minimized.

Copy link

newpavlov commented Aug 14, 2017

After a first read I quite like this proposal!

I think it's worth to explicitly state that export foo; will also make item visible to entire crate. Additionally I would like to see a more attention to tests and benchmarks directories, I am not sure if they should follow the same rules as examples.

Also how will internal tests modules look like? Today we can write:

#[cfg(test)]
mod tests;

And tests can use [dev-dependencies], while in the proposal, as I understand it, tests will be always loaded, which will cause an error on normal build due to the usage of dependency not listed in the [dependencies] section.

@theduke

This comment has been minimized.

Copy link

theduke commented Aug 14, 2017

(edited for clarity and a suggestion)

My main issue with the (overall very good) proposal are these two points:

  • the ambivalence between use statements and using an item still persists !
  • use statements still require use self::... for relative imports

What I would much prefer to see:

Refering to external items always requires the :: prefix, both in use statements and when using the item.

This makes it unambiguous that ::* always is an external item/module, crate::* is from the crate root, and everything else is relative/local.

// BEFORE:
use std::collections::HashMap;
type T = ::std::collections::HashMap;
use self::submod::SomeType;

// AFTER:
use ::std::collections::HashMap;
type T = ::std::collections::HashMap;
use submod::SomeType;

I reckon this is harder/impossible to do in a backwards-compatible manner?

@phaylon

This comment has been minimized.

Copy link

phaylon commented Aug 14, 2017

I'd also note an addition to the Loading Files Drawbacks: The workflow where you simply comment out module trees during refactoring would become a lot more work. In this scheme, I'd have to maintain a manual module list directly in Cargo.toml, and comment out any export statements anyway.

A flag inside each module (a solution that comes up usually) would require opening every file. I would also assume the file would need to parse correctly for that.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Aug 14, 2017

@phaylon That's also closely related to another issue: platform-specific modules. Without the mod declaration, each platform-specific module would have to be placed inside a cfg-d mod { ... } block, which causes an extra 4-spaces worth of indentation in all of the platform-specific files.

Both of these issues seem like things that could potentially be solved via some kind of ignore annotation in Cargo.toml. I'm sure other ideas will come up, too.

@phaylon

This comment has been minimized.

Copy link

phaylon commented Aug 14, 2017

Last issue (for now ;)): Another Loading Files alternative is to have auto loading of modules, but a Cargo.toml option to disable it and allow explicit module specification with the usual mod syntax (instead of requiring a list in Cargo.toml).

Personally I'd prefer that option to be off by default and activated for new projects in cargo new but I have no problem just disabling that :)

@rpjohnst

This comment has been minimized.

Copy link

rpjohnst commented Aug 14, 2017

I am concerned about a) the amount of breakage and b) the potential confusion introduced by export.

Would it perhaps be better to stick with the current semantics for pub and pub(crate)? This matches other languages more closely (e.g. C# public vs internal, Java public vs default), and it involves less invasive change to existing semantics. The point at which a library author goes to remove a mod file; declaration, thus making the module visible to the whole crate, provides a natural point for them to reevaluate the visibility of its children.

This notably still keeps the advantages of relative paths for re-exports, via the $visibility $item syntax. It also keeps the improvements to local visibility reasoning, via the changes to module visibility. It still allows for the new public-in-private warning. It completely dodges questions like "pub vs export" by simply reusing the existing pub(restricted) syntax in which all visibility names start with pub.

It does have the downside of potentially making pub(crate) and pub(super) more common. At the same time though, accidentally exporting something doesn't seem to be a very big problem- I suspect things that need to remain private for correctness/safety reasons are likely to simply be private rather than pub of any variety.

Overall, I believe this removes the dependence on an epoch boundary? Everything can be transitioned piecemeal- loading files by removing mod file;, crate:: by simply adding it in the appropriate places, and re-exports by removing use absolute::prefix:: from pub use absolute::prefix::Item;.


```rust
use crate::foo::Bar;
::crate::foo::Bar;

This comment has been minimized.

@purpleposeidon

purpleposeidon Aug 14, 2017

Shouldn't this be crate::foo::Bar;? (Or maybe crate::foo::baz();)

This comment has been minimized.

@withoutboats

withoutboats Aug 14, 2017

Contributor

An unresolved question :-)

crate:: matches self:: and super::. Maybe ::self:: and ::super:: should be valid (and crate should accept both forms also).

This comment has been minimized.

@stevenblenkinsop

stevenblenkinsop Aug 14, 2017

So, provided this were the case, then in each module, the root :: would contain external crates, this crate, this module, and this module's parent? One nice thing about this would be that it would mean you could say "use paths are always relative to ::", rather than needing the exception "unless they begin with crate, self, or super".

@@ -0,0 +1,194 @@
# Loading Files

When building a Rust project, rustc will and parse some files as Rust code in

This comment has been minimized.

@est31

est31 Aug 14, 2017

Contributor

Will and parse

@KodrAus

This comment has been minimized.

Copy link
Contributor

KodrAus commented Aug 14, 2017

I'm glad to see that pub(self) is a thing. It's much more obvious than the implicit public to descendents.

EDIT: Oh I hadn't realised it already exists.

@bvssvni

This comment has been minimized.

Copy link

bvssvni commented Aug 14, 2017

One thing I could not find mentioned anywhere in the RFC is how to reexport stuff internally.

For example, if you have a folder with lots of files, you might want to reexport those under the same module to avoid typing the full path to dozens of different modules. At the same time the module might not be exported and visible in the API. If you deprecate pub use I don't see how you can reexport stuff internally.

@rpjohnst

This comment has been minimized.

Copy link

rpjohnst commented Aug 14, 2017

@bvssvni It's covered here- the syntax is $visibility $path;. As currently written, if you only want to reexport within the crate, you just use pub instead of export.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Aug 14, 2017

@rpjohnst is correct, though I think that's been made obvious enough in the RFC.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Aug 17, 2017

I don't believe this RFC can be accepted in a piecemeal fashion, because these changes interact in many ways. The advantages that we see in this system - incremental learning, heightened local reasoning, ergonomics for the common cases - all emerge from the cumulative impact of these changes together.

@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Aug 17, 2017

FWIW, I think internal would have much more of the right connotations for me than local. I would intuit local to have significantly smaller scope than the whole crate, closer to something like normal private items, and would be confused about what the difference is (and may wonder whether local is somehow even more restrictive). I suppose others may have the same kind of intuitions about internal though, so YMMV.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Aug 17, 2017

@glaebhoerl I agree, but would much rather have a shorter keyword than the most intuitive one. In fact, I rather like @est31's loc.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Aug 17, 2017

And I still like protected the best 😅. I'm going to go with local in the RFC and we can bikeshed that name once we've got more consensus on the bigger stuff.

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Aug 17, 2017

would much rather have a shorter keyword than the most intuitive one

What about int? Stands for "intuitive" and "internal".

:trollface:

@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Aug 17, 2017

We can also have uint, short for uninternal, for exported items.

@stevenblenkinsop

This comment has been minimized.

Copy link

stevenblenkinsop commented Aug 17, 2017

On a related note, I was thinking about ins for "insular".

@Korvox

This comment has been minimized.

Copy link

Korvox commented Aug 17, 2017

I feel bad for thinking it, but if Rust ever wanted to earn some street e-cred you could call it protec for... you know.

@anowell

This comment has been minimized.

Copy link

anowell commented Aug 17, 2017

How is there not a suggestion to use visible or vis as the visibility modifier keyword?

@stevenblenkinsop

This comment has been minimized.

Copy link

stevenblenkinsop commented Aug 17, 2017

I like it. There might be concern that it doesn't inherently answer the question "visible to whom?" which is the whole point of a visibility modifier. This might make the pub vs vis distinction unclear. On the other hand "publicly visible" is clearly a step up from just "visible", so it might be fine. Another objection might be that all names are visible, just not necessary external to the module.

Another one for the pile: "endo".

@CasualX

This comment has been minimized.

Copy link

CasualX commented Aug 17, 2017

What's the reason crate can't be used? Does it collide with crate:: in paths? I seem to remember discussion about itwhere the main downside was that it would look weird but I can't find it back .

@phaylon

This comment has been minimized.

Copy link

phaylon commented Aug 17, 2017

I just want to say that I find the mod-for-configuration idea quite brilliant. Big thanks to everyone who helped arrive at that, and special big thanks to @withoutboats and @aturon for the patience.

And to not let an opportunity for bikeshedding go to waste, I wonder if our for in-crate visibility would be descriptive.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Aug 17, 2017

@phaylon As in "our stuff"?

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Aug 17, 2017

Also, I'm surprised nobody has mentioned priv

@phaylon

This comment has been minimized.

Copy link

phaylon commented Aug 17, 2017

@mark-i-m Yea. As in our trait Foo {}, our(self) enum State {}. Might be a bit too phrase-like for Rust.

@phil-opp

This comment has been minimized.

Copy link

phil-opp commented Aug 17, 2017

I don't like that modules are crate visible by default. Yes, it makes it easier to write simple applications, but at some point the user wants a private module (e.g. moving large internal functions to a submodule). At this point it gets much more difficult:

  • The user has to find out where to put the visibility attribute. It seems a bit strange to add some mod foo; statement to the parent module just to control something common as visibility.
  • The user has to find a visibility attribute that means "private". Everything else in Rust is private by default and neither private nor priv works. The solution is the arcane local(self) which definitely isn't apparent (and you will only ever see it for modules).

To me it violates both the private-by-default and the explicit-instead-of-implicit principles of Rust. Also, it makes hiding implementation details in private modules (the better design) more difficult than just exposing everything, which I think is also against Rust's principles.

@nox

This comment has been minimized.

Copy link
Contributor

nox commented Aug 17, 2017

We would introduce a lint against unnecessary mod statements (which can be turned off if you prefer to have a full list).

This also needs to come with a lint that disallows implicit mod statements, to disable the whole implicitness at scale.

The advantages that we see in this system - incremental learning, heightened local reasoning, ergonomics for the common cases - all emerge from the cumulative impact of these changes together.

Splitting the RFC in multiple ones may make it apparent that these advantages you are seeing are marginal and don't justify that many breaking changes for pretty much all code in existence.

@TimNN

This comment has been minimized.

Copy link
Contributor

TimNN commented Aug 17, 2017

Even though this thread is already closed, I would like to thank everyone involved in the discussion for the work they have put into this. Especially a big "Thank you!" to @aturon on @withoutboats. The outline of the new proposal addresses all the concerns I had with the previous one, and in particular assuages my fears that this RFC could fracture the ecosystem.

@yasammez

This comment has been minimized.

Copy link

yasammez commented Aug 18, 2017

So now that I finally found the time to read through all of this, it is already deprecated: this community is indeed moving fast. Jokes aside I really like the direction this is taking: the first blog post had me aggravated, the second left me still a bit nervous and finally this RFC had me saying „I can live with that“. The new, even more conservative proposal seems even better to me. My only concern with the module system was the whole lot of boilerplate you have to write in order to get names into scope: extern crate, mod, use … I always felt a single use statement ought to be enough: if a module needs to be loaded, the compiler/build system should be able to figure that out and do it for me. With that being addressed, I am already happy ;-)

To bikeshed a bit, local seems unrusty (everything else being an abbreviation): maybe loc is too confusing, but it would line up better with the likes of pub impl and fn. Probably a matter of taste.

Also I'd still prefer the from X use Y syntax above an explicit name for the local crate: the module comparison table posted a while ago left me struck with Python's elegance (something Python manages astoundingly often) and I'd like to see something similar in Rust.

@thepowersgang

This comment has been minimized.

Copy link
Contributor

thepowersgang commented Aug 21, 2017

I second the call for the new RFC to be several RFCs. I don't have many issues with the publicity (introducing local) and use changes, but I am very much against implicit loading of files (i.e. not requiring mod to load a file).

Adding implicit loading will add quite a few maintainability problems (especially in multi-person projects). E.g. Separate checkouts will have strange errors due to missing paths/impls instead of just pointing to the mod that would load the missing file)

The current model is generally suitable and matches the eventual code structure well, and I don't recall any real difficulties learning it (but that's just me).

@withoutboats withoutboats referenced this pull request Aug 23, 2017

Closed

Modules #2121

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Aug 23, 2017

The new proposal is up! I know this has been a long slog for everyone, but it would be really helpful to get feedback afresh, even if you were already a fan of this RFC.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Aug 23, 2017

Cross-posting from the new thread:


A general request: the proposal here appeared to be the consensus direction of the previous thread, but there seems to be significant pushback on this thread. Could those of you who were participating on the previous thread comment saying:

  • Whether you think the changes here are stepping in the right direction?

  • Whether you think this proposal did not correctly capture the consensus direction?

  • Whether you're in favor of the broad approach present in the two proposals, and if not, what the main sticking points are?

I think one of the primary problems in discussion here tends to be some disagreement about goals. This updated proposal works hard to retain the original goals around learning curves, while taking on some new constraints:

  • No breaking changes; purely deprecations
  • Retaining the use of mod for those who prefer to have such declarations in their code
  • Providing direct support for e.g. refactoring workflows (via ignore)

All of these were concerns raised over the course of the last few threads, and I believe this design addresses them while, again, retaining the original benefits. I'd like to understand whether there are additional constraints people feel are missing, or other major downsides to the overall approach that haven't been surfaced yet. (And of course, if you're happy with this approach, it'd be helpful to hear from you too!)

@gnzlbg

This comment has been minimized.

Copy link
Contributor

gnzlbg commented Aug 28, 2017

@aturon Maybe it is because I haven't had the time to let the last proposal sink in me yet, too much information in a too short span of time.

My feeling about this has been that I liked the first proposal. It had significant breaking changes, but I thought they were worth it, and that's what epochs are for. I didn't like the second proposal that much, because I am not sure if the wins outweigh the cost. The third proposal moves further along this direction, and I need more time to properly weigh in if they are worth it.

The good thing is the third proposal is backwards compatible, so we might want to pursue this now, and after some epoch go to something more drastic like the first proposal. Anyhow the RFC's are long, going through the three of them is a bit tiring, and the stress of willing to move them to FCP ASAP doesn't really help.

@Kixunil

This comment has been minimized.

Copy link

Kixunil commented Sep 1, 2017

@aturon the new proposal looks much better to me. I think that adding an option to disable auto-scan of modules would help at least during transition (if I have some project with "sketches" - .rs files that are not compile-able, I'd like to be able to compile the project quickly without adding #[ignore] to everything).

Another thing is that I may want to retain those sketches in my project but keep them private - outside of version control and outside of any public files. Having some private setting to not include them would be helpful.

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Sep 1, 2017

@Kixunil this proposal is not the newest one, its #2126 . It doesnt feature any auto-scanning of modules or anything.

@Kixunil

This comment has been minimized.

Copy link

Kixunil commented Sep 1, 2017

@est31 Ah, I missed that, thanks! I like that approach much more than auto-scanning. (but I was under the impression that auto scanning is a highly desired feature)

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Sep 1, 2017

I was under the impression that auto scanning is a highly desired feature

Its not off the table yet... the RFC I've linked is only intended to extract all the things that there is a broad consensus for and to get it implemented during the impl period. There is another RFC planned to experiment with stuff (can't provide a link as its not opened yet).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment