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.

Sign up for GitHub

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

Jump to bottom

simplify SourceID Ord/Eq #14980

Merged
merged 2 commits into from
Jan 7, 2025
Merged

simplify SourceID Ord/Eq #14980

merged 2 commits into from
Jan 7, 2025

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Dec 24, 2024 •
edited
Loading

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.

@rustbot
Copy link
Collaborator

rustbot commented Dec 24, 2024

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 24, 2024
@weihanglo
Copy link
Member

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 Eh2406 marked this pull request as ready for review December 24, 2024 21:53
@weihanglo weihanglo added the T-cargo Team: Cargo label Dec 24, 2024
@weihanglo
Copy link
Member

@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!

@rfcbot
Copy link
Collaborator

rfcbot commented Dec 24, 2024 •
edited by arlosi
Loading

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

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.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Dec 24, 2024
@Eh2406
Copy link
Contributor Author

Eh2406 commented Dec 24, 2024

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.

@Rustin170506
Copy link
Member

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?

@weihanglo
Copy link
Member

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?

@Eh2406
Copy link
Contributor Author

Eh2406 commented Dec 30, 2024

I only speak English. So I've no idea how realistic these examples are. But some googling found this list https://stackoverflow.com/questions/67106784/why-is-upper-casing-not-enough-for-case-insensitive-comparison and with the help of some intelligent auto complete here are sum reproducers https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f4bc9bc63fa18a63f2e503442521e82c

@rfcbot rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Jan 7, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Jan 7, 2025

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

weihanglo
weihanglo approved these changes Jan 7, 2025
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

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.

@weihanglo weihanglo added this pull request to the merge queue Jan 7, 2025
Merged via the queue into rust-lang:master with commit b696870 Jan 7, 2025
20 checks passed
@Eh2406 Eh2406 deleted the EqSourceId branch January 7, 2025 20:14
@weihanglo weihanglo mentioned this pull request Jan 10, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 11, 2025
@bors
Auto merge of rust-lang#135346 - weihanglo:update-cargo, r=weihanglo
7648efa 
Update cargo

18 commits in fd784878cfa843e3e29a6654ecf564c62fae6735..088d496082726091024f1689c124a0c3dccbd775
2025-01-03 20:06:26 +0000 to 2025-01-10 20:10:21 +0000
- docs(reference): Fix PkgIdSpec kind docs (rust-lang/cargo#15049)
- feat: Added warning when failing to update index cache (rust-lang/cargo#15014)
- docs(ref): Fix the inverted logic about MSRV (rust-lang/cargo#15044)
- chore(deps): update msrv (1 version) to v1.84 (rust-lang/cargo#15041)
- Remove unnecessary into conversions (rust-lang/cargo#15042)
- docs(contrib): Start guidelines for schema design (rust-lang/cargo#15037)
- fix: emit warnings as warnings when learning rust target info (rust-lang/cargo#15036)
- fix(schemas): Fix the `[lints]` JSON Schema (rust-lang/cargo#15035)
- fix(schemas): Fix 'metadata' JSON Schema (rust-lang/cargo#15033)
- shorten comment on Ord for SourceKind (rust-lang/cargo#15029)
- Make `"C"` explicit in `extern "C"`. (rust-lang/cargo#15034)
- simplify SourceID Ord/Eq (rust-lang/cargo#14980)
- Setup cargo environment for `cargo rustc --print` (rust-lang/cargo#15026)
- Avoid naming variables `str` (rust-lang/cargo#15025)
- Bump to 0.87.0; update changelog (rust-lang/cargo#15022)
- Update libgit2 to 1.9 (rust-lang/cargo#15018)
- Remove condition on RUSTUP_WINDOWS_PATH_ADD_BIN (rust-lang/cargo#15017)
- Fix https::self_signed_should_fail for macos (rust-lang/cargo#15016)
@rustbot rustbot added this to the 1.86.0 milestone Jan 11, 2025
@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Jan 17, 2025
@teor2345 teor2345 mentioned this pull request Jan 25, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jan 25, 2025
@weihanglo
Fix comment on Ord for SourceId (#15103)
e63457a 
### 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels
disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cargo Team: Cargo to-announce
Projects
Cargo status tracker
Archived in project
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Eh2406 @rustbot @weihanglo @rfcbot @Rustin170506