Cleanup internal imports #243
Conversation
Imports now only come from two modules: `crate`, which contains all public symbols, and `crate::internal`, which contains all the internal symbols. I'm surprised the split works out so cleanly. All internal apis are now `pub(crate)`. Some of these will become public again in the uv fork. I used nightly rustfmt to group the imports.
| #[cfg(feature = "serde")] | ||
| type SemVS = Range<SemanticVersion>; |
There was a problem hiding this comment.
I had a bunch of errors caused by that feature-gated import in the past, so now i inlined it
| indexes_to_remove in prop::collection::vec((any::<prop::sample::Index>(), any::<prop::sample::Index>(), any::<prop::sample::Index>()), 1..10) | ||
| indexes_to_remove in vec((any::<Index>(), any::<Index>(), any::<Index>()), 1..10) |
There was a problem hiding this comment.
We were already using plain vec in other places
|
Two follow up thoughts. If there are nightly FMT ways to preserve this consistent formatting, can we add it to our CI runs? Similarly, if there are clippy lints encouraging |
| use crate::solver::DependencyProvider; | ||
| use crate::type_aliases::{IncompDpId, Map}; | ||
| use crate::version_set::VersionSet; | ||
| use crate::{DependencyProvider, DerivationTree, Map, NoSolutionError, VersionSet}; |
There was a problem hiding this comment.
Well this is exactly the kind of cyclic imports that I’ve always wanted to avoid. crate imports internal.core which in turn imports crate. I believe this lead into putting definitions and functions in a much less organized and maintainable way in the long term, because we don’t care where we put it and just pub(crate) it.
There was a problem hiding this comment.
I do find everything at the root very useful and convenient for outside callers of the library, or test files. Just not for the inside of the library itself.
There was a problem hiding this comment.
I've reverted the internal import changes
There was a problem hiding this comment.
Sorry for the long wait time. I didn’t mean specifically internal just inside the library in general. The idea is to avoid cyclic imports, which are signs that something is not compartmentalized properly. Because a module that requires cyclic imports means that any other module that would need a functionality from this one, would also end up with the whole crate (all its modules) as a dependency. Which makes things like dead code elimination more complex, and other bad architecture proofs in general.
So basically, I find it fine to have examples, benchmarks, external tests, ... to import everything from the new top level crate. But I’d like to avoid having the library modules (anything that is in the import path from the root lib.rs) to import crate module.
That being said, I know I’m a bit stubborn on this, and I know this isn’t something very common in rust crates, so I’m willing to bend if you all think its better.
There was a problem hiding this comment.
I've never gotten the hang of keeping my imports well structured. So I personally would not mind making it a cyclical ball of spaghetti. :-P On the other hand, I'm terrible at naming things and I never remember to fix documentation. This project is better for your efforts to rein in those bad habits of mine.
So whichever way we decide, I would deeply appreciate automation that insists on it. Something in CI, even just a grep for the "bad" patterns, or are there tools in the ecosystem for identifying cyclical imports.
There was a problem hiding this comment.
Cyclical ball of spaghetti sounds delicious! ahah. I wanted to explain my reasoning because every time something is cyclical, it makes everything more complex. Dependency solving is more complex, memory releasing for garbage collector is more complex, etc. I see no reason why import graph solving would not suffer from similar issues, potentially leading to longer compilation times or less optimal dead code elimination for compiled bundles.
But that’s just my hunch and it isn’t backed by any scientific proof or concrete testing using Rust and using our code base in particular. So take that with a grain of salt. And if you think it makes code reading and maintenance simpler I can’t argue against that.
There was a problem hiding this comment.
For concrete data, I guess compiling a rather simple example based on pubgrub could be used. To see if this change makes any impact on compile time and bundle sizes.
There was a problem hiding this comment.
Now that I think about it, it probably would not change anything for pubgrub, because its not like people would use only a subset of pubgrub, they basically would use one of the top level modules that do need everything in the lib. There isn’t any part of this lib that really stands on its own.
There was a problem hiding this comment.
Resolving symbols to the source location is comparatively simple for a compiler: You build an alias map from exported symbol to origin path or the local definition for each file once and then when looking up a symbol, you iterate over the alias until you find the source location. If i want to resolve Foo, i walk through the scopes until i find a use crate::Foo, then i ask crate/src/lib.rs for Foo, it has memorized it as coming from crate::foo/src/foo.rs due to pub use foo::Foo, then i go to crate::foo/src/foo.rs and it tells me it has a local stuct Foo, so i get my fully qualified path crate::foo::Foo (and pub/priv checking along the way).
I found cyclical imports often become unavoidable with more complex crates, e.g. between the error capturing some struct for context and that struct return the error in its methods. I haven't found cyclical imports being a signal for bad architecture.
There are something that are known to be costly to the compiler, notably large/many dependencies, proc macros (need to be compiled, executed and then rustc needs to compile the bulk of generated code), generics (rustc generates and processes monomorphized code for each instantiation, i.e. n different usages give us n times the code) and llvm in general.
I say we structure the code in the way that's easiest for us to maintain and move as much work as possible to the compiler. Pubgrub compile ridiculously fast compare to any other crate i've worked on (0.2s for incremental cargo build, and ~2s for cargo test on my desktop) so i have zero worries about compile times.
There was a problem hiding this comment.
Thanks for the detailed answer! Alright, don’t forget to "unrevert" your changes then.
This reverts commit dd9b290.
Imports now only come from two modules:
crate, which contains all public symbols, andcrate::internal, which contains all the internal symbols. I'm surprised the split works out so cleanly. I used nightly rustfmt to group the imports.All internal apis are now
pub(crate). Some of these will become public again in the uv fork.I used my ide's lints to remove unnecessary path prefixes and cleaned some other nitpicks on the way.
No functional changes, this is a maintenance-only PR.