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

Display raw pointer as *{mut,const} T instead of *-ptr in errors #99517

Merged
merged 1 commit into from
Aug 30, 2022

Conversation

Noratrieb
Copy link
Member

@Noratrieb Noratrieb commented Jul 20, 2022

The *-ptr is rather confusing, and we have the full information for properly displaying the information.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 20, 2022
@rust-highfive
Copy link
Collaborator

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2022
@wesleywiser
Copy link
Member

This seems reasonable to me, but I'm unsure if there's an existing convention here that we are trying to move towards.

@estebank @compiler-errors how do you feel about saying raw pointer in error messages instead of *-ptr?

@estebank
Copy link
Contributor

I'm ok with calling them by name, but it'd be lovely if we did the same thing we do for references and mention what type they are a pointer for.

@Noratrieb Noratrieb changed the title Display raw pointer as raw pointer instead of *-ptr in errors Display raw pointer as *{mut,const} T instead of *-ptr in errors Jul 27, 2022
@Noratrieb
Copy link
Member Author

That's a great idea, especially since it's trivial to implement. So I did that.

@Noratrieb
Copy link
Member Author

@estebank @compiler-errors is this good now?

Copy link
Member

@compiler-errors compiler-errors 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, r=me with one nit

compiler/rustc_middle/src/ty/error.rs Show resolved Hide resolved
@compiler-errors
Copy link
Member

r? @compiler-errors
@bors delegate+

@bors
Copy link
Contributor

bors commented Aug 14, 2022

✌️ @Nilstrieb can now approve this pull request

Comment on lines 267 to 286
let tymut_string = match tymut.mutbl {
hir::Mutability::Mut => tymut.to_string(),
hir::Mutability::Not => format!("const {}", tymut.ty),
};
if tymut_string != "_" && tymut.ty.is_simple_text() {
format!("`*{}`", tymut_string).into()
Copy link
Member

Choose a reason for hiding this comment

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

I think this works

Suggested change
let tymut_string = match tymut.mutbl {
hir::Mutability::Mut => tymut.to_string(),
hir::Mutability::Not => format!("const {}", tymut.ty),
};
if tymut_string != "_" && tymut.ty.is_simple_text() {
format!("`*{}`", tymut_string).into()
if tymut.ty.is_simple_text() {
self.to_string()

I also wonder if we should change the code below for ty::Ref to match

Copy link
Member Author

Choose a reason for hiding this comment

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

I could try it out tomorrow if you want to

Copy link
Member

Choose a reason for hiding this comment

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

Please do, thanks!

Copy link
Member Author

@Noratrieb Noratrieb Aug 16, 2022

Choose a reason for hiding this comment

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

It would be pretty bad, the length check is really needed here:

-          |         expected `&T`, found type parameter `T`
+          |         expected reference, found type parameter `T`
-          |             ----------   ^ expected `&dyn Trait`, found struct `Box`
+          |             ----------   ^ expected reference, found struct `Box`

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if I understand how removing the length check causes &T to be rendered as "reference"

Copy link
Member

Choose a reason for hiding this comment

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

oh, it's because ty is not considered simple_text... ugh

Copy link
Contributor

@estebank estebank Aug 17, 2022

Choose a reason for hiding this comment

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

Could we modify the logic to make a ref dyn Trait be considered "simple"?

Edit: all it would take is adding a match arm in is_simple_ty to handle Dynamic in the same way we handle Adt in is_simple_text.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's more complex than that, since a Dynamic is a List<Binder<ExistentialPredicate>>

I also think that having the length check is nicer than the is_simple_ty check, since this exists to avoid bloating the output, so a length check will always be more accurate than a type complexity check.

Copy link
Member

Choose a reason for hiding this comment

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

Length check is fine then. As a final follow-up, is it possible to make the & and * printing logic as similar as possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

raw pointer is a little too short for the length check imo. I'll use const raw pointer, even though that is not actually used, but since the type will have a const prefix for the pointer, that seems fair

@compiler-errors compiler-errors 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 Aug 16, 2022
@compiler-errors
Copy link
Member

r=me with the last comment or not

@bors delegate+

@bors
Copy link
Contributor

bors commented Aug 20, 2022

✌️ @Nilstrieb can now approve this pull request

The `*-ptr` is rather confusing, and we have the full information for
properly displaying the information.
@compiler-errors
Copy link
Member

Thanks nils @bors r+

@bors
Copy link
Contributor

bors commented Aug 30, 2022

📌 Commit 5021dcd has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 30, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 30, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 30, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#99517 (Display raw pointer as *{mut,const} T instead of *-ptr in errors)
 - rust-lang#99928 (Do not leak type variables from opaque type relation)
 - rust-lang#100473 (Attempt to normalize `FnDef` signature in `InferCtxt::cmp`)
 - rust-lang#100653 (Move the cast_float_to_int fallback code to GCC)
 - rust-lang#100941 (Point at the string inside literal and mention if we need string inte…)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 548ed40 into rust-lang:master Aug 30, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 30, 2022
@Noratrieb Noratrieb deleted the display-raw-ptr branch August 30, 2022 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

7 participants