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

Refactor ppaux out of existence. #58140

Open
wants to merge 68 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@eddyb
Copy link
Member

eddyb commented Feb 4, 2019

A long-time coming, this PR reorganizes and rewrites the pretty-printing architecture of rustc, specifically the parts that involve the typesystem (which used to be in rustc::util::ppaux).

Note: these commits used to be in #57967 before being split off.

The new API (i.e. the Printer and PrettyPrint traits) is in rustc::ty::print.

Design points, roughly:

  • using associated types in Printer to allow building e.g. an AST, not just printing as a side-effect
  • several overloading points for implementers of PrettyPrinter, e.g. how <...> is printed
  • for fmt::Display impls, the value to print is lifted to the ty::tls tcx, and everything after that stays within the ty::print API, which requires 'tcx to match between values and the printer's tcx, without going through fmt::Display again

Most of the behavior is unchanged, except for a few details, which should be clear from the test changes.

r? @nikomatsakis

Fixes #55464

eddyb added some commits Dec 4, 2018

eddyb added some commits Jan 14, 2019

@nikomatsakis
Copy link
Contributor

nikomatsakis left a comment

So I am generally positive on this PR. I think it's a great step towards a better pretty-printing system. However, I would like to see more comments! I left some review comments at specific places that I think could use some documentation to help convey the design a bit. I also left various other nits and questions.

}
}

impl fmt::Debug for ty::ClosureUpvar<'tcx> {

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 6, 2019

Contributor

I have to wonder if -- for some of these -- we should just use derive(Debug)?

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 6, 2019

Contributor

but maybe we should address that separately

This comment has been minimized.

@eddyb

eddyb Feb 10, 2019

Author Member

Yeah I want to get rid of as many of these as possible but wanted to avoid making decisions about them while refactoring.

}
}

impl fmt::Debug for ty::RegionKind {

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 6, 2019

Contributor

I guess we did not use derive(Debug) here because of things like ReFree and ReVar?

This comment has been minimized.

@eddyb

eddyb Feb 10, 2019

Author Member

Yeah but we can probably do whatever, this shouldn't be that used anyway.


impl fmt::Debug for ty::TraitRef<'tcx> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
// HACK(eddyb) this is used across the compiler to print

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 6, 2019

Contributor

Can we convert this into a FIXME (ie, open an issue and cite it here)? As you and I discussed, I think the default {:?} should dump all data, but then we should have some way to just print the rest. I don't think we need to address it in this PR.

This comment has been minimized.

@eddyb

eddyb Feb 10, 2019

Author Member

I think fmt::Display should also be <T as Trait<U>>, and that you'd have a method to print just the Trait<U> part, something like .print_only_trait_path().

Will open an issue.

@@ -472,6 +752,13 @@ impl<'a, 'tcx> Lift<'tcx> for ty::InstanceDef<'a> {
}
}

BraceStructLiftImpl! {

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 6, 2019

Contributor

Did your PR to use proc macros in rustc land? Maybe we should convert these things to proc macros or custom derives... (obviously not in this PR, just thinking out loud)

@@ -0,0 +1,318 @@
use hir::map::{DefPathData, DisambiguatedDefPathData};

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 6, 2019

Contributor

I feel like there should be some doc-comments in this file =) Some questions I would like to see answered:

  • I don't really understand the role of these traits
  • When is it appropriate to create a new printer (I've seen a few in this PR, but I can't quite tell what for)
  • When would it make sense to (e.g.) add a new print_my_type method to Printer?

This comment has been minimized.

@eddyb

eddyb Feb 10, 2019

Author Member
  • I don't really understand the role of these traits

The PR description tries to explain it but it probably fails to do so fully.

I need a better name than Printer/print_* for the general case, it's effectively a serialization scheme where the primitives are paths and several typesystem entities, only PrettyPrinter is really about "printing".

There might not even any point to having Printer (we can take the little logic it has and make it available some other way), and we can move its interface into PrettyPrinter, which can actually have defaults for (almost) everything (i.e. you can get FmtPrinter, or something similar, very easily, and then you can just customize the parts you care about).

  • When is it appropriate to create a new printer (I've seen a few in this PR, but I can't quite tell what for)

It's not clear to me right now too, other than to replace the old ItemPathBuffer trait.

In #57967 you can see I created a Printer in rustc_codegen_utils::symbol_names::mw which relies on the associated types, but almost everything else is PrettyPrinter and that you would create whenever FmtPrinter is not enough.

I would even take the RegionHighlightMode out of FmtPrinter and move it to wherever most of the lifetime placeholder code lives, but I felt that was even more churn to add to this PR.

  • When would it make sense to (e.g.) add a new print_my_type method to Printer?

When you want to overload it in an implementation, without having to reimplement printing for whatever happens to contain it (e.g. paths, types, etc.). This is usually the case for all "typesystem entities".

For example, @varkor will have to add a print_const method that takes a ty::LazyConst<'tcx> (unless @oli-obk wants to rename it again), because printers will want to choose how they print constants without having to handle that in a few separate places.

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 13, 2019

Contributor

Writing up these notes in a comment would be great

}
}

// HACK(eddyb) this duplicates `FmtPrinter`'s `path_generic_args`,

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 6, 2019

Contributor

I didn't look too closely here, but is it just not convenient to write a helper fn or something?

This comment has been minimized.

@eddyb

eddyb Feb 13, 2019

Author Member

Oh I did that in https://github.com/alexcrichton/rustc-demangle/pull/23/files#diff-8519e21583289947083908b68780a70cR915 but that still duplicates a bit of logic.

I would need to get rid of Printer, I guess? PrettyPrinter can tolerate an API that's more flexible in terms of mixing how/when things are printed.

};
}

// HACK(eddyb) this is separate because `ty::RegionKind` doesn't need lifting.

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 6, 2019

Contributor

Nit: This comment kind of lacks context. Separate from what, for example? I guess you mean:

Implement Display for RegionKind separately. We can't use the macro below because RegionKind doesn't need lifting.

I don't personally feel a "HACK" tag is necessary here but feel free to re-insert if you like =)

&'tcx ty::List<ty::ExistentialPredicate<'tcx>>,

// HACK(eddyb) these are exhaustive instead of generic,
// because `for<'gcx: 'tcx, 'tcx>` isn't possible yet.

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 6, 2019

Contributor

good comment =)

vid: Option<ty::Region<'tcx>>,
expected_has_vid: Option<usize>,
actual_has_vid: Option<usize>,
any_self_ty_has_vid: bool,
) {
// HACK(eddyb) maybe move this in a more central location.
#[derive(Copy, Clone)]
struct Highlighted<'a, 'gcx, 'tcx, T> {

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 6, 2019

Contributor

It does seem a bit odd for this struct to be private to this function, when the region highlighting machinery itself all likes in the generic ty::print module. We could move it there I guess, but I'm also ok to hold off a bit, because of the reasons in this comment -- basically this mechanism is perhaps not as general as I was initially thinking.

This comment has been minimized.

@eddyb

eddyb Feb 10, 2019

Author Member

I would want to move it out of ty::print::pretty, but maybe not in this PR.

Error = fmt::Error,
>,
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 6, 2019

Contributor

Something about this impl doesn't feel right to me. I mean, I'm not saying it's buggy or anything like that. I guess it's fine. I think I'm just a bit sad about how this Display impl creates the pretty-printer, rather than adding a bit of configuration to an existing one...somehow...

Basically, I was sort of hoping we can get to a place where you can kind of "compose" flags like this, so that you could write:

foo.with_highlight(...).with_highlight(...).with_fuzzy_bear(...)

and thus print foo with all those things. Or..something like that. Anyway, I don't think that's necessarily something to address here, it's just something I'd like to think about maybe in some future PR.

This comment has been minimized.

@eddyb

eddyb Feb 10, 2019

Author Member

I want to take configuration out of FmtPrinter (and do it in types instead of data, basically).

The reason the printer is created here is that this is a printing entry-point.

That is, Highlighted<T> display-formats similar to T does, but with different configuration.

The T wouldn't have been ty::print::Print::print-ed without going through fmt::Display, so there's no meaningful "existing printer".

An earlier version of this code created a pretty-printer once but when someone touched the diagnostics I had to introduce this wrapper type mid-rebase.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 7, 2019

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 13, 2019

I don't really have enough energy to track this PR. So r=me once rebased. Ideally, we'd add a few more comments about the various things I highlighted in my review as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment