Refactor fmt::Display and fmt::Debug impls in ppaux #44888

tirr-c · 2017-09-27T16:13:18Z

Also fixes #44887.

There was a problem that unnamed late-bound regions are always named 'r while they are displayed using std::fmt::Display.

fn main() {
    f(|_: (), _: ()| {});
}
fn f<F>(_: F) where F: Fn(&(), for<'r> fn(&'r ())) {}

Before (incorrectly shadows lifetime, for<'r> omitted for the second argument):

error[E0631]: type mismatch in closure arguments
 --> test.rs:2:5
  |
2 |     f(|_: (), _: ()| {});
  |     ^ ----------------- found signature of `fn((), ()) -> _`
  |     |
  |     expected signature of `for<'r> fn(&'r (), fn(&'r ())) -> _`
  |
  = note: required by `f`

After:

error[E0631]: type mismatch in closure arguments
 --> test.rs:2:5
  |
2 |     f(|_: (), _: ()| {});
  |     ^ ----------------- found signature of `fn((), ()) -> _`
  |     |
  |     expected signature of `for<'s> fn(&'s (), for<'r> fn(&'r ())) -> _`
  |
  = note: required by `f`

r? @nikomatsakis

arielb1 · 2017-09-27T16:22:33Z

src/librustc/ty/context.rs

@@ -882,6 +882,10 @@ pub struct GlobalCtxt<'tcx> {
    /// Maps Expr NodeId's to `true` iff `&expr` can have 'static lifetime.
    pub rvalue_promotable_to_static: RefCell<NodeMap<bool>>,

+    pub display_used_late_bound_region_names: RefCell<Option<FxHashSet<Name>>>,


This is likely to cause problems with incremental compilation. I think the amount of globals ppaux is depending on is getting far out of hand, and we should stop using Display & Debug and start using our own trait.

tirr-c · 2017-10-01T17:18:42Z

I've refactored ppaux, and I think it's halfway done. I believe we can make this better, especially those macros; they are very rough. I defined macros because I can't just write impl<T> fmt::Display for T where T: Print.

r? @arielb1

arielb1 · 2017-10-02T09:12:40Z

[00:35:42] ---- [ui] ui/regions-fn-subtyping-return-static.rs stdout ----
[00:35:42] 	normalized stderr:
[00:35:42] error[E0308]: mismatched types
[00:35:42]   --> $DIR/regions-fn-subtyping-return-static.rs:51:12
[00:35:42]    |
[00:35:42] 51 |     want_F(bar); //~ ERROR E0308
[00:35:42]    |            ^^^ expected concrete lifetime, found bound lifetime parameter 'cx
[00:35:42]    |
[00:35:42]    = note: expected type `for<'cx> fn(&'cx S) -> &'cx S`
[00:35:42]               found type `for<'a> fn(&'a S) -> &S {bar::<'_>}`
[00:35:42] 
[00:35:42] error: aborting due to previous error
[00:35:42] 
[00:35:42] 
[00:35:42] 
[00:35:42] expected stderr:
[00:35:42] error[E0308]: mismatched types
[00:35:42]   --> $DIR/regions-fn-subtyping-return-static.rs:51:12
[00:35:42]    |
[00:35:42] 51 |     want_F(bar); //~ ERROR E0308
[00:35:42]    |            ^^^ expected concrete lifetime, found bound lifetime parameter 'cx
[00:35:42]    |
[00:35:42]    = note: expected type `for<'cx> fn(&'cx S) -> &'cx S`
[00:35:42]               found type `fn(&'a S) -> &S {bar::<'_>}`
[00:35:42] 
[00:35:42] error: aborting due to previous error
[00:35:42] 
[00:35:42] 
[00:35:42] 
[00:35:42] diff of stderr:
[00:35:42] 
[00:35:42]  error[E0308]: mismatched types
[00:35:42]    --> $DIR/regions-fn-subtyping-return-static.rs:51:12
[00:35:42]     |
[00:35:42]  51 |     want_F(bar); //~ ERROR E0308
[00:35:42]     |            ^^^ expected concrete lifetime, found bound lifetime parameter 'cx
[00:35:42]     |
[00:35:42]     = note: expected type `for<'cx> fn(&'cx S) -> &'cx S`
[00:35:42] -              found type `fn(&'a S) -> &S {bar::<'_>}`
[00:35:42] +              found type `for<'a> fn(&'a S) -> &S {bar::<'_>}`
[00:35:42]  
[00:35:42]  error: aborting due to previous error

arielb1 · 2017-10-02T09:18:12Z

src/librustc/util/ppaux.rs

-    };
+default impl<T> DefaultWith<T> for PrintContext {}
+
+impl<'tcx, T> DefaultWith<T> for PrintContext


that's an... ok use of specialization. I suppose we can avoid it if we have to?

Oops, I've missed your review -- what do you mean by that? I can't quite get it...

arielb1 · 2017-10-02T11:30:27Z

still same test needs updating

arielb1 · 2017-10-02T12:55:59Z

Nice cleanup with the macros. There were a few directions I wanted to go there, but they can be in a separate PRs.

LGTM, r=me after this passes travis.

tirr-c · 2017-10-02T18:22:56Z

I haven't fully tested this, I'll let travis do necessary tests.

arielb1 · 2017-10-03T10:00:09Z

src/librustc/util/ppaux.rs

+        })
+    }
+    fn prepare_late_bound_region_info<'tcx, T>(&mut self, value: &ty::Binder<T>)
+    where T: TypeFoldable<'tcx>


nit: indentation, should be

fn prepare_late_bound_region_info<'tcx, T>(&mut self, value: &ty::Binder<T>) where T: TypeFoldable<'tcx>

arielb1 · 2017-10-03T10:10:10Z

nah you don't have to fix the indentation

@bors r+

bors · 2017-10-03T10:10:11Z

📌 Commit b9b45ed has been approved by arielb1

arielb1 · 2017-10-03T10:10:29Z

Thanks for the PR! That's a very cool use of macros.

bors · 2017-10-03T10:28:39Z

⌛ Testing commit b9b45ed1a4f1168906f2725a2696e79b07843ef6 with merge c914972d622e7c4f44a62adf06fbba4c30fb5084...

bors · 2017-10-03T12:18:10Z

💔 Test failed - status-travis

bors · 2017-10-08T08:25:04Z

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

Now they don't shadow other lifetimes.

arielb1 · 2017-10-11T13:16:38Z

@bors r+

bors · 2017-10-11T13:16:40Z

📌 Commit 84cb90f has been approved by arielb1

bors · 2017-10-11T16:00:31Z

⌛ Testing commit 84cb90f with merge cbf5d39...

@nikomatsakis

Refactor fmt::Display and fmt::Debug impls in ppaux Also fixes #44887. There was a problem that unnamed late-bound regions are *always* named `'r` while they are displayed using `std::fmt::Display`. --- ```rust fn main() { f(|_: (), _: ()| {}); } fn f<F>(_: F) where F: Fn(&(), for<'r> fn(&'r ())) {} ``` Before (incorrectly shadows lifetime, `for<'r>` omitted for the second argument): ``` error[E0631]: type mismatch in closure arguments --> test.rs:2:5 | 2 | f(|_: (), _: ()| {}); | ^ ----------------- found signature of `fn((), ()) -> _` | | | expected signature of `for<'r> fn(&'r (), fn(&'r ())) -> _` | = note: required by `f` ``` After: ``` error[E0631]: type mismatch in closure arguments --> test.rs:2:5 | 2 | f(|_: (), _: ()| {}); | ^ ----------------- found signature of `fn((), ()) -> _` | | | expected signature of `for<'s> fn(&'s (), for<'r> fn(&'r ())) -> _` | = note: required by `f` ``` r? @nikomatsakis

bors · 2017-10-11T18:33:28Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing cbf5d39 to master...

rust-highfive assigned nikomatsakis Sep 27, 2017

arielb1 reviewed Sep 27, 2017

View reviewed changes

aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 28, 2017

tirr-c force-pushed the binder-hr-region branch 2 times, most recently from 88ced5d to 257bf32 Compare October 1, 2017 16:52

tirr-c changed the title ~~Name higher-ranked lifetimes properly while displaying~~ Refactor fmt::Display and fmt::Debug impls in ppaux Oct 1, 2017

tirr-c force-pushed the binder-hr-region branch from 257bf32 to ba022e0 Compare October 1, 2017 17:12

rust-highfive assigned arielb1 and unassigned nikomatsakis Oct 1, 2017

arielb1 reviewed Oct 2, 2017

View reviewed changes

tirr-c force-pushed the binder-hr-region branch from d83914d to 5a2bcca Compare October 2, 2017 09:21

tirr-c force-pushed the binder-hr-region branch from 5a2bcca to 512603b Compare October 2, 2017 18:21

tirr-c force-pushed the binder-hr-region branch from 512603b to b9b45ed Compare October 3, 2017 07:05

arielb1 reviewed Oct 3, 2017

View reviewed changes

tirr-c force-pushed the binder-hr-region branch from b9b45ed to c11eeb3 Compare October 3, 2017 13:02

tirr-c added 2 commits October 8, 2017 20:53

Name higher-ranked lifetimes properly while displaying

9b0480b

Now they don't shadow other lifetimes.

Introduce Print trait for displaying types

cbc6e90

tirr-c force-pushed the binder-hr-region branch from c11eeb3 to e46bf1c Compare October 8, 2017 12:56

Fix tests

84cb90f

tirr-c force-pushed the binder-hr-region branch from e46bf1c to 84cb90f Compare October 8, 2017 14:52

bors merged commit 84cb90f into rust-lang:master Oct 11, 2017

tirr-c deleted the binder-hr-region branch October 11, 2017 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor fmt::Display and fmt::Debug impls in ppaux #44888

Refactor fmt::Display and fmt::Debug impls in ppaux #44888

tirr-c commented Sep 27, 2017

arielb1 Sep 27, 2017

tirr-c commented Oct 1, 2017

arielb1 commented Oct 2, 2017

arielb1 Oct 2, 2017

tirr-c Oct 2, 2017 •

edited

Loading

arielb1 commented Oct 2, 2017

arielb1 commented Oct 2, 2017

tirr-c commented Oct 2, 2017 •

edited

Loading

arielb1 Oct 3, 2017

arielb1 commented Oct 3, 2017

bors commented Oct 3, 2017

arielb1 commented Oct 3, 2017

bors commented Oct 3, 2017

bors commented Oct 3, 2017

bors commented Oct 8, 2017

arielb1 commented Oct 11, 2017

bors commented Oct 11, 2017

bors commented Oct 11, 2017

bors commented Oct 11, 2017

Refactor fmt::Display and fmt::Debug impls in ppaux #44888

Refactor fmt::Display and fmt::Debug impls in ppaux #44888

Conversation

tirr-c commented Sep 27, 2017

arielb1 Sep 27, 2017

Choose a reason for hiding this comment

tirr-c commented Oct 1, 2017

arielb1 commented Oct 2, 2017

arielb1 Oct 2, 2017

Choose a reason for hiding this comment

tirr-c Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

arielb1 commented Oct 2, 2017

arielb1 commented Oct 2, 2017

tirr-c commented Oct 2, 2017 • edited Loading

arielb1 Oct 3, 2017

Choose a reason for hiding this comment

arielb1 commented Oct 3, 2017

bors commented Oct 3, 2017

arielb1 commented Oct 3, 2017

bors commented Oct 3, 2017

bors commented Oct 3, 2017

bors commented Oct 8, 2017

arielb1 commented Oct 11, 2017

bors commented Oct 11, 2017

bors commented Oct 11, 2017

bors commented Oct 11, 2017

tirr-c Oct 2, 2017 •

edited

Loading

tirr-c commented Oct 2, 2017 •

edited

Loading