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

Allow customising ty::TraitRef's printing behavior. #59188

Closed
eddyb opened this issue Mar 14, 2019 · 8 comments · Fixed by #66613
Closed

Allow customising ty::TraitRef's printing behavior. #59188

eddyb opened this issue Mar 14, 2019 · 8 comments · Fixed by #66613
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Mar 14, 2019

(Originally discussed in #58140 (comment))
Right now a TraitRef prints as <T as Trait<U>> via "debug" ({:?}) and Trait<U> via "display" ({}) - this is used all over rustc to indicate what printed form of the trait is desired.

However, it would be overall cleaner for "display" and "debug" printing to match (or even for fmt::Display to not be implemented at all, perhaps?), and provide all the information that exists (i.e. including the Self type).

We would then need a replacement for printing "just the trait path with its parameters", and I think we can have a method, .print_only_trait_path(), that returns a wrapper which has the different formatting behavior.

@eddyb eddyb added C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Mar 15, 2019
@eddyb
Copy link
Member Author

eddyb commented Mar 15, 2019

Blocked on #58140.

Also, in order to do this thoroughly, fmt::Display would need to be removed from TraitRef (comment-out its section in src/librustc/ty/print/pretty.rs) and all uses of formatting it with {} would have to be put through the new .print_only_trait_path() method instead.

Only after that is done and everything compiles again, can fmt::Display be re-enabled for TraitRef (and it can be given the same behavior as {:?} today).
Some simplification of ty::print may be possible afterwards (right now associated items of a trait have to print <T as Trait<U>> themselves, without the special-casing Trait<U> would be printed).

cc @nikomatsakis

@stepnivlk
Copy link
Contributor

hey @eddyp #58140 got merged does it mean you're happy with proceeding forward with this one? If so I think I can tackle it.

@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 10, 2019
@nathankleyn
Copy link
Contributor

Hey @eddyb! I would love to pick this one up and got a bit of the way into exploring this, but wondering — what did you have in mind for where .print_only_trait_path would be implemented?

As in — is this method added to the existing Print / PrettyPrinter traits somehow? Using a new wrapper trait with that method which is impld only for ty::TraitRef? A fn at the top level somewhere? Somehow else?

@eddyp
Copy link
Contributor

eddyp commented Apr 18, 2019

@stepnivlk you probably meant @eddyb

@eddyb
Copy link
Member Author

eddyb commented Apr 19, 2019

TraitRef::print_only_trait_path would only construct a trivial wrapper around self (and that wrapper would be e.g. pub struct TraitRefPrintOnlyTraitPath<'tcx>(ty::TraitRef<'tcx>);).
We can also have the same method on PolyTraitRef (i.e. Binder<TraitRef>) that returns Binder<TraitRefPrintOnlyTraitPath> (using map_bound).

Then, in ty::print::pretty, we'd have change this to be on TraitRefPrintOnlyTraitPath:

ty::TraitRef<'tcx> {
p!(print_def_path(self.def_id, self.substs));
}

and also here:
ty::Binder<ty::TraitRef<'tcx>>,

(since that is the behavior for printing with {}, i.e. fmt::Display, it's the one we want to force to go through print_only_trait_path)

@nathankleyn
Copy link
Contributor

Thanks @eddyb — that's really helpful! I'll try to make a start on this and see how far I get, it's a really great way to explore my way around the compiler internals some more! 🤞

nathankleyn added a commit to nathankleyn/rust that referenced this issue Apr 25, 2019
Add a wrapper newtype called `TraitRefPrintOnlyTraitPath` which
overrides the printing behaviour of `ty::TraitRef` to be the simple
format `Trait<U>`.

Printing of `ty::TraitRef` for `Debug` and `Display` is now the more
verbose `<T as Trait<U>>`.

This fixes rust-lang#59188.
@basil-cow
Copy link
Contributor

basil-cow commented Nov 20, 2019

@nathankleyn would you mind me snatching this?

@nathankleyn
Copy link
Contributor

@Areredify Feel free! Some partial work here if it helps: nathankleyn@f5ec9d8

However it was, when I was last working on it, giving me some confusing issues and I couldn't untangle it.

RalfJung added a commit to RalfJung/rust that referenced this issue Nov 29, 2019
Allow customising ty::TraitRef's printing behavior

This pr allows to explicitly choose which representation of `TraitRef` (`<T as Trait<U>>` or `Trait<U>`) you want to print. `Debug` and `Display` representations of `TraitRef` now match.

Closes rust-lang#59188.
bors added a commit that referenced this issue Nov 29, 2019
Allow customising ty::TraitRef's printing behavior

This pr allows to explicitly choose which representation of `TraitRef` (`<T as Trait<U>>` or `Trait<U>`) you want to print. `Debug` and `Display` representations of `TraitRef` now match.

Closes #59188.
@bors bors closed this as completed in 64efc45 Nov 30, 2019
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. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants