Improve errors for recursive type aliases #88121

camelid · 2021-08-17T20:21:32Z

Fixes #17539.

rust-highfive · 2021-08-17T20:21:35Z

r? @petrochenkov

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

camelid · 2021-08-17T20:22:04Z

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

src/test/ui/infinite/infinite-vec-type-recursion.stderr

estebank · 2021-08-18T10:48:51Z

compiler/rustc_query_system/src/query/job.rs

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


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

compiler/rustc_query_system/src/query/job.rs

estebank

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 · 2021-08-19T23:42:30Z

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 · 2021-08-20T01:36:39Z

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 · 2021-08-20T03:31:59Z

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 · 2021-08-21T20:17:33Z

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

compiler/rustc_middle/src/query/mod.rs

compiler/rustc_query_impl/src/keys.rs

camelid · 2021-08-21T21:43:00Z

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

camelid · 2021-08-22T01:30:51Z

Rebased (just to keep this up to date).

estebank

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.

estebank · 2021-08-27T21:10:55Z

src/test/ui/infinite/infinite-trait-alias-recursion.stderr

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


This should likely point at only T1.

estebank · 2021-08-27T21:12:11Z

src/test/ui/infinite/infinite-type-alias-mutual-recursion.stderr

+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() {}
+   | |____________^


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

compiler/rustc_query_system/src/query/mod.rs

camelid · 2021-08-27T21:51:49Z

@bors try @rust-timer queue

rust-timer · 2021-08-27T21:51:50Z

Awaiting bors try build completion.

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

bors · 2021-08-27T21:51:57Z

⌛ Trying commit c3df832 with merge fb1a092cc53518e43fa12776af7db523aa9bfecc...

bors · 2021-08-27T23:14:13Z

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

rust-timer · 2021-08-27T23:14:15Z

Queued fb1a092cc53518e43fa12776af7db523aa9bfecc with parent dfd8472, future comparison URL.

rust-timer · 2021-08-28T02:28:19Z

Finished benchmarking try commit (fb1a092cc53518e43fa12776af7db523aa9bfecc): 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

compiler/rustc_query_impl/src/plumbing.rs

camelid · 2021-08-28T19:26:43Z

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 · 2021-08-30T03:09:18Z

Ok, updated it to use the HIR map directly!

estebank · 2021-08-30T11:43:54Z

@bors r+

bors · 2021-08-30T11:43:56Z

📌 Commit d96234b has been approved by estebank

bors · 2021-09-01T03:43:40Z

⌛ Testing commit d96234b with merge c4f26b1...

bors · 2021-09-01T06:13:08Z

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

camelid added A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Aug 17, 2021

rust-highfive assigned petrochenkov Aug 17, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 17, 2021

rust-highfive assigned estebank and unassigned petrochenkov Aug 17, 2021

camelid commented Aug 17, 2021

View reviewed changes

src/test/ui/infinite/infinite-vec-type-recursion.stderr Outdated Show resolved Hide resolved

estebank reviewed Aug 18, 2021

View reviewed changes

compiler/rustc_query_system/src/query/job.rs Outdated Show resolved Hide resolved

estebank approved these changes Aug 18, 2021

View reviewed changes

camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 20, 2021

camelid force-pushed the better-recursive-alias-error branch from 2c7d13b to 568eb9c Compare August 21, 2021 20:16

This comment has been minimized.

Sign in to view

camelid removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 21, 2021

camelid commented Aug 21, 2021

View reviewed changes

compiler/rustc_middle/src/query/mod.rs Show resolved Hide resolved

compiler/rustc_query_impl/src/keys.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

camelid force-pushed the better-recursive-alias-error branch from 0e0788a to 02529bb Compare August 21, 2021 23:23

Improve errors for recursive type aliases

2f48bfa

camelid force-pushed the better-recursive-alias-error branch from 02529bb to e9f6e4b Compare August 22, 2021 01:30

inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021

estebank approved these changes Aug 27, 2021

View reviewed changes

Note that type aliases cannot be recursive

cd0fc44

camelid added 3 commits August 27, 2021 14:50

Add test of recursive trait aliases

bf360dc

Note that trait aliases cannot be recursive

c861964

Add test for type alias mutual recursion

c3df832

camelid force-pushed the better-recursive-alias-error branch from e9f6e4b to c3df832 Compare August 27, 2021 21:51

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 27, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 28, 2021

camelid commented Aug 28, 2021

View reviewed changes

compiler/rustc_query_impl/src/plumbing.rs Outdated Show resolved Hide resolved

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 Aug 30, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 1, 2021

bors merged commit c4f26b1 into rust-lang:master Sep 1, 2021

rustbot added this to the 1.56.0 milestone Sep 1, 2021

camelid deleted the better-recursive-alias-error branch September 1, 2021 15:51

jashank mentioned this pull request Oct 7, 2021

Type-alias cycle reporting could be improved if aliased type is already in scope #89616

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve errors for recursive type aliases #88121

Improve errors for recursive type aliases #88121

camelid commented Aug 17, 2021 •

edited

rust-highfive commented Aug 17, 2021

camelid commented Aug 17, 2021

estebank Aug 18, 2021

estebank left a comment

camelid commented Aug 19, 2021 •

edited

camelid commented Aug 20, 2021

camelid commented Aug 20, 2021

camelid commented Aug 21, 2021

This comment has been minimized.

camelid commented Aug 21, 2021

This comment has been minimized.

camelid commented Aug 22, 2021

estebank left a comment

estebank Aug 27, 2021

estebank Aug 27, 2021

camelid commented Aug 27, 2021

rust-timer commented Aug 27, 2021

bors commented Aug 27, 2021

bors commented Aug 27, 2021

rust-timer commented Aug 27, 2021

rust-timer commented Aug 28, 2021

camelid commented Aug 28, 2021

camelid commented Aug 30, 2021

estebank commented Aug 30, 2021

bors commented Aug 30, 2021

bors commented Sep 1, 2021

bors commented Sep 1, 2021

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

Improve errors for recursive type aliases #88121

Improve errors for recursive type aliases #88121

Conversation

camelid commented Aug 17, 2021 • edited

rust-highfive commented Aug 17, 2021

camelid commented Aug 17, 2021

estebank Aug 18, 2021

Choose a reason for hiding this comment

estebank left a comment

Choose a reason for hiding this comment

camelid commented Aug 19, 2021 • edited

camelid commented Aug 20, 2021

camelid commented Aug 20, 2021

camelid commented Aug 21, 2021

This comment has been minimized.

camelid commented Aug 21, 2021

This comment has been minimized.

camelid commented Aug 22, 2021

estebank left a comment

Choose a reason for hiding this comment

estebank Aug 27, 2021

Choose a reason for hiding this comment

estebank Aug 27, 2021

Choose a reason for hiding this comment

camelid commented Aug 27, 2021

rust-timer commented Aug 27, 2021

bors commented Aug 27, 2021

bors commented Aug 27, 2021

rust-timer commented Aug 27, 2021

rust-timer commented Aug 28, 2021

camelid commented Aug 28, 2021

camelid commented Aug 30, 2021

estebank commented Aug 30, 2021

bors commented Aug 30, 2021

bors commented Sep 1, 2021

bors commented Sep 1, 2021

camelid commented Aug 17, 2021 •

edited

camelid commented Aug 19, 2021 •

edited