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.

Sign up for GitHub

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

Jump to bottom

Pretty-print inherent projections correctly #111486

Merged
merged 1 commit into from
May 23, 2023
Merged

Pretty-print inherent projections correctly #111486

merged 1 commit into from
May 23, 2023

Conversation

fmease
Copy link
Member

@fmease fmease commented May 11, 2023 •
edited
Loading

Previously, we were trying to pretty-print inherent projections with Printer::print_def_path which is incorrect since
it expects the substitutions to be of a certain format (parents substs followed by own substs) which doesn't hold for
inherent projections (self type subst followed by own substs).
Now we print inherent projections manually.

Fixes #111390.
Fixes #111397.

Lacking tests! Is there a test suite / compiletest flags for the pretty-printer? In most if not all cases,
inherent projections are normalized away before they get the chance to appear in diagnostics.

If I were to create regression tests for linked issues, they would need to be mir-opt tests to exercise
-Zdump-mir=all (right?) which doesn't feel quite adequate to me.

@rustbot label F-inherent_associated_types

@rustbot
Copy link
Collaborator

rustbot commented May 11, 2023

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. F-inherent_associated_types `#![feature(inherent_associated_types)]` labels May 11, 2023
@fmease

This comment was marked as outdated.

Sign in to view
@fmease fmease closed this May 12, 2023
@fmease fmease reopened this May 17, 2023
@fmease fmease force-pushed the pp-inh-proj branch 2 times, most recently from 596e476 to 79f2c1f Compare May 17, 2023 12:23
@fmease
Copy link
Member Author

fmease commented May 17, 2023

I've changed the approach (over at fmease#4, you can see an explanation why the previous one wasn't correct).

@rust-log-analyzer

This comment has been minimized.

Sign in to view
fmease
fmease commented May 17, 2023
View reviewed changes
compiler/rustc_middle/src/ty/print/pretty.rs
@@ -2821,7 +2838,11 @@ define_print_and_forward_display! {
}

ty::AliasTy<'tcx> {
p!(print_def_path(self.def_id, self.substs));
if let DefKind::Impl { of_trait: false } = cx.tcx().def_kind(cx.tcx().parent(self.def_id)) {
Copy link
Member Author

@fmease fmease May 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm no longer using AliasTy::kind() here since it ICEs on AssocConsts (see the rust-long-analyzer above). AliasTys can actually be AssocConsts to represent associated const projections even though it's not really documented anywhere as I assume it to be a temporary hack to make #![feature(associated_const_equality)] work until AliasTerm is introduced.

@fmease fmease marked this pull request as draft May 17, 2023 18:35
@fmease
Copy link
Member Author

fmease commented May 17, 2023 •
edited
Loading

Alright, now it should be ready for review! Apologies for the spam. I've added some bug!()s for some ty::Alias(ty::Inherent, _) match arms that shouldn't be reachable in practice which previously called print_def_path on inherent projections which strictly speaking wasn't correct (if they were reachable).

If those cases are ever reached (which I don't believe) the proper solution would be to uplift pretty_print_inherent_projection from PrettyPrinter to Printer, to rename it & and to call it instead. I'm not doing that rn since it'd require a bit of boilerplate w/o any obvious benefit.

@fmease fmease marked this pull request as ready for review May 17, 2023 21:17
@petrochenkov
Copy link
Contributor

r=me after writing a more readable commit message
@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2023
@fmease
Copy link
Member Author

fmease commented May 22, 2023

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 22, 2023
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 22, 2023

📌 Commit 778abc7 has been approved by petrochenkov

It is now in the queue for this repository.

@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 May 22, 2023
@fmease fmease mentioned this pull request May 23, 2023
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request May 23, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2023
@bors
Auto merge of rust-lang#111882 - matthiaskrgr:rollup-1xyv5mq, r=matth…
d569987 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#111427 ([rustdoc][JSON] Use exclusively externally tagged enums in the JSON representation)
 - rust-lang#111486 (Pretty-print inherent projections correctly)
 - rust-lang#111722 (Document stack-protector option)
 - rust-lang#111761 (fix(resolve): not defined `extern crate shadow_name`)
 - rust-lang#111845 (Update books)
 - rust-lang#111851 (CFI: Fix encode_region: unexpected ReEarlyBound(0, 'a))
 - rust-lang#111871 (Migrate GUI colors test to original CSS color format)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 221039b into rust-lang:master May 23, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 23, 2023
@fmease fmease mentioned this pull request May 23, 2023
Merged
@fmease fmease deleted the pp-inh-proj branch May 23, 2023 22:03
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 24, 2023
@matthiaskrgr
Rollup merge of rust-lang#111887 - fmease:inh-proj-pp-tests, r=compil…
d49d347 
…er-errors

Add regression tests for pretty-printing inherent projections

Tests for rust-lang#111486.
Fixes rust-lang#111879.

r? `@matthiaskrgr`
@fmease fmease mentioned this pull request Jan 5, 2024
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@petrochenkov petrochenkov

Labels
F-inherent_associated_types `#![feature(inherent_associated_types)]` 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
Milestone
1.71.0
5 participants
@fmease @rustbot @rust-log-analyzer @petrochenkov @bors