The Wayback Machine - https://web.archive.org/web/20210901103337/https://github.com/rust-lang/rust/pull/88121
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

Improve errors for recursive type aliases #88121

Merged
merged 6 commits into from Sep 1, 2021

Conversation

@camelid
Copy link
Member

@camelid camelid commented Aug 17, 2021

Fixes #17539.

@rust-highfive
Copy link
Collaborator

@rust-highfive rust-highfive commented Aug 17, 2021

r? @petrochenkov

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

@camelid
Copy link
Member Author

@camelid camelid commented Aug 17, 2021

r? @estebank (feel free to reassign, but I know you were involved with that issue)

@@ -591,10 +591,14 @@ pub(crate) fn report_cycle<'a>(
err.span_note(span, &format!("...which requires {}...", query.description));
}

This comment has been minimized.

@estebank

estebank Aug 18, 2021
Contributor

I'm intrigued if the iteration above doesn't need to be reverted to be correct 🤔 (no need to address now)

Copy link
Contributor

@estebank estebank left a comment

I'm ok with the approach. Do you want to try out to expand the query with the DefId to see what the impact of that is code and perf wise?

@camelid
Copy link
Member Author

@camelid camelid commented Aug 19, 2021

Do you want to try out to expand the query with the DefId to see what the impact of that is code and perf wise?

I tried doing that but then realized that I'd need to execute a query (to get the DefKind) in the midst of reporting a query cycle error, which doesn't look to be possible with the current code architecture. I also tried recording DefKind, but that's defined in rustc_hir, which I think means that we'd get a cyclic dependency graph (it's kind of funny—I'd get a dependency cycle while working on improving type alias cycle errors...).

I could move DefKind to rustc_span::def_id (or maybe a new module rustc_span::def), which I think would solve the problem. What do you think; should I move it?

@camelid
Copy link
Member Author

@camelid camelid commented Aug 20, 2021

Do you want to try out to expand the query with the DefId to see what the impact of that is code and perf wise?

I tried doing that but then realized that I'd need to execute a query (to get the DefKind) in the midst of reporting a query cycle error, which doesn't look to be possible with the current code architecture. I also tried recording DefKind, but that's defined in rustc_hir, which I think means that we'd get a cyclic dependency graph (it's kind of funny—I'd get a dependency cycle while working on improving type alias cycle errors...).

I could move DefKind to rustc_span::def_id (or maybe a new module rustc_span::def), which I think would solve the problem. What do you think; should I move it?

I tried doing that, but that requires moving a bunch of other things, like CtorKind, to rustc_span as well, and CtorKind has an inherent impl that requires rustc_hir and rustc_ast, which rustc_span doesn't have access to. Also, it seems like a big change for not enough benefit.

An alternative approach could be to just track a boolean that says whether something is a type alias or not; it's also somewhat hacky. Yet another approach could be to have a mini-DefKind that tracks less information but can be implemented without any other dependencies. That seems like maybe the best approach. What do you think?

@camelid
Copy link
Member Author

@camelid camelid commented Aug 20, 2021

Yet another approach could be to have a mini-DefKind that tracks less information but can be implemented without any other dependencies. That seems like maybe the best approach. What do you think?

Ok, that worked! I'll probably finish up and then push up my changes tomorrow or this weekend.

@camelid camelid force-pushed the camelid:better-recursive-alias-error branch from 2c7d13b to 568eb9c Aug 21, 2021
@camelid
Copy link
Member Author

@camelid camelid commented Aug 21, 2021

I'm working on getting it working for trait aliases too, and then I think this should be ready for review.

@rust-log-analyzer

This comment has been hidden.

@camelid
Copy link
Member Author

@camelid camelid commented Aug 21, 2021

Once we think the logic is finished, we should do a perf run as you suggested earlier.

@rust-log-analyzer

This comment has been hidden.

@camelid camelid force-pushed the camelid:better-recursive-alias-error branch from 0e0788a to 02529bb Aug 21, 2021
@camelid camelid force-pushed the camelid:better-recursive-alias-error branch from 02529bb to e9f6e4b Aug 22, 2021
@camelid
Copy link
Member Author

@camelid camelid commented Aug 22, 2021

Rebased (just to keep this up to date).

Copy link
Contributor

@estebank estebank left a comment

r=me. The only thing I'd want us to have is the docstring on the new enum. Feel free to ignore the other drive by comments.

error[E0391]: cycle detected when computing the super predicates of `T1`
--> $DIR/infinite-trait-alias-recursion.rs:3:1
|
LL | trait T1 = T2;
| ^^^^^^^^^^^^^^
Comment on lines +1 to +5

This comment has been minimized.

@estebank

estebank Aug 27, 2021
Contributor

This should likely point at only T1.

note: cycle used when collecting item types in top-level module
--> $DIR/infinite-type-alias-mutual-recursion.rs:1:1
|
LL | / type X1 = X2;
LL | |
LL | | type X2 = X3;
LL | | type X3 = X1;
LL | |
LL | | fn main() {}
| |____________^
Comment on lines +21 to +30

This comment has been minimized.

@estebank

estebank Aug 27, 2021
Contributor

This reminds me, we need a way to talk about files without showing a snippet.

@camelid camelid force-pushed the camelid:better-recursive-alias-error branch from e9f6e4b to c3df832 Aug 27, 2021
@camelid
Copy link
Member Author

@camelid camelid commented Aug 27, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

@rust-timer rust-timer commented Aug 27, 2021

Awaiting bors try build completion.

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

@bors
Copy link
Contributor

@bors bors commented Aug 27, 2021

Trying commit c3df832 with merge fb1a092...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 27, 2021
… r=<try>

Improve errors for recursive type aliases

Fixes rust-lang#17539.
@bors
Copy link
Contributor

@bors bors commented Aug 27, 2021

☀️ Try build successful - checks-actions
Build commit: fb1a092 (fb1a092cc53518e43fa12776af7db523aa9bfecc)

@rust-timer
Copy link
Collaborator

@rust-timer rust-timer commented Aug 27, 2021

Queued fb1a092 with parent dfd8472, future comparison URL.

@rust-timer
Copy link
Collaborator

@rust-timer rust-timer commented Aug 28, 2021

Finished benchmarking try commit (fb1a092): comparison url.

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

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.

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

@camelid
Copy link
Member Author

@camelid camelid commented Aug 28, 2021

Perf looks good, I just have one question, and then this should be ready to be merged!

`tcx.def_kind()` could theoretically invoke another query, which could
cause an infinite query loop. Accessing the HIR map directly makes that
less likely to happen.

I also changed it to use `as_local()` (`tcx.def_kind()` seems to
implicitly call `expect_local()`) and `opt_def_kind()` to reduce the
chance of panicking on valid code.
@camelid
Copy link
Member Author

@camelid camelid commented Aug 30, 2021

Ok, updated it to use the HIR map directly!

@estebank
Copy link
Contributor

@estebank estebank commented Aug 30, 2021

@bors r+

@bors
Copy link
Contributor

@bors bors commented Aug 30, 2021

📌 Commit d96234b has been approved by estebank

@bors
Copy link
Contributor

@bors bors commented Sep 1, 2021

Testing commit d96234b with merge c4f26b1...

@bors
Copy link
Contributor

@bors bors commented Sep 1, 2021

☀️ Test successful - checks-actions
Approved by: estebank
Pushing c4f26b1 to master...

@bors bors merged commit c4f26b1 into rust-lang:master Sep 1, 2021
11 checks passed
11 checks passed
@github-actions
PR (mingw-check, ubuntu-latest-xl)
Details
@github-actions
PR (x86_64-gnu-llvm-10, ubuntu-latest-xl)
Details
@github-actions
PR (x86_64-gnu-tools, 1, ubuntu-latest-xl)
Details
@github-actions
auto
Details
@github-actions
master
Details
@github-actions
bors build finished
Details
@github-actions
bors build finished
Details
@github-actions
bors build finished
Details
@github-actions
bors build finished
Details
@bors
homu Test successful
Details
@rustbot rustbot added this to the 1.56.0 milestone Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment