Introduce `TypeVisitor::BreakTy` #78779

LeSeulArtichaut · 2020-11-05T17:14:57Z

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

~~Blocked on FCP in rust-lang/compiler-team#383.~~

scottmcm · 2020-11-06T06:29:13Z

compiler/rustc_trait_selection/src/traits/structural_match.rs

@@ -227,8 +217,7 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> {

        if !self.type_marked_structural(ty) {
            debug!("Search found ty: {:?}", ty);
-            self.found = Some(NonStructuralMatchTy::Adt(&adt_def));
-            return ControlFlow::BREAK;
+            return ControlFlow::Break(NonStructuralMatchTy::Adt(&adt_def));


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

oli-obk · 2020-11-06T07:01:36Z

compiler/rustc_middle/src/ty/fold.rs

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

    fn has_type_flags(&self, flags: TypeFlags) -> bool {
-        self.visit_with(&mut HasTypeFlagsVisitor { flags }).is_break()
+        self.visit_with(&mut HasTypeFlagsVisitor { flags }).break_value() == Some(FoundFlags)


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

(This was #78182 (comment))

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

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

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.

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?)

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

65cdc21 🙂

compiler/rustc_middle/src/ty/fold.rs

LeSeulArtichaut · 2020-11-06T18:20:31Z

r? @oli-obk

lcnr · 2020-11-06T21:48:13Z

compiler/rustc_middle/src/ty/fold.rs

@@ -964,14 +993,16 @@ impl LateBoundRegionsCollector {
 }

 impl<'tcx> TypeVisitor<'tcx> for LateBoundRegionsCollector {
-    fn visit_binder<T: TypeFoldable<'tcx>>(&mut self, t: &Binder<T>) -> ControlFlow<()> {
+    type BreakTy = !;


compiler/rustc_trait_selection/src/traits/const_evaluatable.rs

lcnr · 2020-11-06T21:54:04Z

compiler/rustc_trait_selection/src/traits/structural_match.rs

        debug!("Search visiting ty: {:?}", ty);

        let (adt_def, substs) = match *ty.kind() {
            ty::Adt(adt_def, substs) => (adt_def, substs),
            ty::Param(_) => {
-                self.found = Some(NonStructuralMatchTy::Param);
-                return ControlFlow::BREAK;
+                return ControlFlow::Break(NonStructuralMatchTy::Param);


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

compiler/rustc_trait_selection/src/traits/structural_match.rs

lcnr · 2020-11-06T21:57:53Z

compiler/rustc_typeck/src/check/check.rs

            debug!("check_opaque_for_inheriting_lifetimes: (visit_ty) t={:?}", t);
            if t != self.opaque_identity_ty && t.super_visit_with(self).is_break() {
-                self.ty = Some(t);
-                return ControlFlow::BREAK;
+                return ControlFlow::Break(Some(t));


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

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

lcnr · 2020-11-06T22:00:02Z

compiler/rustc_typeck/src/check/check.rs

        debug!(
            "check_opaque_for_inheriting_lifetimes: prohibit_opaque={:?}, visitor={:?}",
            prohibit_opaque, visitor
        );

-        if prohibit_opaque {
+        if let Some(ty) = prohibit_opaque.break_value() {


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 🤔

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

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

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 · 2020-11-06T22:23:16Z

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

oli-obk · 2020-11-09T16:19:48Z

compiler/rustc_middle/src/ty/fold.rs

    fn visit_ty(&mut self, t: Ty<'_>) -> ControlFlow<Self::BreakTy> {
        debug!(
            "HasTypeFlagsVisitor: t={:?} t.flags={:?} self.flags={:?}",
            t,
            t.flags(),
            self.flags
        );
-        if t.flags().intersects(self.flags) { ControlFlow::BREAK } else { ControlFlow::CONTINUE }
+        if t.flags().intersects(self.flags) {


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

oli-obk · 2020-11-15T12:49:50Z

r=me,lcnr with the dead function removed

LeSeulArtichaut · 2020-11-15T13:49:44Z

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 · 2020-11-15T13:53:26Z

@bors r+

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

bors · 2020-11-15T13:53:27Z

📌 Commit f6e6a15 has been approved by oli-obk

LeSeulArtichaut · 2020-11-15T13:56:04Z

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

oli-obk · 2020-11-15T14:03:17Z

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

@bors rollup=iffy

bors · 2020-11-16T05:41:24Z

⌛ Testing commit f6e6a15 with merge aacb816092fb150a8459de3135f62d6199fedf62...

bors · 2020-11-16T09:56:23Z

💥 Test timed out

oli-obk · 2020-11-16T10:15:32Z

@bors retry

…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.~~

bors · 2020-11-17T12:24:40Z

⌛ Testing commit f6e6a15 with merge e0ef0fc...

bors · 2020-11-17T14:43:31Z

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

…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.~~

LeSeulArtichaut marked this pull request as draft November 5, 2020 17:15

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

LeSeulArtichaut force-pushed the ty-visitor-return branch from 81f4cb3 to b52a299 Compare November 5, 2020 19:13

scottmcm reviewed Nov 6, 2020

View reviewed changes

oli-obk reviewed Nov 6, 2020

View reviewed changes

LeSeulArtichaut force-pushed the ty-visitor-return branch from b52a299 to 6995765 Compare November 6, 2020 16:29

LeSeulArtichaut marked this pull request as ready for review November 6, 2020 18:14

LeSeulArtichaut added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 6, 2020

rust-highfive assigned oli-obk Nov 6, 2020

LeSeulArtichaut added the S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. label Nov 6, 2020

lcnr reviewed Nov 6, 2020

View reviewed changes

compiler/rustc_trait_selection/src/traits/const_evaluatable.rs Outdated Show resolved Hide resolved

lcnr reviewed Nov 6, 2020

View reviewed changes

compiler/rustc_trait_selection/src/traits/const_evaluatable.rs Outdated Show resolved Hide resolved

lcnr reviewed Nov 6, 2020

View reviewed changes

compiler/rustc_trait_selection/src/traits/structural_match.rs Outdated Show resolved Hide resolved

lcnr reviewed Nov 6, 2020

View reviewed changes

lcnr approved these changes Nov 6, 2020

View reviewed changes

LeSeulArtichaut force-pushed the ty-visitor-return branch 2 times, most recently from df22308 to a815e9e Compare November 6, 2020 22:46

oli-obk reviewed Nov 9, 2020

View reviewed changes

LeSeulArtichaut added 6 commits November 14, 2020 20:25

Introduce TypeVisitor::BreakTy

e0f3119

Use TypeVisitor::BreakTy in structural_match::Search

17b395d

Use TypeVisitor::BreakTy in ProhibitOpaqueVisitor

23feec3

Use TypeVisitor::BreakTy in HasTypeFlagsVisitor

29b140a

Use TypeVisitor::BreakTy in HasEscapingVarsVisitor

44f7d8f

Use TypeVisitor::BreakTy in UnresolvedTypeFinder

df6e87c

Use TypeVisitor::BreakTy in ProhibitOpaqueTypes

07b37cf

Remove dead TypeFoldable::visit_tys_shallow method

f6e6a15

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 mentioned this pull request Nov 15, 2020

Introduce Visitor::BreakTy rust-lang/chalk#651

Merged

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

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

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 mentioned this pull request Nov 17, 2020

Rollup of 9 pull requests #79122

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 17, 2020

bors merged commit e0ef0fc into rust-lang:master Nov 17, 2020

rustbot added this to the 1.50.0 milestone Nov 17, 2020

LeSeulArtichaut deleted the ty-visitor-return branch November 17, 2020 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `TypeVisitor::BreakTy` #78779

Introduce `TypeVisitor::BreakTy` #78779

LeSeulArtichaut commented Nov 5, 2020 •

edited

Loading

scottmcm Nov 6, 2020

LeSeulArtichaut Nov 6, 2020

oli-obk Nov 6, 2020

LeSeulArtichaut Nov 6, 2020 •

edited

Loading

oli-obk Nov 6, 2020

lcnr Nov 6, 2020

oli-obk Nov 9, 2020

oli-obk Nov 9, 2020

lcnr Nov 9, 2020 •

edited

Loading

LeSeulArtichaut Nov 14, 2020

LeSeulArtichaut commented Nov 6, 2020

lcnr Nov 6, 2020

lcnr Nov 6, 2020

lcnr Nov 6, 2020

lcnr Nov 6, 2020

lcnr Nov 6, 2020

LeSeulArtichaut Nov 6, 2020

lcnr Nov 6, 2020 •

edited

Loading

lcnr left a comment •

edited

Loading

LeSeulArtichaut commented Nov 6, 2020

oli-obk Nov 9, 2020 •

edited

Loading

oli-obk commented Nov 15, 2020

LeSeulArtichaut commented Nov 15, 2020

oli-obk commented Nov 15, 2020

bors commented Nov 15, 2020

LeSeulArtichaut commented Nov 15, 2020

oli-obk commented Nov 15, 2020

bors commented Nov 16, 2020

bors commented Nov 16, 2020

oli-obk commented Nov 16, 2020

bors commented Nov 17, 2020

bors commented Nov 17, 2020

Introduce TypeVisitor::BreakTy #78779

Introduce TypeVisitor::BreakTy #78779

Conversation

LeSeulArtichaut commented Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeSeulArtichaut Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeSeulArtichaut commented Nov 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

lcnr left a comment • edited Loading

Choose a reason for hiding this comment

LeSeulArtichaut commented Nov 6, 2020

oli-obk Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

oli-obk commented Nov 15, 2020

LeSeulArtichaut commented Nov 15, 2020

oli-obk commented Nov 15, 2020

bors commented Nov 15, 2020

LeSeulArtichaut commented Nov 15, 2020

oli-obk commented Nov 15, 2020

bors commented Nov 16, 2020

bors commented Nov 16, 2020

oli-obk commented Nov 16, 2020

bors commented Nov 17, 2020

bors commented Nov 17, 2020

Introduce `TypeVisitor::BreakTy` #78779

Introduce `TypeVisitor::BreakTy` #78779

LeSeulArtichaut commented Nov 5, 2020 •

edited

Loading

LeSeulArtichaut Nov 6, 2020 •

edited

Loading

lcnr Nov 9, 2020 •

edited

Loading

lcnr Nov 6, 2020 •

edited

Loading

lcnr left a comment •

edited

Loading

oli-obk Nov 9, 2020 •

edited

Loading