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

Have the spans of TAIT type conflict errors point to the actual site instead of the owning function #93818

Closed
wants to merge 2 commits into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 9, 2022

This means we now end up with spans in the query result of borrowck, but that is unavoidable (and there are already some in there, so it isn't too bad).

@rust-highfive
Copy link
Collaborator

r? @nagisa

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

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 9, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 9, 2022
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 9, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 9, 2022
@bors
Copy link
Contributor

bors commented Feb 9, 2022

⌛ Trying commit 91fa570 with merge 07de7cd7d7549d7e3c9cd9dec398a2e0cc1f1964...

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

I'm ok with the changes as they are (it can be r=me contingent on perf), but I would love follow up work to extend the concrete type differs error to have spans pointing at the TAIT definition and potentially the signature of the enclosing item.

@@ -609,7 +610,7 @@ fn find_opaque_ty_constraints(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Ty<'_> {
// ```
let tables = self.tcx.typeck(def_id);
if let Some(_) = tables.tainted_by_errors {
self.found = Some((DUMMY_SP, self.tcx.ty_error()));
self.found = Some(ty::OpaqueHiddenType { span: DUMMY_SP, ty: self.tcx.ty_error() });
Copy link
Contributor

Choose a reason for hiding this comment

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

I hope we correctly silence these 😅

I was about to tell you off for the DUMMY_SP but that's of course preexisting 😊

Comment on lines 2 to 11
--> $DIR/issue-86465.rs:6:5
|
LL | fn f<'t, 'u>(a: &'t u32, b: &'u u32) -> (X<'t, 'u>, X<'u, 't>) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `&'a u32`, got `&'b u32`
LL | (a, a)
| ^^^^^^ expected `&'a u32`, got `&'b u32`
|
note: previous use here
--> $DIR/issue-86465.rs:5:1
--> $DIR/issue-86465.rs:6:5
|
LL | fn f<'t, 'u>(a: &'t u32, b: &'u u32) -> (X<'t, 'u>, X<'u, 't>) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | (a, a)
| ^^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

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

Shame that we don't provide more info here to help people understand what's going on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could special case if prev and the primary spans are the same? 🤷‍♀️

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 hope I can figure out how to point to the individual tuple fields at some point, but yea, maybe special casing for equal spans is the best for now

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 10, 2022

@rust-timer build 07de7cd7d7549d7e3c9cd9dec398a2e0cc1f1964

@rust-timer
Copy link
Collaborator

Queued 07de7cd7d7549d7e3c9cd9dec398a2e0cc1f1964 with parent b7cd0f7, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (07de7cd7d7549d7e3c9cd9dec398a2e0cc1f1964): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 10, 2022
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 10, 2022

@bors r=estebank

@bors
Copy link
Contributor

bors commented Feb 10, 2022

📌 Commit dd7a93d has been approved by estebank

@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 Feb 10, 2022
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 11, 2022

@bors r-

I'll revert the original PR, so leaving this one unmerged makes that easier

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 11, 2022
@bors
Copy link
Contributor

bors commented Feb 11, 2022

☔ The latest upstream changes (presumably #93893) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 14, 2022

closing in favour of doing this in the upcoming lazy TAIT PR attempt number 2

@oli-obk oli-obk closed this Feb 14, 2022
@oli-obk oli-obk deleted the spans_are_kewl branch February 14, 2022 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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