Add initial implementation of HIR-based WF checking for diagnostics #83898

Aaron1011 · 2021-04-05T18:01:29Z

During well-formed checking, we walk through all types 'nested' in
generic arguments. For example, WF-checking Option<MyStruct<u8>>
will cause us to check MyStruct<u8> and u8. However, this is done
on a rustc_middle::ty::Ty, which has no span information. As a result,
any errors that occur will have a very general span (e.g. the
definintion of an associated item).

This becomes a problem when macros are involved. In general, an
associated type like type MyType = Option<MyStruct<u8>>; may
have completely different spans for each nested type in the HIR. Using
the span of the entire associated item might end up pointing to a macro
invocation, even though a user-provided span is available in one of the
nested types.

This PR adds a framework for HIR-based well formed checking. This check
is only run during error reporting, and is used to obtain a more precise
span for an existing error. This is accomplished by individually
checking each 'nested' type in the HIR for the type, allowing us to
find the most-specific type (and span) that produces a given error.

The majority of the changes are to the error-reporting code. However,
some of the general trait code is modified to pass through more
information.

Since this has no soundness implications, I've implemented a minimal
version to begin with, which can be extended over time. In particular,
this only works for HIR items with a corresponding DefId (e.g. it will
not work for WF-checking performed within function bodies).

rust-highfive · 2021-04-05T18:01:33Z

r? @varkor

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

Aaron1011 · 2021-04-05T18:28:33Z

compiler/rustc_trait_selection/src/traits/chalk_fulfill.rs

