simplify SourceID Ord/Eq by Eh2406 · Pull Request #14980 · rust-lang/cargo (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation10 Commits2 Checks20 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

What does this PR try to resolve?

This is a followup to #14800. Like that PR, this is a small incremental change that does not pull its own weight. If this PR is accepted, the next PR will unlock large performance wins. I am not posting them together because the logic of why this PR is correct is subtle and deserves to be discussed and reviewed without unrelated code changes.

How should we test and review this PR?

All tests pass on all commits. This should be reviewed one commit at a time.

Additional information

I pushed one commit at a time, so that CI can confirm that the assert (in the first commit) is never hit.

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Potential risk:

If someone has two alternate registry hosted on github whose name has one of the weird languages where a < b does not imply a.to_lower() < b.to_lower() (we did this normalization in CanonicialUrl for only github.com domain), then the package order in the lockfile file could switch between different versions of Cargo.

Though this is pretty niche and I doubt it impacts any real world use case.

Eh2406 marked this pull request as ready for review

December 24, 2024 21:53

@rfcbot fcp merge

I propose to the team to merge this. See the comment in 2a9527b for detailed explanation.

tl;dr

• Cargo can never have two SourceIds with everything the same except for the url field, because when interning, we hash only kind, precise, and canonical_url, not url.
• One of the risks we can think of is described in simplify SourceID Ord/Eq #14980
• The other one might be a path dependency having a path ending with .git got chopped off, making the package order different in lockfile. I also think this is a weird use case we can ignore.

If there is any implication we're missing here, please call it out!

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

• @Eh2406
• @Muscraft
• @Rustin170506
• @arlosi
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

Note that all of the risks identified so far involve the implications to ord and the performance benefits mostly relate to eq. If more risks are found, or the risks related to ord are to painful this PR can be updated to leave ord as is and instead do the comparable thing directly on eq. I prefer the code simplicity of eq delegating to ord, but opinions can legitimately vary.

If someone has two alternate registry hosted on github whose name has one of the weird languages where a < b does not imply a.to_lower() < b.to_lower()

May I ask, do you have a real example of this?

I don't, unfortunately. I don't even know if there is a language has such odd upper/lowercase comparison. CJK might potentially maybe?

cc @Eh2406, is it possible to create one real example on your side?

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

Choose a reason for hiding this comment

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

Thanks Jacob.

Since we have reached a consensus, going to merge this now. I don't think waiting for an entire 10-day FCP will make any difference. If there is a real use case, only when the change hits nightly and people start using it will we know.

Merged via the queue into rust-lang:master with commit b696870

Jan 7, 2025

20 checks passed

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 11, 2025

github-merge-queue bot pushed a commit that referenced this pull request

Jan 25, 2025

What does this PR try to resolve?

In PR #14980, the Ord impl for SourceId was changed, but the comment wasn’t updated. So it is now incorrect.

This PR updates the comment to match the implementation.

How should we test and review this PR?

Read the Ord for SourceId code and make sure the updated comment describes what it does.

Additional information

None

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request

Apr 9, 2025

Upstream changes relative to 1.85.1:

Version 1.86.0 (2025-04-03)

Language

• [Stabilize upcasting trait objects to supertraits.] (rust-lang/rust#134367)
• [Allow safe functions to be marked with the #[target_feature] attribute.] (rust-lang/rust#134090)
• [The missing_abi lint now warns-by-default.] (rust-lang/rust#132397)
• Rust now lints about double negations, to catch cases that might have intended to be a prefix decrement operator (--x) as written in other languages. This was previously a clippy lint, clippy::double_neg, and is [now available directly in Rust as double_negations.] (rust-lang/rust#126604)
• [More pointers are now detected as definitely not-null based on their alignment in const eval.] (rust-lang/rust#133700)
• [Empty repr() attribute applied to invalid items are now correctly rejected.] (rust-lang/rust#133925)
• [Inner attributes #![test] and #![rustfmt::skip] are no longer accepted in more places than intended.] (rust-lang/rust#134276)

Compiler

• [Debug-assert that raw pointers are non-null on access.] (rust-lang/rust#134424)
• [Change -O to mean -C opt-level=3 instead of -C opt-level=2 to match Cargo's defaults.] (rust-lang/rust#135439)
• [Fix emission of overflowing_literals under certain macro environments.] (rust-lang/rust#136393)

Platform Support

• [Replace i686-unknown-redox target with i586-unknown-redox.] (rust-lang/rust#136698)
• [Increase baseline CPU of i686-unknown-hurd-gnu to Pentium 4.] (rust-lang/rust#136700)
• New tier 3 targets:
  • [{aarch64-unknown,x86_64-pc}-nto-qnx710_iosock] (rust-lang/rust#133631). For supporting Neutrino QNX 7.1 with io-socket network stack.
  • [{aarch64-unknown,x86_64-pc}-nto-qnx800] (rust-lang/rust#133631). For supporting Neutrino QNX 8.0 (no_std-only).
  • [{x86_64,i686}-win7-windows-gnu] (rust-lang/rust#134609). Intended for backwards compatibility with Windows 7. {x86_64,i686}-win7-windows-msvc are the Windows MSVC counterparts that already exist as Tier 3 targets.
  • amdgcn-amd-amdhsa.
  • x86_64-pc-cygwin.
  • [{mips,mipsel}-mti-none-elf] (rust-lang/rust#135074). Initial bare-metal support.
  • m68k-unknown-none-elf.
  • [armv7a-nuttx-{eabi,eabihf}, aarch64-unknown-nuttx, and thumbv7a-nuttx-{eabi,eabihf}] (rust-lang/rust#135757).

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

• The type of FromBytesWithNulError in CStr::from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, FromBytesWithNulError> was [changed from an opaque struct to an enum] (rust-lang/rust#134143), allowing users to examine why the conversion failed.
• [Remove RustcDecodable and RustcEncodable.] (rust-lang/rust#134272)
• [Deprecate libtest's --logfile option.] (rust-lang/rust#134283)
• [On recent versions of Windows, std::fs::remove_file will now remove read-only files.] (rust-lang/rust#134679)

Stabilized APIs

• [{float}::next_down] (https://doc.rust-lang.org/stable/std/primitive.f64.html#method.next_down)
• [{float}::next_up] (https://doc.rust-lang.org/stable/std/primitive.f64.html#method.next_up)
• [<[_]>::get_disjoint_mut] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.get_disjoint_mut)
• [<[_]>::get_disjoint_unchecked_mut] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.get_disjoint_unchecked_mut)
• [slice::GetDisjointMutError] (https://doc.rust-lang.org/stable/std/slice/enum.GetDisjointMutError.html)
• [HashMap::get_disjoint_mut] (https://doc.rust-lang.org/std/collections/hash_map/struct.HashMap.html#method.get_disjoint_mut)
• [HashMap::get_disjoint_unchecked_mut] (https://doc.rust-lang.org/std/collections/hash_map/struct.HashMap.html#method.get_disjoint_unchecked_mut)
• [NonZero::count_ones] (https://doc.rust-lang.org/stable/std/num/struct.NonZero.html#method.count_ones)
• [Vec::pop_if] (https://doc.rust-lang.org/std/vec/struct.Vec.html#method.pop_if)
• [sync::Once::wait] (https://doc.rust-lang.org/stable/std/sync/struct.Once.html#method.wait)
• [sync::Once::wait_force] (https://doc.rust-lang.org/stable/std/sync/struct.Once.html#method.wait_force)
• [sync::OnceLock::wait] (https://doc.rust-lang.org/stable/std/sync/struct.OnceLock.html#method.wait)

These APIs are now stable in const contexts:

• [hint::black_box] (https://doc.rust-lang.org/stable/std/hint/fn.black_box.html)
• [io::Cursor::get_mut] (https://doc.rust-lang.org/stable/std/io/struct.Cursor.html#method.get_mut)
• [io::Cursor::set_position] (https://doc.rust-lang.org/stable/std/io/struct.Cursor.html#method.set_position)
• [str::is_char_boundary] (https://doc.rust-lang.org/stable/std/primitive.str.html#method.is_char_boundary)
• [str::split_at] (https://doc.rust-lang.org/stable/std/primitive.str.html#method.split_at)
• [str::split_at_checked] (https://doc.rust-lang.org/stable/std/primitive.str.html#method.split_at_checked)
• [str::split_at_mut] (https://doc.rust-lang.org/stable/std/primitive.str.html#method.split_at_mut)
• [str::split_at_mut_checked] (https://doc.rust-lang.org/stable/std/primitive.str.html#method.split_at_mut_checked)

Cargo

• [When merging, replace rather than combine configuration keys that refer to a program path and its arguments.] (rust-lang/cargo#15066)
• [Error if both --package and --workspace are passed but the requested package is missing.] (rust-lang/cargo#15071) This was previously silently ignored, which was considered a bug since missing packages should be reported.
• [Deprecate the token argument in cargo login to avoid shell history leaks.] (rust-lang/cargo#15057)
• [Simplify the implementation of SourceID comparisons.] (rust-lang/cargo#14980) This may potentially change behavior if the canonicalized URL compares differently in alternative registries.

Rustdoc

• [Add a sans-serif font setting.] (rust-lang/rust#133636)

Compatibility Notes

• [The wasm_c_abi future compatibility warning is now a hard error.] (rust-lang/rust#133951) Users of wasm-bindgen should upgrade to at least version 0.2.89, otherwise compilation will fail.
• [Remove long-deprecated no-op attributes #![no_start] and #![crate_id].] (rust-lang/rust#134300)
• [The future incompatibility lint cenum_impl_drop_cast has been made into a hard error.] (rust-lang/rust#135964) This means it is now an error to cast a field-less enum to an integer if the enum implements Drop.
• [SSE2 is now required for "i686" 32-bit x86 hard-float targets; disabling it causes a warning that will become a hard error eventually.] (rust-lang/rust#137037) To compile for pre-SSE2 32-bit x86, use a "i586" target instead.

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

• [Build the rustc on AArch64 Linux with ThinLTO + PGO.] (rust-lang/rust#133807) The ARM 64-bit compiler (AArch64) on Linux is now optimized with ThinLTO and PGO, similar to the optimizations we have already performed for the x86-64 compiler on Linux. This should make it up to 30% faster.

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request

May 10, 2025

This MR contains the following updates:

MR created with the help of el-capitano/tools/renovate-bot.

Proposed changes to behavior should be submitted there as MRs.

Release Notes

Configuration

📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

♻ Rebasing: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 Ignore: Close this MR and you won't be reminded about this update again.

• If you want to rebase/retry this MR, check this box

This MR has been generated by Renovate Bot.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request

Jun 16, 2025

Pkgsrc changes:

• Drop support for building now old 1.76.0 rust natively on 32-bit NetBSD arm ports, pushing those to use the rust-bin variant instead.
• Use of newer GCC on NetBSD/powerpc turned out to not be required, given that your kernel and user-land is new enough. >= 10.0 release?
• Checksum updates.

Upstream changes:

Version 1.86.0 (2025-04-03)

Language

• [Stabilize upcasting trait objects to supertraits.] (rust-lang/rust#134367)
• [Allow safe functions to be marked with the #[target_feature] attribute.] (rust-lang/rust#134090)
• [The missing_abi lint now warns-by-default.] (rust-lang/rust#132397)
• Rust now lints about double negations, to catch cases that might have intended to be a prefix decrement operator (--x) as written in other languages. This was previously a clippy lint, clippy::double_neg, and is [now available directly in Rust as double_negations.] (rust-lang/rust#126604)
• [More pointers are now detected as definitely not-null based on their alignment in const eval.] (rust-lang/rust#133700)
• [Empty repr() attribute applied to invalid items are now correctly rejected.] (rust-lang/rust#133925)
• [Inner attributes #![test] and #![rustfmt::skip] are no longer accepted in more places than intended.] (rust-lang/rust#134276)

Compiler

• [Debug-assert that raw pointers are non-null on access.] (rust-lang/rust#134424)
• [Change -O to mean -C opt-level=3 instead of -C opt-level=2 to match Cargo's defaults.] (rust-lang/rust#135439)
• [Fix emission of overflowing_literals under certain macro environments.] (rust-lang/rust#136393)

Platform Support

• [Replace i686-unknown-redox target with i586-unknown-redox.] (rust-lang/rust#136698)
• [Increase baseline CPU of i686-unknown-hurd-gnu to Pentium 4.] (rust-lang/rust#136700)
• New tier 3 targets:
  • [{aarch64-unknown,x86_64-pc}-nto-qnx710_iosock] (rust-lang/rust#133631). For supporting Neutrino QNX 7.1 with io-socket network stack.
  • [{aarch64-unknown,x86_64-pc}-nto-qnx800] (rust-lang/rust#133631). For supporting Neutrino QNX 8.0 (no_std-only).
  • [{x86_64,i686}-win7-windows-gnu] (rust-lang/rust#134609). Intended for backwards compatibility with Windows 7. {x86_64,i686}-win7-windows-msvc are the Windows MSVC counterparts that already exist as Tier 3 targets.
  • amdgcn-amd-amdhsa.
  • x86_64-pc-cygwin.
  • [{mips,mipsel}-mti-none-elf] (rust-lang/rust#135074). Initial bare-metal support.
  • m68k-unknown-none-elf.
  • [armv7a-nuttx-{eabi,eabihf}, aarch64-unknown-nuttx, and thumbv7a-nuttx-{eabi,eabihf}] (rust-lang/rust#135757).

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

• The type of FromBytesWithNulError in CStr::from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, FromBytesWithNulError> was [changed from an opaque struct to an enum] (rust-lang/rust#134143), allowing users to examine why the conversion failed.
• [Remove RustcDecodable and RustcEncodable.] (rust-lang/rust#134272)
• [Deprecate libtest's --logfile option.] (rust-lang/rust#134283)
• [On recent versions of Windows, std::fs::remove_file will now remove read-only files.] (rust-lang/rust#134679)

Stabilized APIs

• [{float}::next_down] (https://doc.rust-lang.org/stable/std/primitive.f64.html#method.next_down)
• [{float}::next_up] (https://doc.rust-lang.org/stable/std/primitive.f64.html#method.next_up)
• [<[_]>::get_disjoint_mut] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.get_disjoint_mut)
• [<[_]>::get_disjoint_unchecked_mut] (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.get_disjoint_unchecked_mut)
• [slice::GetDisjointMutError] (https://doc.rust-lang.org/stable/std/slice/enum.GetDisjointMutError.html)
• [HashMap::get_disjoint_mut] (https://doc.rust-lang.org/std/collections/hash_map/struct.HashMap.html#method.get_disjoint_mut)
• [HashMap::get_disjoint_unchecked_mut] (https://doc.rust-lang.org/std/collections/hash_map/struct.HashMap.html#method.get_disjoint_unchecked_mut)
• [NonZero::count_ones] (https://doc.rust-lang.org/stable/std/num/struct.NonZero.html#method.count_ones)
• [Vec::pop_if] (https://doc.rust-lang.org/std/vec/struct.Vec.html#method.pop_if)
• [sync::Once::wait] (https://doc.rust-lang.org/stable/std/sync/struct.Once.html#method.wait)
• [sync::Once::wait_force] (https://doc.rust-lang.org/stable/std/sync/struct.Once.html#method.wait_force)
• [sync::OnceLock::wait] (https://doc.rust-lang.org/stable/std/sync/struct.OnceLock.html#method.wait)

These APIs are now stable in const contexts:

• [hint::black_box] (https://doc.rust-lang.org/stable/std/hint/fn.black_box.html)
• [io::Cursor::get_mut] (https://doc.rust-lang.org/stable/std/io/struct.Cursor.html#method.get_mut)
• [io::Cursor::set_position] (https://doc.rust-lang.org/stable/std/io/struct.Cursor.html#method.set_position)
• [str::is_char_boundary] (https://doc.rust-lang.org/stable/std/primitive.str.html#method.is_char_boundary)
• [str::split_at] (https://doc.rust-lang.org/stable/std/primitive.str.html#method.split_at)
• [str::split_at_checked] (https://doc.rust-lang.org/stable/std/primitive.str.html#method.split_at_checked)
• [str::split_at_mut] (https://doc.rust-lang.org/stable/std/primitive.str.html#method.split_at_mut)
• [str::split_at_mut_checked] (https://doc.rust-lang.org/stable/std/primitive.str.html#method.split_at_mut_checked)

Cargo

• [When merging, replace rather than combine configuration keys that refer to a program path and its arguments.] (rust-lang/cargo#15066)
• [Error if both --package and --workspace are passed but the requested package is missing.] (rust-lang/cargo#15071) This was previously silently ignored, which was considered a bug since missing packages should be reported.
• [Deprecate the token argument in cargo login to avoid shell history leaks.] (rust-lang/cargo#15057)
• [Simplify the implementation of SourceID comparisons.] (rust-lang/cargo#14980) This may potentially change behavior if the canonicalized URL compares differently in alternative registries.

Rustdoc

• [Add a sans-serif font setting.] (rust-lang/rust#133636)

Compatibility Notes

• [The wasm_c_abi future compatibility warning is now a hard error.] (rust-lang/rust#133951) Users of wasm-bindgen should upgrade to at least version 0.2.89, otherwise compilation will fail.
• [Remove long-deprecated no-op attributes #![no_start] and #![crate_id].] (rust-lang/rust#134300)
• [The future incompatibility lint cenum_impl_drop_cast has been made into a hard error.] (rust-lang/rust#135964) This means it is now an error to cast a field-less enum to an integer if the enum implements Drop.
• [SSE2 is now required for "i686" 32-bit x86 hard-float targets; disabling it causes a warning that will become a hard error eventually.] (rust-lang/rust#137037) To compile for pre-SSE2 32-bit x86, use a "i586" target instead.

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

• [Build the rustc on AArch64 Linux with ThinLTO + PGO.] (rust-lang/rust#133807) The ARM 64-bit compiler (AArch64) on Linux is now optimized with ThinLTO and PGO, similar to the optimizations we have already performed for the x86-64 compiler on Linux. This should make it up to 30% faster.