Fix default methods with return-position `impl Trait` in trait #107013

compiler-errors · 2023-01-17T23:52:21Z

Removes this weird hack in projection... and replaces it with some other hacks in HIR and MIR typeck.

r? types

Specifically, we should only be able to observe the opaque for a default method in the body of that method itself. For example:

trait Foo {
  async fn foo() {
    // The return type of `foo()` should be treated like an opaque ty here, so we can do inference.
  }

  fn bar() {
    let _ = Self::foo();
    // The return type of `foo()` must remain a projection here, so that when we monomorphize `bar` for some `Self` type, we dispatch to the right `foo` function.
  }
}

compiler-errors · 2023-01-17T23:57:18Z

@bors try @rust-timer queue

bors · 2023-01-17T23:57:27Z

⌛ Trying commit 5e7bb83 with merge e76d8749244270bde4f1e2b60717e8aa5873cfd4...

compiler-errors · 2023-01-17T23:58:37Z

compiler/rustc_borrowck/src/type_check/input_output.rs

@@ -148,6 +150,22 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {

    #[instrument(skip(self), level = "debug")]
    fn equate_normalized_input_or_output(&mut self, a: Ty<'tcx>, b: Ty<'tcx>, span: Span) {
+        let (a, b) = (a, b).fold_with(&mut BottomUpFolder {


Within a method, treat its own RPITITs as ty::Opaque.

This probably could be a custom folder so it's more efficient... perf will tell.

compiler-errors · 2023-01-17T23:59:15Z

compiler/rustc_infer/src/infer/nll_relate/mod.rs

@@ -627,6 +628,42 @@ where
                self.relate_opaques(a, b)
            }

+            // Handle default-body RPITITs


Within a method, relating one of that method's RPITITs against a concrete type should act just like opaque type inference.

Looked at this for ~10 minutes and I don't understand it '^^ would probably be best to chat about this in sync next week

compiler-errors · 2023-01-17T23:59:44Z

compiler/rustc_infer/src/infer/opaque_types.rs

@@ -52,7 +52,7 @@ impl<'tcx> InferCtxt<'tcx> {
        span: Span,
        param_env: ty::ParamEnv<'tcx>,
    ) -> InferOk<'tcx, T> {
-        if !value.has_opaque_types() {
+        if !value.has_projections() {


We need to handle either RPITITs (projections) and opaques... hopefully this doesn't hurt perf too much :/

bors · 2023-01-18T02:26:07Z

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

bors · 2023-01-18T02:26:07Z

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

rust-timer · 2023-01-18T07:35:02Z

Finished benchmarking commit (e76d8749244270bde4f1e2b60717e8aa5873cfd4): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.9%	[0.9%, 0.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.1%	[3.1%, 3.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

lcnr · 2023-02-08T13:22:54Z

tests/ui/async-await/in-trait/async-default-fn-overridden.run.stdout

@@ -0,0 +1 @@
+B


instead of printing stuff return a bool and assert that it is correct?

lcnr · 2023-02-08T13:30:30Z

compiler/rustc_infer/src/infer/nll_relate/mod.rs

@@ -627,6 +628,42 @@ where
                self.relate_opaques(a, b)
            }

+            // Handle default-body RPITITs


Looked at this for ~10 minutes and I don't understand it '^^ would probably be best to chat about this in sync next week

lcnr · 2023-02-15T09:48:56Z

talked about this in sync.

the current approach is not quite right. We probably want to extend the param env with a Projection(RetTyProjection, RetTyOpaque) predicate instead.

@rustbot author

…lt-constraint, r=oli-obk Add a test for default trait method with RPITITs This didn't work in rust-lang#107013, but now that rust-lang#108203 has landed, let's make sure we don't regress it. r? types

compiler-errors added 4 commits January 17, 2023 23:36

Remove default trait RPITIT candidates

6fe1f9f

Teach HIR typeck how to deal with default-body RPITITs

9f76362

Teach MIR borrowck how to deal with default body RPITITs

52d847b

Add test

5e7bb83

rustbot assigned lcnr Jan 17, 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 Jan 17, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 17, 2023

compiler-errors commented Jan 18, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 18, 2023

LastExceed mentioned this pull request Jan 29, 2023

Unexpected behaviour when calling associated async function of a trait with default implementations #107002

Closed

compiler-errors mentioned this pull request Jan 31, 2023

async_fn_in_trait: default fn impl calling fn of same trait with default impl doesn't dispatch properly #107528

Closed

fasterthanlime mentioned this pull request Jan 31, 2023

WriteOwned::writev: Return early in case of partial writes bearcove/loona#74

Merged

lcnr reviewed Feb 8, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 15, 2023

compiler-errors closed this Feb 15, 2023

This was referenced Feb 16, 2023

Remove support for defaulted trait methods with async or return-position impl Trait #108142

Closed

Add a test for default trait method with RPITITs #108290

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix default methods with return-position `impl Trait` in trait #107013

Fix default methods with return-position `impl Trait` in trait #107013

compiler-errors commented Jan 17, 2023 •

edited

Loading

compiler-errors commented Jan 17, 2023

This comment has been minimized.

bors commented Jan 17, 2023

compiler-errors Jan 17, 2023

compiler-errors Jan 17, 2023 •

edited

Loading

lcnr Feb 8, 2023

compiler-errors Jan 17, 2023

bors commented Jan 18, 2023

bors commented Jan 18, 2023

This comment has been minimized.

rust-timer commented Jan 18, 2023

lcnr Feb 8, 2023 •

edited

Loading

lcnr Feb 8, 2023

lcnr commented Feb 15, 2023 •

edited

Loading

Fix default methods with return-position impl Trait in trait #107013

Fix default methods with return-position impl Trait in trait #107013

Conversation

compiler-errors commented Jan 17, 2023 • edited Loading

compiler-errors commented Jan 17, 2023

This comment has been minimized.

bors commented Jan 17, 2023

compiler-errors Jan 17, 2023

Choose a reason for hiding this comment

compiler-errors Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

lcnr Feb 8, 2023

Choose a reason for hiding this comment

compiler-errors Jan 17, 2023

Choose a reason for hiding this comment

bors commented Jan 18, 2023

bors commented Jan 18, 2023

This comment has been minimized.

rust-timer commented Jan 18, 2023

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

lcnr Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

lcnr Feb 8, 2023

Choose a reason for hiding this comment

lcnr commented Feb 15, 2023 • edited Loading

Fix default methods with return-position `impl Trait` in trait #107013

Fix default methods with return-position `impl Trait` in trait #107013

compiler-errors commented Jan 17, 2023 •

edited

Loading

compiler-errors Jan 17, 2023 •

edited

Loading

lcnr Feb 8, 2023 •

edited

Loading

lcnr commented Feb 15, 2023 •

edited

Loading