@@ -58,6 +58,9 @@ impl TraitEngine<'tcx> for FulfillmentContext<'tcx> {
                    obligation: obligation.clone(),
                    code: FulfillmentErrorCode::CodeAmbiguity,
                    points_at_arg_span: false,
+                    // FIXME - does Chalk have a notation of 'root obligation'?


cc @rust-lang/wg-traits: Does Chalk have a way of getting the obligation first passed to TraitEngine::register_predicate_obligation?

Well, does Chalk? No. Chalk only takes a goal and produces an answer. Does the chalk_fulfill code? I'm not sure, I'll have to go back through this.

I imagine that the code here is most correct. The rustc trait solver will register predicates as it tries to solve a predicate, whereas Chalk won't, it just gives back an answer.

Yeah, chalk sort of only has a notion of root obligation, at least as presently designed.

I'm not too concerned because I already fully expect to have to rework a lot of things when we try to land chalk :)

Aaron1011 · 2021-04-05T20:54:22Z

@bors try @rust-timer queue

rust-timer · 2021-04-05T20:54:24Z

Awaiting bors try build completion.

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

bors · 2021-04-05T20:54:31Z

⌛ Trying commit 962fccd92d7b05b813e61caba12756a6c1ccca26 with merge 8a8a6e6b205d7dc020125389bc03483f80fee94c...

bors · 2021-04-05T21:40:54Z

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

rust-timer · 2021-04-05T21:40:55Z

Queued 8a8a6e6b205d7dc020125389bc03483f80fee94c with parent 5a7a0ac, future comparison URL.

rust-timer · 2021-04-06T01:34:14Z

Finished benchmarking try commit (8a8a6e6b205d7dc020125389bc03483f80fee94c): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

varkor · 2021-04-21T11:31:26Z

I'm a little short on time for larger reviews at the moment, so I'm going to reassign to avoid delay.

r? @estebank

bors · 2021-07-16T04:06:36Z

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

estebank · 2021-07-16T17:37:13Z

@Aaron1011 sorry for the delays. Looking at this PR now.

Aaron1011 · 2021-07-16T17:47:08Z

@bors try @rust-timer queue

rust-timer · 2021-07-16T17:47:10Z

Awaiting bors try build completion.

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

bors · 2021-07-16T17:47:24Z

⌛ Trying commit d07f0a702d7c37c08988d06ddda241720f0cad76 with merge 2b90040103d3c924f06ba922b016ccdd6e47f4b9...

estebank

r=me after addressing nitpicks

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

compiler/rustc_middle/src/query/mod.rs

compiler/rustc_trait_selection/src/opaque_types.rs

estebank · 2021-07-16T18:25:09Z

compiler/rustc_trait_selection/src/traits/chalk_fulfill.rs

@@ -58,6 +58,9 @@ impl TraitEngine<'tcx> for FulfillmentContext<'tcx> {
                    obligation: obligation.clone(),
                    code: FulfillmentErrorCode::CodeAmbiguity,
                    points_at_arg_span: false,
+                    // FIXME - does Chalk have a notation of 'root obligation'?


I'm not too concerned because I already fully expect to have to rework a lot of things when we try to land chalk :)

compiler/rustc_trait_selection/src/traits/fulfill.rs

bors · 2021-07-16T18:37:30Z

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

rust-timer · 2021-07-16T18:37:32Z

Queued 2b90040103d3c924f06ba922b016ccdd6e47f4b9 with parent 2119976, future comparison URL.

rust-timer · 2021-07-16T20:19:19Z

Finished benchmarking try commit (2b90040103d3c924f06ba922b016ccdd6e47f4b9): comparison url.

Summary: This benchmark run did not return any significant 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

During well-formed checking, we walk through all types 'nested' in generic arguments. For example, WF-checking `Option<MyStruct<u8>>` will cause us to check `MyStruct<u8>` and `u8`. However, this is done on a `rustc_middle::ty::Ty`, which has no span information. As a result, any errors that occur will have a very general span (e.g. the definintion of an associated item). This becomes a problem when macros are involved. In general, an associated type like `type MyType = Option<MyStruct<u8>>;` may have completely different spans for each nested type in the HIR. Using the span of the entire associated item might end up pointing to a macro invocation, even though a user-provided span is available in one of the nested types. This PR adds a framework for HIR-based well formed checking. This check is only run during error reporting, and is used to obtain a more precise span for an existing error. This is accomplished by individually checking each 'nested' type in the HIR for the type, allowing us to find the most-specific type (and span) that produces a given error. The majority of the changes are to the error-reporting code. However, some of the general trait code is modified to pass through more information. Since this has no soundness implications, I've implemented a minimal version to begin with, which can be extended over time. In particular, this only works for HIR items with a corresponding `DefId` (e.g. it will not work for WF-checking performed within function bodies).

Aaron1011 · 2021-07-16T21:38:17Z

@bors r=estebank

bors · 2021-07-16T21:38:18Z

📌 Commit a765333 has been approved by estebank

bors · 2021-07-16T21:54:47Z

⌛ Testing commit a765333 with merge 32c447e...

bors · 2021-07-17T00:35:31Z

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

rust-highfive assigned varkor Apr 5, 2021

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

Aaron1011 commented Apr 5, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

Aaron1011 force-pushed the feature/hir-wf branch from 8eb793d to 962fccd Compare April 5, 2021 19:58

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

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

rust-highfive assigned estebank and unassigned varkor Apr 21, 2021

Aaron1011 force-pushed the feature/hir-wf branch from 962fccd to 12f6419 Compare May 8, 2021 23:28

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

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2021

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 30, 2021

JohnCSimon removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 22, 2021

This comment has been minimized.

Sign in to view

Aaron1011 force-pushed the feature/hir-wf branch from 0fe07ab to c0b8604 Compare July 10, 2021 15:27

Aaron1011 force-pushed the feature/hir-wf branch from c0b8604 to d07f0a7 Compare July 16, 2021 12:56

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

estebank approved these changes Jul 16, 2021

View reviewed changes

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

Aaron1011 force-pushed the feature/hir-wf branch from d07f0a7 to a765333 Compare July 16, 2021 21:36

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 Jul 16, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 17, 2021

bors merged commit 32c447e into rust-lang:master Jul 17, 2021

rustbot added this to the 1.55.0 milestone Jul 17, 2021

This was referenced Jul 17, 2021

Normalize projections under binders #85499

Merged

Move DepKind and query TLS to rustc_query_system #86038

Closed

Trait upcasting coercion (part1) #86264

Merged

lqd mentioned this pull request Aug 3, 2021

Type checking for this incorrect impl trait panics with incremental compilation #87549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial implementation of HIR-based WF checking for diagnostics #83898

Add initial implementation of HIR-based WF checking for diagnostics #83898

Aaron1011 commented Apr 5, 2021

rust-highfive commented Apr 5, 2021

Aaron1011 Apr 5, 2021

jackh726 Apr 13, 2021

nikomatsakis Apr 21, 2021

estebank Jul 16, 2021

This comment has been minimized.

This comment has been minimized.

Aaron1011 commented Apr 5, 2021

rust-timer commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

rust-timer commented Apr 5, 2021

rust-timer commented Apr 6, 2021

varkor commented Apr 21, 2021

This comment has been minimized.

bors commented Jul 16, 2021

estebank commented Jul 16, 2021

Aaron1011 commented Jul 16, 2021

rust-timer commented Jul 16, 2021

bors commented Jul 16, 2021

estebank left a comment

estebank Jul 16, 2021

bors commented Jul 16, 2021

rust-timer commented Jul 16, 2021

rust-timer commented Jul 16, 2021

Aaron1011 commented Jul 16, 2021

bors commented Jul 16, 2021

bors commented Jul 16, 2021

bors commented Jul 17, 2021

Add initial implementation of HIR-based WF checking for diagnostics #83898

Add initial implementation of HIR-based WF checking for diagnostics #83898

Conversation

Aaron1011 commented Apr 5, 2021

rust-highfive commented Apr 5, 2021

Aaron1011 Apr 5, 2021

Choose a reason for hiding this comment

jackh726 Apr 13, 2021

Choose a reason for hiding this comment

nikomatsakis Apr 21, 2021

Choose a reason for hiding this comment

estebank Jul 16, 2021

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Aaron1011 commented Apr 5, 2021

rust-timer commented Apr 5, 2021

bors commented Apr 5, 2021

bors commented Apr 5, 2021

rust-timer commented Apr 5, 2021

rust-timer commented Apr 6, 2021

varkor commented Apr 21, 2021

This comment has been minimized.

bors commented Jul 16, 2021

estebank commented Jul 16, 2021

Aaron1011 commented Jul 16, 2021

rust-timer commented Jul 16, 2021

bors commented Jul 16, 2021

estebank left a comment

Choose a reason for hiding this comment

estebank Jul 16, 2021

Choose a reason for hiding this comment

bors commented Jul 16, 2021

rust-timer commented Jul 16, 2021

rust-timer commented Jul 16, 2021

Aaron1011 commented Jul 16, 2021

bors commented Jul 16, 2021

bors commented Jul 16, 2021

bors commented Jul 17, 2021