Remove some direct calls to local_def_id_to_hir_id on diagnostics #108915

spastorino · 2023-03-08T18:25:35Z

Was playing with tests/ui/impl-trait/in-trait/default-body-with-rpit.rs and was able to remove some ICEs. Still getting ...

error[E0277]: `impl Future<Output = Foo::{opaque#0}>` is not a future
  --> tests/ui/impl-trait/in-trait/default-body-with-rpit.rs:10:28
   |
10 |     async fn baz(&self) -> impl Debug {
   |                            ^^^^^^^^^^ `impl Future<Output = Foo::{opaque#0}>` is not a future
   |
   = help: the trait `Future` is not implemented for `impl Future<Output = Foo::{opaque#0}>`
   = note: impl Future<Output = Foo::{opaque#0}> must be a future or must implement `IntoFuture` to be awaited
note: required by a bound in `Foo::{opaque#1}`
  --> tests/ui/impl-trait/in-trait/default-body-with-rpit.rs:10:28
   |
10 |     async fn baz(&self) -> impl Debug {
   |                            ^^^^^^^^^^ required by this bound in `Foo::{opaque#1}`

error[E0277]: the size for values of type `impl Future<Output = Foo::{opaque#0}>` cannot be known at compilation time
  --> tests/ui/impl-trait/in-trait/default-body-with-rpit.rs:10:28
   |
10 |     async fn baz(&self) -> impl Debug {
   |                            ^^^^^^^^^^ doesn't have a size known at compile-time
   |
   = help: the trait `Sized` is not implemented for `impl Future<Output = Foo::{opaque#0}>`
note: required by a bound in `Foo::{opaque#1}`
  --> tests/ui/impl-trait/in-trait/default-body-with-rpit.rs:10:28
   |
10 |     async fn baz(&self) -> impl Debug {
   |                            ^^^^^^^^^^ required by this bound in `Foo::{opaque#1}`

error: internal compiler error: compiler/rustc_hir_typeck/src/closure.rs:724:18: async fn generator return type not an inference variable: Foo::{opaque#1}<'_>
  --> tests/ui/impl-trait/in-trait/default-body-with-rpit.rs:10:39
   |
10 |       async fn baz(&self) -> impl Debug {
   |  _______________________________________^
11 | |         ""
12 | |     }
   | |_____^

But I guess this is a little bit of progress anyway.

This one goes on top of #108700 and #108945
r? @compiler-errors

cjgillot

Silly question: why not feed Some(HirId::make_owner(def_id)) to opt_local_def_id_to_hir_id(def_id) for RPITIT items?
The hir_owner and hir_owner_nodes will return None anyway.

compiler/rustc_middle/src/hir/map/mod.rs

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

spastorino · 2023-03-09T02:48:33Z

Silly question: why not feed Some(HirId::make_owner(def_id)) to opt_local_def_id_to_hir_id(def_id) for RPITIT items? The hir_owner and hir_owner_nodes will return None anyway.

any opinion on this @compiler-errors ?

compiler-errors · 2023-03-09T02:51:37Z

Silly question: why not feed Some(HirId::make_owner(def_id)) to opt_local_def_id_to_hir_id(def_id) for RPITIT items? The hir_owner and hir_owner_nodes will return None anyway.

any opinion on this @compiler-errors ?

Sure, try it. If you do that, can you try to find other places you've already modified to side-step the lack of HIR id and revert those, if there are any? Any calls that rely on tcx.hir().get(..) will obviously have to stay modified, but I'm not sure if any other code changes could be safely reverted if you feed opt_local_def_id_to_hir_id. You'll have to think about it.

spastorino · 2023-03-09T14:52:04Z

The test failures are happening in https://github.com/rust-lang/rust/pull/108915/files#diff-885afd8bd7572095e2347dc7eabce51cde3f47e07841d705770d94e0fccf2479R408, @compiler-errors any clue?. I'm not sure why the hir_id may not be an expr now.

spastorino · 2023-03-09T15:10:38Z

Have force pushed to fix something that wasn't good but the Fs are still there and I don't see how 5cac678 makes things different.

compiler-errors

Can you make the maybe_body_id into a LocalDefId instead of a DefId?

Also, you should be able to pull the commit "Make some report and emit errors take DefIds instead of BodyIds" into a separate PR and I can r+ that one after you've applied the feedback.

compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs

compiler/rustc_hir_typeck/src/writeback.rs

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

compiler/rustc_infer/src/infer/error_reporting/need_type_info.rs

spastorino · 2023-03-09T18:25:13Z

Can you make the maybe_body_id into a LocalDefId instead of a DefId?

Also, you should be able to pull the commit "Make some report and emit errors take DefIds instead of BodyIds" into a separate PR and I can r+ that one after you've applied the feedback.

Sorry, maybe_body_id? if you meant, maybe_body_owned_by, the signature is pub fn maybe_body_owned_by(self, id: LocalDefId) -> Option<BodyId>

spastorino · 2023-03-09T18:29:12Z

Getting 3 tests failures with the discussed commit, seems that we are losing some inference info for diagnostics ...

---- [ui] tests/ui/lazy-type-alias-impl-trait/branches3.rs stdout ----
diff of stderr:

1       error[E0282]: type annotations needed
-         --> $DIR/branches3.rs:8:10
+         --> $DIR/branches3.rs:8:13
3          |
4       LL |         |s| s.len()
-          |          ^  - type must be known at this point
-          |
-       help: consider giving this closure parameter an explicit type
-          |
-       LL |         |s: /* Type */| s.len()
-          |           ++++++++++++
+          |             ^ cannot infer type
11
12      error[E0282]: type annotations needed
-         --> $DIR/branches3.rs:15:10
+         --> $DIR/branches3.rs:15:13
14         |
15      LL |         |s| s.len()
-          |          ^  - type must be known at this point
-          |
-       help: consider giving this closure parameter an explicit type
-          |
-       LL |         |s: /* Type */| s.len()
-          |           ++++++++++++
+          |             ^ cannot infer type
22
23      error[E0282]: type annotations needed
-         --> $DIR/branches3.rs:23:10
+         --> $DIR/branches3.rs:23:13
25         |
26      LL |         |s| s.len()
-          |          ^  - type must be known at this point
-          |
-       help: consider giving this closure parameter an explicit type
-          |
-       LL |         |s: /* Type */| s.len()
-          |           ++++++++++++
+          |             ^ cannot infer type
33
34      error[E0282]: type annotations needed
-         --> $DIR/branches3.rs:30:10

spastorino · 2023-03-09T23:38:18Z

@compiler-errors removed from this PR what we've placed in #108945 and this doesn't need any of the previous PRs either, so, I've just left the last commit in here and it's ready to be reviewed.

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

…using-hir-id, r=compiler-errors Make some report and emit errors take DefIds instead of BodyIds Breaking off from rust-lang#108915 r? `@compiler-errors`

spastorino · 2023-03-10T13:59:25Z

@compiler-errors this b7d74f4 is the only meaningful commit in this PR.
I was working on default-body-with-rpit.rs test case and seeing ICEs due to accessing the HIR for these def_ids that doesn't have HIR.
The current result of running default-body-with-rpit.rs is what I've shared in the description of the issue which is clearly a different problem.

I think we could merge this as is or with more commits that fix that test case, but I think this is good and process as is.

Maybe we want some kind of comment in the opt_rpitit_info part of on_unimplemented_note.

spastorino · 2023-03-12T13:55:31Z

@compiler-errors, this one was already ready for a review :).

compiler-errors · 2023-03-14T03:49:59Z

@bors r+ rollup

bors · 2023-03-14T03:50:02Z

📌 Commit b7d74f4e7ac76627a6807933e1dc2cc93b49eda0 has been approved by compiler-errors

It is now in the queue for this repository.

spastorino · 2023-03-14T14:39:41Z

@bors r=compiler-errors

Noticed that after opt_rpitit_info is not a query anymore, I needed to explicitly convert LocalDefId to DefId by adding a to_def_id() call.

bors · 2023-03-14T14:39:43Z

📌 Commit 4824363 has been approved by compiler-errors

It is now in the queue for this repository.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#108722 (Support for Fuchsia RISC-V target) - rust-lang#108880 (Remove tests/ui/impl-trait/in-trait/new-lowering-strategy in favor of using revisions on existing tests) - rust-lang#108909 (Fix object safety checks for new RPITITs) - rust-lang#108915 (Remove some direct calls to local_def_id_to_hir_id on diagnostics) - rust-lang#108923 (Make fns from other crates with RPITIT work for -Zlower-impl-trait-in-trait-to-assoc-ty) - rust-lang#109101 (Fall back to old metadata computation when type references errors) - rust-lang#109105 (Don't ICE for late-bound consts across `AnonConstBoundary`) - rust-lang#109110 (Don't codegen impossible to satisfy impls) - rust-lang#109116 (Emit diagnostic when calling methods on the unit type in method chains) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned compiler-errors Mar 8, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 8, 2023

cjgillot reviewed Mar 8, 2023

View reviewed changes

compiler/rustc_middle/src/hir/map/mod.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/hir/map/mod.rs Outdated Show resolved Hide resolved

compiler-errors reviewed Mar 8, 2023

View reviewed changes

compiler/rustc_middle/src/hir/map/mod.rs Outdated Show resolved Hide resolved

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs Outdated Show resolved Hide resolved

compiler-errors reviewed Mar 8, 2023

View reviewed changes

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs Outdated Show resolved Hide resolved

spastorino force-pushed the new-rpitit-8 branch 2 times, most recently from 243606e to eed78d8 Compare March 9, 2023 02:46

spastorino force-pushed the new-rpitit-8 branch from eed78d8 to a0256c1 Compare March 9, 2023 15:09

compiler-errors reviewed Mar 9, 2023

View reviewed changes

spastorino force-pushed the new-rpitit-8 branch from a0256c1 to dd49661 Compare March 9, 2023 18:16

spastorino mentioned this pull request Mar 9, 2023

Make some report and emit errors take DefIds instead of BodyIds #108945

Merged

spastorino force-pushed the new-rpitit-8 branch from dd49661 to 57474ec Compare March 9, 2023 23:28

spastorino marked this pull request as ready for review March 9, 2023 23:36

compiler-errors reviewed Mar 9, 2023

View reviewed changes

compiler/rustc_trait_selection/src/traits/error_reporting/on_unimplemented.rs Outdated Show resolved Hide resolved

spastorino force-pushed the new-rpitit-8 branch from 57474ec to b7d74f4 Compare March 10, 2023 13:54

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 Mar 14, 2023

Remove some direct calls to local_def_id_to_hir_id on diagnostics

4824363

spastorino force-pushed the new-rpitit-8 branch from b7d74f4 to 4824363 Compare March 14, 2023 14:38

matthiaskrgr mentioned this pull request Mar 14, 2023

Rollup of 9 pull requests #109130

Merged

bors merged commit 6e3a3de into rust-lang:master Mar 14, 2023

rustbot added this to the 1.70.0 milestone Mar 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove some direct calls to local_def_id_to_hir_id on diagnostics #108915

Remove some direct calls to local_def_id_to_hir_id on diagnostics #108915

spastorino commented Mar 8, 2023 •

edited

Loading

cjgillot left a comment

spastorino commented Mar 9, 2023

compiler-errors commented Mar 9, 2023 •

edited

Loading

spastorino commented Mar 9, 2023

spastorino commented Mar 9, 2023

compiler-errors left a comment

spastorino commented Mar 9, 2023

spastorino commented Mar 9, 2023 •

edited

Loading

spastorino commented Mar 9, 2023 •

edited

Loading

spastorino commented Mar 10, 2023 •

edited

Loading

spastorino commented Mar 12, 2023 •

edited

Loading

compiler-errors commented Mar 14, 2023

bors commented Mar 14, 2023

spastorino commented Mar 14, 2023

bors commented Mar 14, 2023

Remove some direct calls to local_def_id_to_hir_id on diagnostics #108915

Remove some direct calls to local_def_id_to_hir_id on diagnostics #108915

Conversation

spastorino commented Mar 8, 2023 • edited Loading

cjgillot left a comment

Choose a reason for hiding this comment

spastorino commented Mar 9, 2023

compiler-errors commented Mar 9, 2023 • edited Loading

spastorino commented Mar 9, 2023

spastorino commented Mar 9, 2023

compiler-errors left a comment

Choose a reason for hiding this comment

spastorino commented Mar 9, 2023

spastorino commented Mar 9, 2023 • edited Loading

spastorino commented Mar 9, 2023 • edited Loading

spastorino commented Mar 10, 2023 • edited Loading

spastorino commented Mar 12, 2023 • edited Loading

compiler-errors commented Mar 14, 2023

bors commented Mar 14, 2023

spastorino commented Mar 14, 2023

bors commented Mar 14, 2023

spastorino commented Mar 8, 2023 •

edited

Loading

compiler-errors commented Mar 9, 2023 •

edited

Loading

spastorino commented Mar 9, 2023 •

edited

Loading

spastorino commented Mar 9, 2023 •

edited

Loading

spastorino commented Mar 10, 2023 •

edited

Loading

spastorino commented Mar 12, 2023 •

edited

Loading