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

Pretty print records using field names #927

Merged
merged 9 commits into from
Dec 6, 2024
Merged

Pretty print records using field names #927

merged 9 commits into from
Dec 6, 2024

Conversation

ranjitjhala
Copy link
Contributor

Instead of just commas or foo.0.1.2 ...

Screenshot 2024-12-05 at 1 56 28 PM

crates/flux-common/src/dbg.rs Outdated Show resolved Hide resolved
crates/flux-middle/src/pretty.rs Outdated Show resolved Hide resolved
define_scoped!(cx, f);
w!("{}: {:?}", ^self.name, &self.value)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
}

@@ -277,8 +278,25 @@ impl Pretty for Ty {
}
TyKind::Param(param_ty) => w!("{}", ^param_ty),
TyKind::Downcast(adt, .., variant_idx, fields) => {
w!("{:?}::{}", adt.did(), ^adt.variant(*variant_idx).name)?;
if !fields.is_empty() {
let is_struct = adt.is_struct();
Copy link
Member

Choose a reason for hiding this comment

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

To ponder: maybe TyKind::Downcast is something we should hide from users so instead of printing

_1: S { x: i32[a0], y: i32[b1] }

we should print

_1.x: i32[a0], _1.y: i32[a1]

But perhaps the context that it is an S may be relevant to users too

But also { x: i32[a0], y: i32[a1] } doesn't have an equivalence in Rust syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, a bit unclear what the right thing here is...

Comment on lines 1205 to 1211
let field_binds = adt_sort_def
.field_names()
.iter()
.zip(flds)
.map(|(name, value)| FieldBind { name: *name, value: value.clone() })
.collect_vec();
w!("{{ {:?} }}", join!(", ", field_binds))
Copy link
Member

@nilehmann nilehmann Dec 5, 2024

Choose a reason for hiding this comment

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

wait @ranjitjhala!. Didn't notice this, I'm not sure we should skip the def_id blindly. So, if you write

S { v: v == S { x: 0, y: 0 } }

I think we do want to keep the S inside

Copy link
Member

@nilehmann nilehmann Dec 5, 2024

Choose a reason for hiding this comment

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

With this change we print S {v: v == {x: 0, y: 0}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, no worries, will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll elide the S (DefId) only when printing ... a top-level index?

Copy link
Member

Choose a reason for hiding this comment

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

yes, that sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used a newtype wrapper type trick to only elide the DefId for top-level indices,

a70354d

did you want to take a look? Or merge?

Copy link
Member

@nilehmann nilehmann left a comment

Choose a reason for hiding this comment

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

looks good

crates/flux-middle/src/rty/pretty.rs Outdated Show resolved Hide resolved
crates/flux-middle/src/rty/pretty.rs Outdated Show resolved Hide resolved
crates/flux-middle/src/rty/pretty.rs Outdated Show resolved Hide resolved
@ranjitjhala ranjitjhala merged commit 184296b into main Dec 6, 2024
7 checks passed
@ranjitjhala ranjitjhala deleted the pretty-record branch December 6, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants