Skip to content

Cleanup: Use rustc's same_type for our same_tys#5528

Merged
bors merged 1 commit intorust-lang:masterfrom
phansch:same-tys
Jun 6, 2020
Merged

Cleanup: Use rustc's same_type for our same_tys#5528
bors merged 1 commit intorust-lang:masterfrom
phansch:same-tys

Conversation

@phansch
Copy link
Contributor

@phansch phansch commented Apr 25, 2020

This delegates our same_tys to ty::TyS::same_type to
remove some duplication.

Our same_tys was introduced 4 years ago in #730. Before, it was
building an inference context to be able to call
can_eq to compare the types. The rustc-dev-guide has some more details
about can_eq in Type Inference -> Trying equality.

Now, using the rustc function, we are essentially comparing the DefIds
of the given types, which also makes more sense, IMO.

I also confirmed that the FIXME is resolved via a bit of dbg!, however
no UI tests seem to have been affected.

changelog: none

@phansch
Copy link
Contributor Author

phansch commented Apr 25, 2020

Ah, it might make sense to also check whether or not the FIXME was resolved already without this PR. Will do that tomorrow.

@phansch phansch added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Apr 25, 2020
flip1995
flip1995 reviewed Apr 25, 2020
View reviewed changes
@phansch phansch force-pushed the same-tys branch 2 times, most recently from bed9d9e to 3cd7608 Compare April 26, 2020 15:21
@phansch phansch changed the title Cleanup: Use rustc's same_types for our same_tys Cleanup: Use rustc's same_type for our same_tys Apr 26, 2020
@phansch
Copy link
Contributor Author

phansch commented Apr 26, 2020

Confirmed that the FIXME was already resolved without these changes:
Selection_078

I removed our same_tys and updated all call sites to use the rustc function instead

@flip1995
Copy link
Member

flip1995 commented Apr 27, 2020

Hmm, generally TyS should never be used outside of the ty module. This should be enforced by an internal rustc lint. But this lint is one of the lints, I wanted to fix for quite some time. Also same_type would have to be re-exported to prevent the usage of TyS in Clippy.

@phansch
Copy link
Contributor Author

phansch commented Apr 27, 2020

Hmm, generally TyS should never be used outside of the ty module.

A right, I'm going to give re-exporting a try later today 👍

@bors
Copy link
Contributor

bors commented May 14, 2020

☔ The latest upstream changes (presumably #5583) made this pull request unmergeable. Please resolve the merge conflicts.

@flip1995 flip1995 self-assigned this May 25, 2020
@flip1995
Copy link
Member

flip1995 commented May 25, 2020

@phansch Did you have any success with reexporting?

@phansch
Copy link
Contributor Author

phansch commented Jun 5, 2020

@phansch Did you have any success with reexporting?

Not really. same_type is the only associated function of TyS. Everything else is a method and accessible via instances of Ty. As far as I can tell the function would either have to be turned into a method or moved out of TyS?

@flip1995
Copy link
Member

flip1995 commented Jun 5, 2020

As far as I can tell the function would either have to be turned into a method or moved out of TyS?

We can just keep it as it is right now, since the internal rustc lint doesn't trigger here. So I would be fine with this right now. (This again reminds me, that I wanted to revisit this internal rustc lint...)

@flip1995
Copy link
Member

flip1995 commented Jun 5, 2020

so rebase + CI pass = r=me

@phansch
Copy link
Contributor Author

phansch commented Jun 6, 2020

@bors r=flip1995

@bors
Copy link
Contributor

bors commented Jun 6, 2020

📌 Commit 623faac has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jun 6, 2020

⌛ Testing commit 623faac with merge e6f05ad...

@bors
Copy link
Contributor

bors commented Jun 6, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing e6f05ad to master...

@bors bors merged commit e6f05ad into rust-lang:master Jun 6, 2020
@phansch phansch deleted the same-tys branch July 5, 2020 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flip1995 flip1995 flip1995 left review comments

Assignees

@flip1995 flip1995

Labels

S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@phansch @flip1995 @bors