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

Better hygiene for core crate naming (#180) #327

Merged
merged 6 commits into from
Dec 26, 2023
Merged

Conversation

tyranron
Copy link
Collaborator

@tyranron tyranron commented Dec 22, 2023

Resolves #180

Synopsis

See #180 (comment) for details.

At the moment, compilation fails in a local crate core is present in workspace or some another crate is renamed as core.

Solution

Use ::derive_more::core instead of ::core in macro expansions.

Checklist

  • Documentation is updated (not required)
  • Tests are added/updated (hard to test)
  • CHANGELOG entry is added

@tyranron tyranron added this to the 1.0.0 milestone Dec 22, 2023
@tyranron tyranron self-assigned this Dec 22, 2023
impl/src/add_assign_like.rs Outdated Show resolved Hide resolved
Comment on lines +54 to +55
#[doc(hidden)]
pub use core;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I don't think this is worth all this. I think the advice: "don't call your crate core" is good enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JelteF we already had that episodically here and there across the code base. I don't see any downsides of this, and it doesn't cost us anything, except making the codebase a little bit more consistent.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This basically starts including all of std in our API surface. Can you check if that has impact on compile times or crate size?

Copy link
Collaborator Author

@tyranron tyranron Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JelteF hmm... I don't think that use statements actually include something into crate (it's extern crate which does), rather than just making outside types nameable or accessible from other crates. Furthermore, every Rust toolchain has the core lib being precompiled and extern crated already there. So it's only up to LTO to eliminate redundant code in the final binary.

Anyway, running

cargo bloat --example deny_missing_docs --features full --profile release --crates --split-std

Shows on master:

 File  .text     Size Crate
12.1%  28.3%  58.6KiB std
11.2%  26.1%  54.2KiB core
 7.2%  16.9%  35.1KiB gimli
 5.3%  12.3%  25.5KiB addr2line
 4.6%  10.7%  22.1KiB rustc_demangle
 2.1%   5.0%  10.3KiB [Unknown]
 1.8%   4.2%   8.7KiB alloc
 0.4%   0.9%   1.9KiB object
 0.2%   0.4%     940B memchr
 0.1%   0.2%     432B trybuild
 0.1%   0.1%     272B panic_abort
 0.0%   0.1%     124B basic_toml
 0.0%   0.0%      24B panic_unwind
 0.0%   0.0%       8B libc
 0.0%   0.0%       4B deny_missing_docs
42.8% 100.0% 207.3KiB .text section size, the file size is 484.1KiB

And on this branch:

 File  .text     Size Crate
12.1%  28.3%  58.6KiB std
11.2%  26.1%  54.2KiB core
 7.2%  16.9%  35.1KiB gimli
 5.3%  12.3%  25.5KiB addr2line
 4.6%  10.7%  22.1KiB rustc_demangle
 2.1%   5.0%  10.3KiB [Unknown]
 1.8%   4.2%   8.7KiB alloc
 0.4%   0.9%   1.9KiB object
 0.2%   0.4%     940B memchr
 0.1%   0.2%     432B trybuild
 0.1%   0.1%     272B panic_abort
 0.0%   0.1%     124B basic_toml
 0.0%   0.0%      24B panic_unwind
 0.0%   0.0%       8B libc
 0.0%   0.0%       4B deny_missing_docs
42.8% 100.0% 207.3KiB .text section size, the file size is 484.1KiB

Which doesn't show any meaningful difference.

And for the timings...

master:

$ cargo bloat --example deny_missing_docs --features full --profile release -j 1 --split-std --time
 Time Crate
9.23s trybuild
2.28s derive_more_impl
1.49s serde_json
1.48s serde_derive
1.38s syn
1.38s serde
1.26s basic_toml
0.53s glob
0.38s rustversion
0.37s proc_macro2
0.28s termcolor
0.26s derive_more
0.17s unicode_segmentation
0.15s convert_case
0.14s ryu
0.11s quote
0.08s once_cell
0.07s deny_missing_docs
0.04s unicode_ident
0.04s itoa
0.02s unicode_xid
0.01s static_assertions

this branch:

$ cargo bloat --example deny_missing_docs --features full --profile release -j 1 --split-std --time
 Time Crate
9.22s trybuild
2.25s derive_more_impl
1.49s serde_derive
1.47s serde_json
1.36s syn
1.36s serde
1.27s basic_toml
0.53s glob
0.38s rustversion
0.37s proc_macro2
0.28s termcolor
0.25s derive_more
0.16s unicode_segmentation
0.15s convert_case
0.14s ryu
0.11s quote
0.08s once_cell
0.07s deny_missing_docs
0.04s itoa
0.04s unicode_ident
0.03s unicode_xid
0.01s static_assertions

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this was stupid friday evening thinking of me. Thank you for explaining that this doesn't impact binary size or build time (I guess generating docs would take a lot longer if it wasn't #[doc(hidden)] though)

@tyranron tyranron requested a review from JelteF December 22, 2023 16:33
@tyranron tyranron marked this pull request as ready for review December 22, 2023 16:34
impl/src/as/mod.rs Outdated Show resolved Hide resolved
JelteF
JelteF previously approved these changes Dec 23, 2023
Copy link
Owner

@JelteF JelteF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I called out one place where maybe one we can use the shorter path, other than that this looks good to me.

Comment on lines +54 to +55
#[doc(hidden)]
pub use core;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this was stupid friday evening thinking of me. Thank you for explaining that this doesn't impact binary size or build time (I guess generating docs would take a lot longer if it wasn't #[doc(hidden)] though)

Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
@tyranron
Copy link
Collaborator Author

tyranron commented Dec 26, 2023

ping @JelteF (needs approve for merge)

@tyranron tyranron merged commit 81ede4a into master Dec 26, 2023
17 checks passed
@tyranron tyranron deleted the 180-better-hygiene branch December 26, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo test fails in package named "core"
2 participants