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

Tweak 'static suggestion code #71235

Merged
merged 6 commits into from
Apr 24, 2020
Merged

Tweak 'static suggestion code #71235

merged 6 commits into from
Apr 24, 2020

Conversation

estebank
Copy link
Contributor

Fix #71196.

@rust-highfive
Copy link
Contributor

r? @ecstatic-morse

(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 Apr 17, 2020
@rust-highfive

This comment has been minimized.

Sign in to view
@ecstatic-morse
Copy link
Contributor

These suggestions look much better to me! Is this ready for review?

@estebank
Copy link
Contributor Author

@ecstatic-morse thanks! Yes, I think I don't have anything else I can improve in the output except for one case in particular that could be confusing, but the added suggestion helps clear it up. I would also like to clean this up, but it can be done in a separate PR.

ecstatic-morse
ecstatic-morse approved these changes Apr 18, 2020
View reviewed changes
Copy link
Contributor

@ecstatic-morse ecstatic-morse left a comment

Choose a reason for hiding this comment

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

Sweet! As I mentioned above, these errors read much better to me. I like how the fix is written out in full, especially in the more complex cases. The logic seems cleaner than the old version as well. I left a few requests for clarification but overall I think this is good.

src/librustc_typeck/astconv.rs
segment.ident.span
} else {
trait_ref.path.span
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, was this fixed elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Partly. I think this used to affect multiple tests but the only case that appears now is at https://github.com/rust-lang/rust/pull/71235/files#diff-1bd3f65ee2d72d0225a5db83a8391165.

src/librustc_resolve/late/lifetimes.rs
} else {
db.help(&format!(
"this function's return type contains a borrowed value, \
but the signature does not say whether it is borrowed from {}",
m
));
true
false
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the semantics of the return value of report_elision_failure have changed. What does the bool indicate here? Would you mind documenting it?

@ecstatic-morse
Copy link
Contributor

r=me with comment about meaning of return value for report_elision_failure.

@estebank
Copy link
Contributor Author

@bors r=ecstatic-morse

@bors
Copy link
Collaborator

bors commented Apr 18, 2020

📌 Commit fe7216d has been approved by ecstatic-morse

@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 Apr 18, 2020
@bors
Copy link
Collaborator

bors commented Apr 20, 2020

⌛ Testing commit fe7216d with merge cc8e308c6b15ea75974aef459c7c340e5fcb31de...

@bors bors mentioned this pull request Apr 20, 2020
Merged
@bors
Copy link
Collaborator

bors commented Apr 20, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 20, 2020
@estebank
Copy link
Contributor Author

@bors retry seems like a transient windows CI issue https://dev.azure.com/rust-lang/rust/_build/results?buildId=26917&view=logs&j=90f6024d-9735-531d-4933-8983ca347d60&t=0ac37a54-82be-5e9e-e574-4c58820a1ef5

@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 Apr 20, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 21, 2020
@Dylan-DPC
Rollup merge of rust-lang#71235 - estebank:lt-sugg-2, r=ecstatic-morse
f940f6d 
Tweak `'static` suggestion code

Fix rust-lang#71196.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Apr 21, 2020
Closed
@JohnTitor
Copy link
Member

Seems failed in rollup: #71377 (comment)
@bors r-

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 22, 2020
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 22, 2020
@estebank
Copy link
Contributor Author

@bors r=ecstatic-morse fixed test

@bors
Copy link
Collaborator

bors commented Apr 22, 2020

📌 Commit 59c816d has been approved by ecstatic-morse

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 22, 2020
@rust-highfive

This comment has been minimized.

Sign in to view
@estebank
Copy link
Contributor Author

@bors r=ecstatic-morse always sort MultiSpan on creation to avoid non-deterministic output

@bors
Copy link
Collaborator

bors commented Apr 23, 2020

📌 Commit 25f8966 has been approved by ecstatic-morse

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 23, 2020
@Dylan-DPC
Rollup merge of rust-lang#71235 - estebank:lt-sugg-2, r=ecstatic-morse
1ba8b13 
Tweak `'static` suggestion code

Fix rust-lang#71196.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Apr 23, 2020
Closed
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Apr 24, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 24, 2020
@bors
Auto merge of rust-lang#71509 - Dylan-DPC:rollup-n8s37rm, r=Dylan-DPC
0612568 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#71235 (Tweak `'static` suggestion code)
 - rust-lang#71318 (miri-unleash tests: ensure they fire even with 'allow(const_err)')
 - rust-lang#71428 (Let compiletest recognize gdb 10.x)
 - rust-lang#71475 (Miri Frame: use mir::Location to represent position in function)
 - rust-lang#71476 (more compact way to adjust test sizes for Miri)

Failed merges:

r? @ghost
@bors bors merged commit 7d8a3ad into rust-lang:master Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ecstatic-morse ecstatic-morse ecstatic-morse approved these changes

Assignees

@ecstatic-morse ecstatic-morse

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong suggestion of using + static instead of <'static> for unions
5 participants
@estebank @rust-highfive @ecstatic-morse @bors @JohnTitor