Stream: t-compiler/major changes

Topic: TypeVisitor: use ops::ControlFlow instead… compiler-team#374


triagebot (Oct 20 2020 at 13:18, on Zulip):

A new proposal has been announced: TypeVisitor: use ops::ControlFlow instead of bool #374. It will be announced at the next meeting to try and draw attention to it, but usually MCPs are not discussed during triage meetings. If you think this would benefit from discussion amongst the team, consider proposing a design meeting.

triagebot (Oct 20 2020 at 13:21, on Zulip):

@T-compiler: Proposal #374 has been seconded, and will be approved in 10 days if no objections are raised.

Jonas Schievink (Oct 20 2020 at 13:22, on Zulip):

Seems reasonable, I get confused about what the bool means all the time

oli (Oct 20 2020 at 13:22, on Zulip):

yea, I'm all for using an explicit enum

oli (Oct 20 2020 at 13:23, on Zulip):

The only change that we could do is create ZSTs other than () that convey their meaning via their name

oli (Oct 20 2020 at 13:23, on Zulip):

like not even associated types, but just two global Zsts that we'd use

lcnr (Oct 20 2020 at 13:23, on Zulip):

what do you mean here?

lcnr (Oct 20 2020 at 13:24, on Zulip):

rn we can use both ControlFlow::BREAK and ControlFlow::CONTINUE

oli (Oct 20 2020 at 13:24, on Zulip):

:face_palm: yea, don't mind me... I forgot the () are just extra data

lcnr (Oct 20 2020 at 13:26, on Zulip):

a bit unrelated, but for #77307 it would be nice to exit the mir::Visitor early, is there a reason we do not allow early exits there?

oli (Oct 20 2020 at 13:28, on Zulip):

visitors generally visit everything. I've had this problem before, too. One way would be to make the return value of the visitor a Result with an associated error type

lcnr (Oct 20 2020 at 13:28, on Zulip):

I would have opened an MCP for also using ops::ControlFlow there

lcnr (Oct 20 2020 at 13:29, on Zulip):

if you think that this is sensible

oli (Oct 20 2020 at 13:31, on Zulip):

that is reasonable, I think we could actually make quite a few existing visitors simpler this way

oli (Oct 20 2020 at 13:32, on Zulip):

while having a non-error value to be bubbled up is hard because then you need to combine it and stuff, just the Break value can trivially get bubbled up

Jack Huey (Oct 20 2020 at 17:17, on Zulip):

So, just for reference, one of the things we've been talking about in wg-traits is adopting the Chalk model for Visitor to rustc

Jack Huey (Oct 20 2020 at 17:18, on Zulip):

Interesting, the Visitor in Chalk can have any type of VisitResult

Jack Huey (Oct 20 2020 at 17:18, on Zulip):

VisitResult

Jack Huey (Oct 20 2020 at 17:20, on Zulip):

Using this, there are different types of results, like FindAny or ReturnEarly

LeSeulArtichaut (Oct 20 2020 at 20:45, on Zulip):

@lcnr Do you plan to do the refractor? Do you want me to do it?

lcnr (Oct 20 2020 at 20:47, on Zulip):

hmm, if you are interested in working on this go ahead :thumbs_up: at least this isn't something we might end up having to discard :)

scottmcm (Oct 21 2020 at 01:31, on Zulip):

https://github.com/rust-lang/rust/pull/76614 is open to swap the type arguments so that the future "In the future we may change the used type to std::ops::ControlFlow<(), Self::BreakTy>" would be ControlFlow<Self::BreakTy> instead.

@Leonora Tindall Any status update on that PR? It would be nice to get that in before a whole bunch new usage sites show up...

(Though I guess if this MCP would only use ControlFlow<(), ()> (the control flow Owl) changing the order wouldn't matter.)

scottmcm (Oct 21 2020 at 01:40, on Zulip):

Aside: there were some conversations (pr thread, zulip link) about whether ControlFlow::BREAK and ControlFlow::CONTINUE were the best way of exposing the () versions. If anyone as part of using this develops opinions about that (@lcnr?), please let the tracking issue know (#75744).

scottmcm (Oct 21 2020 at 01:42, on Zulip):

(In the compiler you can even use try{} instead of ControlFlow::Continue(())...)

LeSeulArtichaut (Oct 21 2020 at 12:52, on Zulip):

I opened #78182 to implement the MCP.

LeSeulArtichaut (Oct 21 2020 at 12:53, on Zulip):

scottmcm said:

https://github.com/rust-lang/rust/pull/76614 is open to swap the type arguments so that the future "In the future we may change the used type to std::ops::ControlFlow<(), Self::BreakTy>" would be ControlFlow<Self::BreakTy> instead.

Leonora Tindall Any status update on that PR? It would be nice to get that in before a whole bunch new usage sites show up...

(Though I guess if this MCP would only use ControlFlow<(), ()> (the control flow Owl) changing the order wouldn't matter.)

Also a simple find and replace should be enough to change ControlFlow<(), ()> to ControlFlow<()>

triagebot (Oct 30 2020 at 18:02, on Zulip):

This proposal has been accepted: #374.

Last update: May 07 2021 at 07:00UTC