Skip to content

refactor builtin unsize handling, extend comments #114169

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 2 commits into from
Jul 31, 2023
Merged

refactor builtin unsize handling, extend comments #114169

merged 2 commits into from
Jul 31, 2023

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Jul 28, 2023

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Jul 28, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 28, 2023

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2023
@rust-log-analyzer

This comment has been minimized.

# to view
lcnr added 2 commits July 28, 2023 13:00
@lcnr
refactor builtin unsize handling, extend comments

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
f1753ff
@lcnr
fix comment
17f87c5
compiler-errors
compiler-errors approved these changes Jul 28, 2023
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

r=me nits or no

compiler/rustc_trait_selection/src/solve/trait_goals.rs
Comment on lines +526 to +529
a_data: &'tcx ty::List<ty::PolyExistentialPredicate<'tcx>>,
a_region: ty::Region<'tcx>,
b_data: &'tcx ty::List<ty::PolyExistentialPredicate<'tcx>>,
b_region: ty::Region<'tcx>,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it costs very much to just destructure ty::Dynamic again and bug!() here instead of passing this data, but I guess it's ok 😅

That is, I'd rather just pass all the data once (in the goal) rather than both as one package (the goal) + partially destructured (a/b data)

compiler/rustc_trait_selection/src/solve/trait_goals.rs
Comment on lines +610 to +611
b_data: &'tcx ty::List<ty::PolyExistentialPredicate<'tcx>>,
b_region: ty::Region<'tcx>,
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@compiler-errors compiler-errors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 28, 2023
@lcnr
Copy link
Contributor Author

lcnr commented Jul 31, 2023

@bors r=compiler-errors rollup

I see where you're coming from but also dislike having to bug!() inside of these functions

@bors
Copy link
Collaborator

bors commented Jul 31, 2023

📌 Commit 17f87c5 has been approved by compiler-errors

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 31, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 31, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 31, 2023
@bors
Auto merge of rust-lang#114294 - matthiaskrgr:rollup-yk78pvd, r=matth…
b3df56a 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#114111 (Improve test case for experimental API remove_matches)
 - rust-lang#114169 (refactor builtin unsize handling, extend comments)
 - rust-lang#114182 (clean up after 113312)
 - rust-lang#114193 (Update lexer emoji diagnostics to Unicode 15.0)
 - rust-lang#114200 (Detect trait upcasting through struct tail unsizing in new solver select)
 - rust-lang#114228 (Check lazy type aliases for well-formedness)
 - rust-lang#114267 (Map RPITIT's opaque type bounds back from projections to opaques)
 - rust-lang#114269 (Migrate GUI colors test to original CSS color format)
 - rust-lang#114286 (Add missing feature gate in multiple_supertrait_upcastable doc)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b8f78fb into rust-lang:master Jul 31, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 31, 2023
@lcnr lcnr deleted the unsize branch July 31, 2023 23:46
@klensy
Copy link
Contributor

klensy commented Aug 3, 2023

While working on #114209 i noticed that consider_builtin_unsize_candidate was changed, but not rematch_unsize which have note that it should be in sync with it consider_builtin_unsize_candidate. Is it was in sync before, now, or it it still not?

@lcnr
Copy link
Contributor Author

lcnr commented Aug 4, 2023

the changes are non-functional so I think it should still be in sync, but I did not think about this explicitly so I am going to take another look.

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@compiler-errors compiler-errors

Assignees

@compiler-errors compiler-errors

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.
6 participants
@lcnr @rustbot @rust-log-analyzer @bors @klensy @compiler-errors