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.

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

TypeVisitor: use std::ops::ControlFlow instead of bool #78182

Merged
merged 6 commits into from
Oct 31, 2020

Conversation

LeSeulArtichaut
Copy link
Contributor

@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. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. labels Oct 21, 2020
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 21, 2020
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

Thanks.

Some nitpicks and suggestion but this is already looking a lot clearer to me ❤️

I do have to admit that my brain ended up skipping over some bool -> ControlFlow conversions so hopefully I didn't accidentially miss anything 😅

compiler/rustc_middle/src/macros.rs Show resolved Hide resolved
}
Assert { ref cond, ref msg, .. } => {
if cond.visit_with(visitor) {
if cond.visit_with(visitor) == ControlFlow::BREAK {
Copy link
Contributor

@lcnr lcnr Oct 21, 2020

Choose a reason for hiding this comment

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

that's weird, why are we only visiting here if cond.visit_with(...) returns true?

Is this a preexisting bug? cc @oli-obk @jonas-schievink maybe

Copy link
Contributor

Choose a reason for hiding this comment

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

yea this looks wrong. I don't know why it would be this way around. I mean this is essentially cond.visit_with(visitor) && {...} which is not something that is ever used anywhere

@@ -74,7 +76,7 @@ pub trait TypeFoldable<'tcx>: fmt::Debug + Clone {
}

fn has_type_flags(&self, flags: TypeFlags) -> bool {
self.visit_with(&mut HasTypeFlagsVisitor { flags })
self.visit_with(&mut HasTypeFlagsVisitor { flags }) == ControlFlow::Break(())
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it might make sense to introduce a newtype here in the future, so this can be visit_with(...).break_value() == Some(FoundFlags).

Not part of this PR though

compiler/rustc_middle/src/ty/fold.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/fold.rs Outdated Show resolved Hide resolved
compiler/rustc_privacy/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_privacy/src/lib.rs Outdated Show resolved Hide resolved
@@ -258,16 +248,16 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> {
let ty = self.tcx().normalize_erasing_regions(ty::ParamEnv::empty(), field_ty);
debug!("structural-match ADT: field_ty={:?}, ty={:?}", field_ty, ty);

if ty.visit_with(self) {
if ty.visit_with(self) == ControlFlow::BREAK {
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, we probably want to store self.found in ControlFlow::Break here.

(also not part of this PR)

@@ -798,26 +800,26 @@ fn check_where_clauses<'tcx, 'fcx>(
params: FxHashSet<u32>,
}
impl<'tcx> ty::fold::TypeVisitor<'tcx> for CountParams {
Copy link
Contributor

Choose a reason for hiding this comment

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

That folder seems weird to me, why are we storing the params in a hashmap if we only care about if there is more than one of them?

Using an option is probably more appropriate here but whatever 🤷 Isn't really relevant for this PR

fn visit_region(&mut self, _: ty::Region<'_>) -> bool {
true
fn visit_region(&mut self, _: ty::Region<'_>) -> ControlFlow<(), ()> {
ControlFlow::BREAK
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that we aren't ignoring ReStatic here.

@LeSeulArtichaut LeSeulArtichaut force-pushed the ty-visitor-contolflow branch 3 times, most recently from 6d4777b to 8e58212 Compare October 21, 2020 15:31
compiler/rustc_middle/src/mir/type_foldable.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/subst.rs Outdated Show resolved Hide resolved
compiler/rustc_mir/src/monomorphize/polymorphize.rs Outdated Show resolved Hide resolved
compiler/rustc_mir/src/monomorphize/polymorphize.rs Outdated Show resolved Hide resolved
compiler/rustc_privacy/src/lib.rs Outdated Show resolved Hide resolved
fn visit_def_id(&mut self, def_id: DefId, kind: &str, descr: &dyn fmt::Display) -> bool {
self.check_def_id(def_id, kind, descr)
fn visit_def_id(&mut self, def_id: DefId, kind: &str, descr: &dyn fmt::Display) -> ControlFlow<(), ()> {
if self.check_def_id(def_id, kind, descr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

these are very common. Maybe we should have a ControlFlow::break_if(some_bool) and ControlFlow::continue_if(some_bool) for convenience? Or will that become infeasible with types other than ()?

Copy link
Contributor

@lcnr lcnr Oct 21, 2020

Choose a reason for hiding this comment

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

yeah, I think adding these methods make sense. I am not yet sure how to generalize them to deal with types other than ControlFlow<(), ()>

Copy link
Member

Choose a reason for hiding this comment

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

There has been almost no thought put into what methods the ControlFlow type should have, so if you come up with reasonable ones definitely add them. It used to be an internal implementation detail, so doesn't even have the usual combinators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scottmcm I'd say definitely is_break and is_continue (name to be bikeshed of course). I'll file a PR

@@ -691,26 +693,26 @@ impl<'tcx, OP> TypeVisitor<'tcx> for ConstrainOpaqueTypeRegionVisitor<OP>
where
OP: FnMut(ty::Region<'tcx>),
{
fn visit_binder<T: TypeFoldable<'tcx>>(&mut self, t: &ty::Binder<T>) -> bool {
fn visit_binder<T: TypeFoldable<'tcx>>(&mut self, t: &ty::Binder<T>) -> ControlFlow<(), ()> {
t.as_ref().skip_binder().visit_with(self);
Copy link
Contributor

Choose a reason for hiding this comment

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

ControlFlow should probably be #[must_use] so we'd be forced to use let _ = here, but that's for another PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, making it #[must_use] seems like a good idea to me

Copy link
Contributor

@lcnr lcnr Oct 21, 2020

Choose a reason for hiding this comment

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

I missed this in my review, but it seems weird to not propagate ControlFlow::Break here, I guess that this visitor never actually wants to break early so it is fine but it does seem strange.

Another reason for custom break types as we could use ! here to make this part of the API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #78202

Copy link
Member

Choose a reason for hiding this comment

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

Another thing you could consider to avoid the conversation on the type would be to just mark the methods on the trait as #[must_use]. It looks like that works even for calls on concrete things that don't copy the attribute to their implementation. Probably still not this PR, though.

debug!("check_opaque_for_inheriting_lifetimes: (visit_ty) t={:?}", t);
if t != self.opaque_identity_ty && t.super_visit_with(self) {
if t != self.opaque_identity_ty && t.super_visit_with(self) == ControlFlow::BREAK {
Copy link
Contributor

Choose a reason for hiding this comment

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

This alos looks like a candidate for returning a value via Break

@oli-obk
Copy link
Contributor

oli-obk commented Oct 21, 2020

This PR is awesome (and exposes so many issues of different sublety

@bors

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 23, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 23, 2020

📌 Commit 901c1f446e22f1d067d2c0948b95fdd5ebe3d39d 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 Oct 23, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Oct 23, 2020

@bors r- I guess we should wait on the FCP 😅

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 23, 2020
@LeSeulArtichaut LeSeulArtichaut removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 23, 2020
@bors

This comment has been minimized.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 24, 2020
@LeSeulArtichaut
Copy link
Contributor Author

@oli-obk @lcnr The MCP finished its 10 days FCP

@LeSeulArtichaut LeSeulArtichaut added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 30, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Oct 30, 2020

@bory r=lcnr,oli-obk

@LeSeulArtichaut

This comment has been minimized.

@lcnr
Copy link
Contributor

lcnr commented Oct 30, 2020

@bors r=lcnr,oli-obk rollup=never

@bors
Copy link
Contributor

bors commented Oct 30, 2020

📌 Commit 9433eb8 has been approved by lcnr,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 Oct 30, 2020
@bors
Copy link
Contributor

bors commented Oct 30, 2020

⌛ Testing commit 9433eb8 with merge 0d033de...

@bors
Copy link
Contributor

bors commented Oct 31, 2020

☀️ Test successful - checks-actions
Approved by: lcnr,oli-obk
Pushing 0d033de to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 31, 2020
@bors bors merged commit 0d033de into rust-lang:master Oct 31, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 31, 2020
@LeSeulArtichaut LeSeulArtichaut deleted the ty-visitor-contolflow branch October 31, 2020 08:14
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 9, 2020
fix `super_visit_with` for `Terminator`

fixes rust-lang#78182 (comment)

r? `@oli-obk`

cc `@LeSeulArtichaut`
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 15, 2021
…m-basics, r=m-ou-se

Stabilize `ops::ControlFlow` (just the type)

Tracking issue: rust-lang#75744 (which also tracks items *not* closed by this PR).

With the new `?` desugar implemented, [it's no longer possible to mix `Result` and `ControlFlow`](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=13feec97f5c96a9d791d97f7de2d49a6).  (At the time of making this PR, godbolt was still on the 2021-05-01 nightly, where you can see that [the mixing example compiled](https://rust.godbolt.org/z/13Ke54j16).)  That resolves the only blocker I know of, so I'd like to propose that `ControlFlow` be considered for stabilization.

Its basic existence was part of rust-lang/rfcs#3058, where it got a bunch of positive comments (examples [1](rust-lang/rfcs#3058 (comment)) [2](rust-lang/rfcs#3058 (review)) [3](rust-lang/rfcs#3058 (comment)) [4](rust-lang/rfcs#3058 (comment))).  Its use in the compiler has been well received (rust-lang#78182 (comment)), and there are ecosystem updates interested in using it (rust-itertools/itertools#469 (comment), jonhoo/rust-imap#194).

As this will need an FCP, picking a libs member manually:
r? `@m-ou-se`

## Stabilized APIs

```rust
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum ControlFlow<B, C = ()> {
    /// Exit the operation without running subsequent phases.
    Break(B),
    /// Move on to the next phase of the operation as normal.
    Continue(C),
}
```

As well as using `?` on a `ControlFlow<B, _>` in a function returning `ControlFlow<B, _>`.  (Note, in particular, that there's no `From::from`-conversion on the `Break` value, the way there is for `Err`s.)

## Existing APIs *not* stabilized here

All the associated methods and constants: `break_value`, `is_continue`, `map_break`, [`CONTINUE`](https://doc.rust-lang.org/nightly/std/ops/enum.ControlFlow.html#associatedconstant.CONTINUE), etc.

Some of the existing methods in nightly seem reasonable, some seem like they should be removed, and some need more discussion to decide.  But none of them are *essential*, so [as in the RFC](https://rust-lang.github.io/rfcs/3058-try-trait-v2.html#methods-on-controlflow), they're all omitted from this PR.

They can be considered separately later, as further usage demonstrates which are important.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants