-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
[nll] improve the "fully elaborated type" case in region errors #52648
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
Conversation
|
This currently uses the slightly-yucky string manipulation approach that we spoke about briefly in Zulip @nikomatsakis. If this isn't the way we want to do it then I'm happy to change it. |
src/test/ui/issue-52533.nll.stderr
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe missing whitespace between region and type(u32)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Thanks, never spotted these when looking through the changed tests, pushed a fix for that.
nikomatsakis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the string manipulation doesn't feel great but we do have to decide what would be a better way...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably here we don't want to say closure necessarily?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this to match the rest of the PR that detects "function" or "closure" based on the mir_def_id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, this is pretty grody
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed this with most recent commit.
98e4cb2 to
70ed4e2
Compare
src/librustc/util/ppaux.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment please =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated most recent commit to address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably put this code into ppaux and encapsulate the HIGHLIGHT_REGION variable itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated most recent commit to address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as an aside, it might be nice to not print '_ in this case but just &
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated most recent commit to address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so wait why this diff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh because we don't find the '1 at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated most recent commit to address this.
src/librustc/util/ppaux.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, I know I sort of asked for this, but I think in a way I liked '_ better -- in particular, I'd like to see a test that shows what happens when you elaborate a struct type like TyCtxt -- I'd like to see TyCtxt<'_, '1, '_>, in particular. =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead I would modify the TyRef code to look for either "'_" or "" and just skip printing in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this in recent commit.
src/test/ui/issue-52533-1.nll.stderr
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is so beautiful
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Oops! Fixed that. |
|
@bors r+ |
|
📌 Commit 3efc77b464330a0462f0f7036f77a8eff77d1363 has been approved by |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Oops again! Fixed that. |
|
@bors r+ p=1 |
|
📌 Commit 301e4da54648f9fdeff0865048e2c6ade258a58b has been approved by |
|
@bors r=nikomatsakis p=1 NLL critical |
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit 301e4da54648f9fdeff0865048e2c6ade258a58b has been approved by |
|
@bors r- Merge conflict ↓ |
|
Rebased! |
|
@bors r=nikomatsakis p=1 |
|
📌 Commit 2e4224a has been approved by |
[nll] improve the "fully elaborated type" case in region errors Fixes #52533. r? @nikomatsakis
|
☀️ Test successful - status-appveyor, status-travis |
Fixes #52533.
r? @nikomatsakis