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

Relate receiver invariantly in method probe for Mode::Path #129073

Merged
merged 1 commit into from
Sep 17, 2024
Merged

Relate receiver invariantly in method probe for Mode::Path #129073

merged 1 commit into from
Sep 17, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Aug 14, 2024 •
edited
Loading

Effectively reverts part of #126128
Fixes #126227

This PR changes method probing to use equality for fully path-based method lookup, and subtyping for receiver . method lookup.

r? lcnr

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 14, 2024
@lcnr
Copy link
Contributor

lcnr commented Aug 14, 2024

should do a crater run for this

@lcnr lcnr added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 14, 2024
@compiler-errors
Copy link
Member Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 14, 2024
@bors
Auto merge of rust-lang#129073 - compiler-errors:receiver-variance, r…
774bd5b 
…=<try>

Relate receiver invariantly in method probe for `Mode::Path`

Reverts rust-lang#126128
Fixes the part of rust-lang#122317 which always used subtyping instead of eq
Fixes rust-lang#126227 (TODO: test)

WIP: description

r? lcnr
@bors
Copy link
Contributor

bors commented Aug 14, 2024

⌛ Trying commit f4e027c with merge 774bd5b...

@bors
Copy link
Contributor

bors commented Aug 14, 2024

☀️ Try build successful - checks-actions
Build commit: 774bd5b (774bd5ba31b89dab12d58c29690ad9f7e5c9f063)

@lcnr
Copy link
Contributor

lcnr commented Aug 15, 2024

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-129073 created and queued.
🤖 Automatically detected try build 774bd5b
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2024
@craterbot
Copy link
Collaborator

🚧 Experiment pr-129073 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-129073 is completed!
📊 36 regressed and 5 fixed (500916 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Aug 19, 2024
@compiler-errors
Copy link
Member Author

@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-129073/retry-regressed-list.txt p=1

@craterbot
Copy link
Collaborator

👌 Experiment pr-129073-1 created and queued.
🤖 Automatically detected try build 774bd5b
⚠️ Try build based on commit f4e027c, but latest commit is 14a5410. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2024
@craterbot
Copy link
Collaborator

🚧 Experiment pr-129073-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-129073-1 is completed!
📊 0 regressed and 0 fixed (36 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Aug 22, 2024
@compiler-errors compiler-errors marked this pull request as ready for review August 26, 2024 22:18
@rfcbot
Copy link

rfcbot commented Aug 27, 2024 •
edited by BoxyUwU
Loading

Team member @lcnr 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 Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 27, 2024
@compiler-errors
Copy link
Member Author

@lcnr: Sorry. I thought that PR had caused the regression, but turns out that I had " fixed" it by making it eq then subsequently used sup because of regressions in . method dispatch: 8995c2c.

It was sup all along. So the state of the world before and after #122317 was the same, and the state of the world after this PR is objectively better.

@lcnr lcnr mentioned this pull request Sep 3, 2024
Open
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Sep 5, 2024
@rfcbot
Copy link

rfcbot commented Sep 5, 2024

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

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 15, 2024
@rfcbot
Copy link

rfcbot commented Sep 15, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rustbot rustbot mentioned this pull request Sep 15, 2024
Open
3 tasks
@workingjubilee
Copy link
Member

workingjubilee commented Sep 15, 2024 •
edited
Loading

It seems that this sample code started compiling in 1.0, stopped compiling in 1.54, started compiling again in 1.63, and will stop compiling again in 1.83:
https://rust.godbolt.org/z/4bxY7Gr4j

@lcnr
Copy link
Contributor

lcnr commented Sep 17, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Sep 17, 2024

📌 Commit 05483d5 has been approved by lcnr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 17, 2024
fee1-dead added a commit to fee1-dead-contrib/rust that referenced this pull request Sep 17, 2024
@fee1-dead
Rollup merge of rust-lang#129073 - compiler-errors:receiver-variance,…
1f1beb6 
… r=lcnr

Relate receiver invariantly in method probe for `Mode::Path`

Effectively reverts part of rust-lang#126128
Fixes rust-lang#126227

This PR changes method probing to use equality for fully path-based method lookup, and subtyping for receiver `.` method lookup.

r? lcnr
@fee1-dead fee1-dead mentioned this pull request Sep 17, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2024
@bors
Auto merge of rust-lang#130461 - fee1-dead-contrib:rollup-lbf8gqt, r=…
466c6fd 
…fee1-dead

Rollup of 8 pull requests

Successful merges:

 - rust-lang#128961 (Fix rust-lang#128930: Print documentation of CLI options missing their arg)
 - rust-lang#129073 (Relate receiver invariantly in method probe for `Mode::Path`)
 - rust-lang#129674 (Add new_cyclic_in for Rc and Arc)
 - rust-lang#130201 (Encode `coroutine_by_move_body_def_id` in crate metadata)
 - rust-lang#130275 (Don't call `extern_crate` when local crate name is the same as a dependency and we have a trait error)
 - rust-lang#130440 (Don't ICE in `opaque_hidden_inferred_bound` lint for RPITIT in trait with no default method body)
 - rust-lang#130454 (tests: allow trunc/select instructions to be missing)
 - rust-lang#130458 (`rustc_codegen_ssa` cleanups)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Sep 17, 2024

⌛ Testing commit 05483d5 with merge e9e13a6...

@bors
Copy link
Contributor

bors commented Sep 17, 2024

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing e9e13a6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 17, 2024
@bors bors merged commit e9e13a6 into rust-lang:master Sep 17, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 17, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e9e13a6): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 0.7%, secondary -2.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.7% [0.7%, 0.7%] 1
Regressions ❌
(secondary)		 2.8% [2.8%, 2.8%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -3.9% [-4.0%, -3.8%] 3
All ❌✅ (primary) 0.7% [0.7%, 0.7%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 767.973s -> 767.732s (-0.03%)
Artifact size: 341.29 MiB -> 341.31 MiB (0.01%)

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr left review comments

Assignees

@lcnr lcnr

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.83.0
Development

Successfully merging this pull request may close these issues.

inherent method selection uses subtyping even when using paths
9 participants
@compiler-errors @lcnr @bors @craterbot @rfcbot @workingjubilee @rust-timer @apiraino @rustbot