Skip to content

Introduce TypeVisitor::BreakTy#78779

Merged
bors merged 9 commits intorust-lang:masterfrom
LeSeulArtichaut:ty-visitor-return
Nov 17, 2020
Merged

Introduce TypeVisitor::BreakTy#78779
bors merged 9 commits intorust-lang:masterfrom
LeSeulArtichaut:ty-visitor-return

Conversation

@LeSeulArtichaut
Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut commented Nov 5, 2020 •
edited
Loading

@LeSeulArtichaut LeSeulArtichaut marked this pull request as draft November 5, 2020 17:15
@LeSeulArtichaut LeSeulArtichaut added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 5, 2020
scottmcm
scottmcm reviewed Nov 6, 2020
View reviewed changes
compiler/rustc_trait_selection/src/traits/structural_match.rs Outdated
Copy link
Member

@scottmcm scottmcm Nov 6, 2020

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor Author

@LeSeulArtichaut LeSeulArtichaut Nov 6, 2020

Choose a reason for hiding this comment

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

Yeah, I find the code so much cleaner this way 😄

oli-obk
oli-obk reviewed Nov 6, 2020
View reviewed changes
compiler/rustc_middle/src/ty/fold.rs Outdated
Copy link
Contributor

@oli-obk oli-obk Nov 6, 2020

Choose a reason for hiding this comment

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

I don't see the advantage of using a type other than () for the break in this case.

Copy link
Contributor Author

@LeSeulArtichaut LeSeulArtichaut Nov 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

(This was #78182 (comment))

Copy link
Contributor

@oli-obk oli-obk Nov 6, 2020

Choose a reason for hiding this comment

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

Oh right, I remember that comment. @lcnr can you elaborate on the motivation? We should encode it in documentation/comments

Copy link
Contributor

@lcnr lcnr Nov 6, 2020

Choose a reason for hiding this comment

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

Using a ZST we explicitly document the reasons we stop visiting and the reason we check if we terminated early.

While it isn't too relevant, it makes it easier to prevent bugs like stopping early because of a different reason and forgetting to update a call site. I also feel like using a ZST here has a very low cost and removes some questions on why we are using is_break and under which conditions we want to stop visiting.

Don't really care too much about this though

Copy link
Contributor

@oli-obk oli-obk Nov 9, 2020

Choose a reason for hiding this comment

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

I have nothing to argue against that 😆

If we can manage to remove is_break in that case (when all visitors have been migrated), then that would enforce this by there not being a way around it.

Copy link
Contributor

@oli-obk oli-obk Nov 9, 2020

Choose a reason for hiding this comment

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

Oh... now that I finished reading the PR, I think we should also stop having () as a default for BreakTy and require the user to specify one (or use ! as the default?)

Copy link
Contributor

@lcnr lcnr Nov 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

using ! as a default would be interesting 🤔 can't be misused and makes people notice the possibility

Copy link
Contributor Author

@LeSeulArtichaut LeSeulArtichaut Nov 14, 2020

Choose a reason for hiding this comment

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

65cdc21 🙂

@LeSeulArtichaut LeSeulArtichaut marked this pull request as ready for review November 6, 2020 18:14
@LeSeulArtichaut LeSeulArtichaut added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 6, 2020
@LeSeulArtichaut
Copy link
Contributor Author

LeSeulArtichaut commented Nov 6, 2020

r? @oli-obk

@LeSeulArtichaut LeSeulArtichaut added the S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. label Nov 6, 2020
lcnr
lcnr reviewed Nov 6, 2020
View reviewed changes
compiler/rustc_middle/src/ty/fold.rs Outdated
Copy link
Contributor

@lcnr lcnr Nov 6, 2020

Choose a reason for hiding this comment

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

✨

lcnr
lcnr reviewed Nov 6, 2020
View reviewed changes
lcnr
lcnr reviewed Nov 6, 2020
View reviewed changes
lcnr
lcnr reviewed Nov 6, 2020
View reviewed changes
compiler/rustc_trait_selection/src/traits/structural_match.rs Outdated
Copy link
Contributor

@lcnr lcnr Nov 6, 2020

Choose a reason for hiding this comment

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

this is such a huge improvement to the implementation which still used bools ❤️ thank you for implementing all of this

lcnr
lcnr reviewed Nov 6, 2020
View reviewed changes
lcnr
lcnr reviewed Nov 6, 2020
View reviewed changes
compiler/rustc_typeck/src/check/check.rs Outdated
Copy link
Contributor

@lcnr lcnr Nov 6, 2020

Choose a reason for hiding this comment

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

It looks to me like there are actually 2 visitors merged into 1 here.

The first one finds all opaque types and the second one then searches for inherited parent lifetimes, might make sense to try and split this. Not sure if you want to do this in one PR

Copy link
Contributor

@lcnr lcnr Nov 6, 2020

Choose a reason for hiding this comment

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

(depends on https://github.com/rust-lang/rust/pull/78779/files#r519026794)

lcnr
lcnr reviewed Nov 6, 2020
View reviewed changes
compiler/rustc_typeck/src/check/check.rs Outdated
Copy link
Contributor

@lcnr lcnr Nov 6, 2020

Choose a reason for hiding this comment

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

Can ty.unwrap() fail here? I would expect this to not be the case. Might be missing something though.

Maybe if we have a PredicateAtom::RegionOutlives 🤔

Copy link
Contributor Author

@LeSeulArtichaut LeSeulArtichaut Nov 6, 2020

Choose a reason for hiding this comment

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

IIUC it can: https://github.com/rust-lang/rust/blob/149f593bf6d204ea226461c18a95f558a6ad250b/compiler/rustc_typeck/src/check/check.rs#L462-L473

Copy link
Contributor

@lcnr lcnr Nov 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

yeah, but this only ends up returning None if we aren't inside of a type, as we would otherwise return Some(ty) there

edit: this may happen if the predicate is an outlives predicate though I don't know if they are possible for opaque types, don't have the capacity to look into this myself atm

lcnr
lcnr approved these changes Nov 6, 2020
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I personally would like to push a bit more to using ZST here and pretty much never use (). So I wouldn't add a associated type default here.

afaict @oli-obk has a different opinion so I am also fine with merging this as is.

@LeSeulArtichaut
Copy link
Contributor Author

LeSeulArtichaut commented Nov 6, 2020

We could change things in follow-up PRs. I made every change in a separate commit so I can easily drop some.

@LeSeulArtichaut LeSeulArtichaut force-pushed the ty-visitor-return branch 2 times, most recently from df22308 to a815e9e Compare November 6, 2020 22:46
oli-obk
oli-obk reviewed Nov 9, 2020
View reviewed changes
compiler/rustc_middle/src/ty/fold.rs Outdated
Copy link
Contributor

@oli-obk oli-obk Nov 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe create a self.check_flags method that handles this intersects call and the if-condition?

@oli-obk
Copy link
Contributor

oli-obk commented Nov 15, 2020

r=me,lcnr with the dead function removed

@LeSeulArtichaut
Copy link
Contributor Author

LeSeulArtichaut commented Nov 15, 2020

Done.
Note: rust-lang/compiler-team#383 still has two more hours in FCP to be accepted, but I haven't seen any objections anywhere so by the time this lands the MCP should be accepted.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 15, 2020

@bors r+

yea, I think we can safely send it to bors, too

@bors
Copy link
Collaborator

bors commented Nov 15, 2020

📌 Commit f6e6a15 has been approved by oli-obk

@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 Nov 15, 2020
@LeSeulArtichaut
Copy link
Contributor Author

LeSeulArtichaut commented Nov 15, 2020

Should this be rollup=never as a medium-sized refractor? (I'm still trying to understand what deserves rollup=never besides perf-sensitive changes)

@LeSeulArtichaut LeSeulArtichaut mentioned this pull request Nov 15, 2020
Merged
@oli-obk
Copy link
Contributor

oli-obk commented Nov 15, 2020

for non-perf non prioritized things that probably conflict with a bunch of things we have

@bors rollup=iffy

@LeSeulArtichaut LeSeulArtichaut removed the S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. label Nov 15, 2020
@bors
Copy link
Collaborator

bors commented Nov 16, 2020

⌛ Testing commit f6e6a15 with merge aacb816092fb150a8459de3135f62d6199fedf62...

@bors
Copy link
Collaborator

bors commented Nov 16, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 16, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Nov 16, 2020

@bors retry

@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 Nov 16, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 17, 2020
@Dylan-DPC
Rollup merge of rust-lang#78779 - LeSeulArtichaut:ty-visitor-return, …
975b50d 
…r=oli-obk

Introduce `TypeVisitor::BreakTy`

Implements MCP rust-lang/compiler-team#383.
r? `@ghost`
cc `@lcnr` `@oli-obk`

~~Blocked on FCP in rust-lang/compiler-team#383.~~
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Nov 17, 2020
Closed
@bors
Copy link
Collaborator

bors commented Nov 17, 2020

⌛ Testing commit f6e6a15 with merge e0ef0fc...

@bors
Copy link
Collaborator

bors commented Nov 17, 2020

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing e0ef0fc to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 17, 2020
@bors bors merged commit e0ef0fc into rust-lang:master Nov 17, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 17, 2020
@LeSeulArtichaut LeSeulArtichaut deleted the ty-visitor-return branch November 17, 2020 14:54
flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 20, 2020
@bors
Auto merge of rust-lang#78779 - LeSeulArtichaut:ty-visitor-return, r=…
a467c51 
…oli-obk

Introduce `TypeVisitor::BreakTy`

Implements MCP rust-lang/compiler-team#383.
r? `@ghost`
cc `@lcnr` `@oli-obk`

~~Blocked on FCP in rust-lang/compiler-team#383.~~
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oli-obk oli-obk oli-obk left review comments

@scottmcm scottmcm scottmcm left review comments

@lcnr lcnr lcnr approved these changes

Assignees

@oli-obk oli-obk

Labels

C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.50.0

Development

Successfully merging this pull request may close these issues.

6 participants

@LeSeulArtichaut @oli-obk @bors @scottmcm @lcnr @rustbot