Fix late-bound ICE #92289

terrarier2111 · 2021-12-26T10:44:30Z

rust-highfive · 2021-12-26T10:44:33Z

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

terrarier2111 · 2021-12-26T10:47:11Z

I am not sure tho if the long "FIXME" comment is still correct after this or if this is even the correct way to fix the issue, as one test changed significantly with this fix applied.

terrarier2111 · 2022-01-08T09:10:42Z

@wesleywiser Could you take a look at this?

wesleywiser · 2022-01-10T19:42:29Z

compiler/rustc_typeck/src/check/check.rs

        fcx.require_type_is_sized(declared_ret_ty, decl.output.span(), traits::SizedReturnType);
+        fcx.check_expr(&body.value);


@estebank I see you added this branch in e536257 including the FIXME. Did you add the FIXME because it would be great to unify the ty::Dynamic case with all the others and remove the check or for another reason? Just trying to figure out what we'd like to have fixed. 🙂

@wesleywiser correct, before my change this was branchless code. I want it to return to being branchless code while also supporting the improved diagnostics.

src/test/ui/unsized/expect_unsized_return_sized.rs

wesleywiser · 2022-01-10T19:48:10Z

src/test/ui/impl-trait/point-to-type-err-cause-on-impl-trait-return.stderr

-   = note: for information on trait objects, see <https://doc.rust-lang.org/book/ch17-02-trait-objects.html#using-trait-objects-that-allow-for-values-of-different-types>
-   = note: if all the returned values were of the same type you could use `impl std::fmt::Display` as the return type
-   = note: for information on `impl Trait`, see <https://doc.rust-lang.org/book/ch10-02-traits.html#returning-types-that-implement-traits>
-   = note: you can create a new `enum` with a variant for each returned type


cc @estebank This PR resolves an ICE but causes some changes to diagnostics. Most of the notes appear to be more or less present as help messages instead but this note about creating a new enum is missing. Do you have concerns with merging this PR?

Given this fixes an ICE, lets merge, but I at least I would open a ticket pointing at this PR about getting these back :-/

wesleywiser · 2022-03-14T13:32:55Z

Since this is waiting on @estebank to take a look, I'm going to reassign.

r? @estebank

estebank · 2022-03-25T00:52:19Z

Apologies for the delays in getting back to this. @terrarier2111 if you have a bit of time, would it be possible to see how removing the fixme if/else and instead do what we used to do (just what is in the else branch) affects the diagnostics?

estebank · 2022-03-25T00:53:49Z

src/test/ui/impl-trait/point-to-type-err-cause-on-impl-trait-return.stderr

-LL ~ fn pug() -> Box<dyn std::fmt::Display> {
-LL |     match 13 {
-LL ~         0 => Box::new(0i32),
-LL ~         1 => Box::new(1u32),
-LL ~         _ => Box::new(2u32),
-   |
-


I guess this is the only output we'd lose, right? Can we check whether this suggestion in particular still triggers anywhere else?

At least the help "return a boxed trait object instead" triggers in dyn-trait-return-should-be-impl-trait

terrarier2111 · 2022-03-26T09:58:35Z

Apologies for the delays in getting back to this. @terrarier2111 if you have a bit of time, would it be possible to see how removing the fixme if/else and instead do what we used to do (just what is in the else branch) affects the diagnostics?

The "match arms have incompatible types" and "if and else have incompatible types" warnings in "point-to-type-err-cause-on-impl-trait-return" go away and the "if and else have incompatible types" warning in "dyn-trait-return-should-be-impl-trait" goes away, everything else stays as-is.

Fix late-bound ICE in `dyn` return type suggestion This fixes the root-cause of the attached issues -- the root problem is that we're using the return type from a signature with late-bound instead of early-bound regions. The change on line 1087 (`let Some(liberated_sig) = typeck_results.liberated_fn_sigs().get(fn_hir_id) else { return false; };`) makes sure we're grabbing the _right_ return type for this suggestion to check the `dyn` predicates with. Fixes rust-lang#91801 Fixes rust-lang#91803 This fix also includes some drive-by changes, specifically: 1. Don't suggest boxing when we have `-> dyn Trait` and are already returning `Box<T>` where `T: Trait` (before we always boxed the value). 2. Suggestion applies even when the return type is a type alias (e.g. `type Foo = dyn Trait`). This does cause the suggestion to expand to the aliased type, but I think it's still beneficial. 3. Split up the multipart suggestion because there's a 6-line max in the printed output... I am open to splitting out the above changes, if we just want to fix the ICE first. cc: `@terrarier2111` and rust-lang#92289

Fix late-bound ICE in `dyn` return type suggestion This fixes the root-cause of the attached issues -- the root problem is that we're using the return type from a signature with late-bound instead of early-bound regions. The change on line 1087 (`let Some(liberated_sig) = typeck_results.liberated_fn_sigs().get(fn_hir_id) else { return false; };`) makes sure we're grabbing the _right_ return type for this suggestion to check the `dyn` predicates with. Fixes rust-lang#91801 Fixes rust-lang#91803 This fix also includes some drive-by changes, specifically: 1. Don't suggest boxing when we have `-> dyn Trait` and are already returning `Box<T>` where `T: Trait` (before we always boxed the value). 2. Suggestion applies even when the return type is a type alias (e.g. `type Foo = dyn Trait`). This does cause the suggestion to expand to the aliased type, but I think it's still beneficial. 3. Split up the multipart suggestion because there's a 6-line max in the printed output... I am open to splitting out the above changes, if we just want to fix the ICE first. cc: ``@terrarier2111`` and rust-lang#92289

Fix late-bound ICE in `dyn` return type suggestion This fixes the root-cause of the attached issues -- the root problem is that we're using the return type from a signature with late-bound instead of early-bound regions. The change on line 1087 (`let Some(liberated_sig) = typeck_results.liberated_fn_sigs().get(fn_hir_id) else { return false; };`) makes sure we're grabbing the _right_ return type for this suggestion to check the `dyn` predicates with. Fixes rust-lang#91801 Fixes rust-lang#91803 This fix also includes some drive-by changes, specifically: 1. Don't suggest boxing when we have `-> dyn Trait` and are already returning `Box<T>` where `T: Trait` (before we always boxed the value). 2. Suggestion applies even when the return type is a type alias (e.g. `type Foo = dyn Trait`). This does cause the suggestion to expand to the aliased type, but I think it's still beneficial. 3. Split up the multipart suggestion because there's a 6-line max in the printed output... I am open to splitting out the above changes, if we just want to fix the ICE first. cc: ```@terrarier2111``` and rust-lang#92289

bors · 2022-04-05T05:01:12Z

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

pnkfelix · 2022-04-14T14:22:24Z

Visiting at T-compiler triage meeting.

We believe the root cause (of the issues addressed by this PR, namely #91803 and #91801) has since been independently addressed by PR #95603.

Therefore, we are closing this PR as no longer necessary.

Thank you so much for your contribution, @terrarier2111 ! and also, if this summary is incorrect, and this PR is providing new value that is not addressed by PR #95603, please do feel free to reopen this PR!

rust-highfive assigned wesleywiser Dec 26, 2021

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 26, 2021

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

wesleywiser reviewed Jan 10, 2022

View reviewed changes

Fix late-bound ICE

42b26cd

terrarier2111 force-pushed the fix-late-bound-ice branch from 63a5634 to 42b26cd Compare January 16, 2022 10:29

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 Feb 27, 2022

rust-highfive assigned estebank and unassigned wesleywiser Mar 14, 2022

estebank reviewed Mar 25, 2022

View reviewed changes

compiler-errors mentioned this pull request Apr 2, 2022

Fix late-bound ICE in dyn return type suggestion #95603

Merged

pnkfelix closed this Apr 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix late-bound ICE #92289

Fix late-bound ICE #92289

terrarier2111 commented Dec 26, 2021

rust-highfive commented Dec 26, 2021

terrarier2111 commented Dec 26, 2021

terrarier2111 commented Jan 8, 2022

wesleywiser Jan 10, 2022

estebank Mar 25, 2022

wesleywiser Jan 10, 2022

estebank Mar 25, 2022

wesleywiser commented Mar 14, 2022

estebank commented Mar 25, 2022

estebank Mar 25, 2022

terrarier2111 Mar 26, 2022

terrarier2111 commented Mar 26, 2022

bors commented Apr 5, 2022

pnkfelix commented Apr 14, 2022 •

edited

Loading

		fcx.require_type_is_sized(declared_ret_ty, decl.output.span(), traits::SizedReturnType);
		fcx.check_expr(&body.value);

Fix late-bound ICE #92289

Fix late-bound ICE #92289

Conversation

terrarier2111 commented Dec 26, 2021

rust-highfive commented Dec 26, 2021

terrarier2111 commented Dec 26, 2021

terrarier2111 commented Jan 8, 2022

wesleywiser Jan 10, 2022

Choose a reason for hiding this comment

estebank Mar 25, 2022

Choose a reason for hiding this comment

wesleywiser Jan 10, 2022

Choose a reason for hiding this comment

estebank Mar 25, 2022

Choose a reason for hiding this comment

wesleywiser commented Mar 14, 2022

estebank commented Mar 25, 2022

estebank Mar 25, 2022

Choose a reason for hiding this comment

terrarier2111 Mar 26, 2022

Choose a reason for hiding this comment

terrarier2111 commented Mar 26, 2022

bors commented Apr 5, 2022

pnkfelix commented Apr 14, 2022 • edited Loading

pnkfelix commented Apr 14, 2022 •

edited

Loading