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

fix: Add tuple to render_const_scalar #14223

Merged
merged 1 commit into from
Mar 1, 2023
Merged

fix: Add tuple to render_const_scalar #14223

merged 1 commit into from
Mar 1, 2023

Conversation

HKalbasi
Copy link
Member

cc @lowr

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 28, 2023
@HKalbasi
Copy link
Member Author

I think showing <not-supported>, <layout-error> and similar is a pretty bad user experience, but currently display is widely assumed to be infallible. Thoughts?

Copy link
Contributor

@lowr lowr left a comment

Choose a reason for hiding this comment

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

Thanks so much for implementing it so quickly!

I think showing <not-supported>, <layout-error> and similar is a pretty bad user experience, but currently display is widely assumed to be infallible. Thoughts?

It might look better if we write _ for any value we cannot render, but it could also hide potential bugs and be confusing for users (<not-supported> and the like are at least descriptive). Not too sure about this.

Comment on lines 449 to 452
let Ok(layout) = layout_of_ty(f.db, &ty, krate) else {
return f.write_str("<layout-error>");
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this else branch reachable at this point? I assume not, as layout_of_ty() for the tuple itself would have failed if that for any of the inner types would, but if it is, this seems to result in incomplete rendering like (42, <layout-error>.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch! I changed it to continue instead of return. It is indeed unreachable but unreachable things might be reached sometimes.

@HKalbasi
Copy link
Member Author

I think _ is better too , but yes the current state is better for debug, and bugs are probable at this time, so I didn't change it for now.

@HKalbasi
Copy link
Member Author

HKalbasi commented Mar 1, 2023

Let's merge this to unblock #14176

@bors r+

@bors
Copy link
Contributor

bors commented Mar 1, 2023

📌 Commit f64fe66 has been approved by HKalbasi

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 1, 2023

⌛ Testing commit f64fe66 with merge ef9d5db...

@bors
Copy link
Contributor

bors commented Mar 1, 2023

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing ef9d5db to master...

@bors bors merged commit ef9d5db into rust-lang:master Mar 1, 2023
@lnicola lnicola changed the title Add tuple to render_const_scalar fix: Add tuple to render_const_scalar Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants