Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VM not instantiating closure tear off type parameters to bounds #54426

Closed
biggs0125 opened this issue Dec 20, 2023 · 4 comments
Closed

VM not instantiating closure tear off type parameters to bounds #54426

biggs0125 opened this issue Dec 20, 2023 · 4 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. P2 A bug or feature request we're likely to work on triaged Issue has been triaged by sub team

Comments

@biggs0125
Copy link

class A<P> {
  final List bar2TypeArguments = [];

  void foo<Q1>() {
    void bar<T1>(List<T1>? arg) {
      void bar2<T2 extends Map<Q1, T1>>(List<T2>? arg) {
        bar2TypeArguments..add(T2);
      }

      dynamic f = bar2;
      f(null);
    }

    // Call with explicit type arguments.
    bar<int>(null);
  }
}

void main() {
  final a = new A<int>();
  a.foo<num>();
  print(a.bar2TypeArguments);  // Expect "[Map<num, int>]" but get "[Map<dynamic, dynamic>]"
}

In this test A.foo.Q1 is instantiated to num and A.foo.bar.T1 is instantiated to int. So when bar2 gets torn-off and invoked dynamically, A.foo.bar.bar2.T2 should be instantiated to its bounds which is Map<A.foo.Q1, A.foo.bar.T1> or in this context Map<num, int>.

Failing test
Test introduced in this CL

@biggs0125 biggs0125 added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Dec 20, 2023
@alexmarkov
Copy link
Contributor

It looks like we're not instantiating default type arguments during dynamic closure calls. /cc @sstrickl

@a-siva a-siva added P2 A bug or feature request we're likely to work on triaged Issue has been triaged by sub team labels Dec 21, 2023
@a-siva
Copy link
Contributor

a-siva commented Dec 21, 2023

@sstrickl I assume you are going to pick this up.

copybara-service bot pushed a commit that referenced this issue Jan 3, 2024
from the isolate stress test list as it is failing

(see #54426)

Change-Id: I7f5df7faa2cdc633901e54829ff9a7ccd824f5a9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/344461
Reviewed-by: Alexander Aprelev <aam@google.com>
Commit-Queue: Siva Annamalai <asiva@google.com>
@mkustermann
Copy link
Member

@sstrickl I assume you are going to pick this up.

@a-siva in that case we should assign @sstrickl :)

@sstrickl
Copy link
Contributor

sstrickl commented Jan 4, 2024

So looking at the code, when we instantiate default type arguments inside a closure function, we use the same code as other types of functions. That means we use the instantiator and function type arguments passed to the function to instantiate the default type arguments, instead of using the instantiator type arguments and parent function type arguments stored in the closure if this is a closure function.

(We do properly use them to instantiate the default type arguments during the type checking that happens within the invoke field dispatcher that handles the dynamic closure call, but that's only used for type checking within the dispatcher and the original function type arguments, if any, are passed through unchanged.)

So the fix is relatively straightforward. However, I'm taking a deeper look at it because the simplest fix would cause us to recompute this every time the closure is called. Instead, we should probably instead precompute and cache the instantiation of the default type arguments at closure creation time, trading off a bit of space in the closure object to avoid repeated TAV instantiation within the closure function and also removing the instantiation done within the invoke field dispatcher with a simple field load instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. P2 A bug or feature request we're likely to work on triaged Issue has been triaged by sub team
Projects
None yet
Development

No branches or pull requests

5 participants