[dart2wasm] Argument types are not checked in dynamic invocations #50367
Comments
|
Another repro: void test(l1) {
l1.insert(null, 5);
}
void main() {
test([0, 1, 2, 3, 4]);
}AOT output: dart2wasm traps: |
|
My plan to fix this:
I think that's it. Any feedback before I start implementing this @askeksa-google @joshualitt? |
|
If you are implementing this as part of the existing dynamic call mechanism, then we should be able to always emit a direct call, so it's not necessary to put the type checking trampoline into the dispatch table. The existing dynamic call code already has a concept of emitting trampolines to share code between calls of the same selector in most cases. Would it be possible to reuse that mechanism for the type checking trampolines (i.e. do the type check inside the existing trampoline) instead of adding a separate kind of reference? In cases where we can't use a trampoline, the checks can be performed inline in the call like the existing checks. Basically this amounts to just adding the type checks to the call code wherever that is emitted. |
Hm, right, I think this should be possible and it'll be simpler too. I'll implement this. I was assuming we want to avoid duplicating type checking code every time a member is called in dynamic invocation ( |
|
For the type checks I was hoping to use makeType to push the class C<A> {
A _a;
C(this._a);
void setA(A a) {
_a = a;
}
}
void main() {
dynamic a = C<int>(123);
a.setA(123);
}In To allow specifying how a type parameter should be resolved in w.ValueType makeType(CodeGenerator codeGen, DartType type,
void instantiateTypeParameter(TypeParameter parameter)) { ... }This works fine but reveals another problem: So we need to decouple |
|
I think your idea of generating type testing stubs via a special kind of reference would solve this. Then the generation of that stub can set up the proper environment for This is somewhat similar to how the VM does it, actually. It also has special stubs in front of methods to type check arguments for dynamic calls. |
Good point. I tried decoupling sdk/pkg/dart2wasm/lib/types.dart Lines 408 to 423 in e970e6c This allocates parameter list for the function type, and the list elements are compiled from expressions. So I think it would probably be easier to refactor Before doing that, it should simplify the code a little bit it we split |
I just realized it's actually very easy to create a new |
|
It could make sense to refactor some of the code generator at some point, yes. Is any of that necessary with the type checking stub approach, though? |
This works and it takes only a few lines, but the refactoring I mentioned above in w.ValueType makeType(CodeGenerator codeGen, DartType type,
void instantiateTypeParameter(TypeParameter parameter)) { ... }is a bit difficult to do because Example call stack: At the bottom We could try making Maybe the stub approach is the best. |
|
I realized as I read the code a bit more that generating type-checking wrapper functions (AOT calls these "dynamic invocation forwarders") is actually much simpler than my original plan above. First, (not quite a blocker for this issue but a simplification) we don't need dispatch table or selectors to compile dynamic invocations. We should decouple dispatch table and dynamic invocation code. I will do that first. (I suspect the reason why we used dispatch table was because we thought dynamic calls need to go through the dispatch table in runtime, but that's not the case. I'm updating indirect calls via the dispatch table with direct calls in https://dart-review.googlesource.com/c/sdk/+/271322. Another CL in progress to remove rest of the uses of selectors and dispatch table.) After that, we add a pass before code generation for members to create new references for dynamic invocation forwarders for members, using the The rest should be trivial. When compiling a forwarder we generate type checks using |
|
Sounds like a good plan! A few thoughts...
I would expect that a separate pass is not needed here. The stubs can be requested lazily by the dynamic call generation by passing the dynamic stub reference to
Use |
We discussed this offline but just to document it here, the pass is to map a member name to the forwarder function that will check the class ID, then check the arguments type, and then call the actual member. In https://github.com/osa1/sdk/tree/dart2wasm_forwarders I have a prototype that again blocked by In that branch I wanted to avoid adding a new pass and I kept using For each member that can be dynamically accessed (in a dynamic set, get, or invocation) I generate a function that checks receiver class ID with the target class IDs, and when a match is found I generate the shape check (number of arguments must match) and then type checks for the arguments before calling the member. The problem is these forwarder functions are not class members, so So what we really want to do is, we still want functions for dynamic invocations of members, but type checks need to be done by instance methods of the receivers, which will have access to For example, suppose we have this class: class C<A> {
void f<B>(A a, B b) { ... }
}and this dynamic invocation: (C<int>() as dynamic).f<bool>(true, "test")The type tests will compare the argument type For Where to compare the call site and member shapes? When we find the class matching the receiver's class ID, we need to check that the member and the call sites have compatible "shapes", i.e. it should have required positional parameters, should either have no type args or the same number of type args as the member, it should have required named arguments and no extra named arguments. This check can be done in the forwarder or in the type-checking members. The code should not be too complicated, so we won't need Note that if we implement shape checking in the forwarder, then the type-checking methods can take type arguments, instead of taking a Wasm array of types representing the type arguments, but this probably won't simplify the type checks. In the example above, So for now I'm thinking of implementing simple forwarders and check everything (shape and types) in instance members. |
|
I almost finished implementing the plan above in https://github.com/osa1/sdk/tree/dart2wasm_forwarders_2. Type checking of named arguments implemented as well. However type checking with type parameters do not work as expected yet. It needs a bit more debugging.. |
|
Type checking works nicely as well. I just need to implement dynamic sets and then we can start testing and refactoring. |
This reimplements dynamic call code generation to add support for type
checking, named parameters (optional and required), and fixes a few
related bugs on the way.
Currently we do not try to be as efficient as possible. The goal with
this patch to implement it correctly.
Summary of the changes:
- For every dynamic access kind and member name, we generate a new
"forwarder" function. Dynamic gets, sets, and invocations are compiled
to calls to the forwarders with the right access kind (invocation,
get, set) and member name.
For example, if the program has dynamic invocation of a member "f", we
create an "invocation forwarder for f". If it has a dynamic get of a
member "x", we generate "getter forwarder for x".
- Forwarder functions take 4 arguments:
- Receiver of the invocation, get, or set.
- A Dart list for type arguments in the invocation. For gets and sets
the list is empty.
- A Dart list for positional arguments in the invocation. For gets the
list is empty. For sets, the list only has one element.
- A Dart list for named arguments. For gets and sets the list is
empty. The list has alternating elements of type `Symbol` and
`Object?`, for the name and value of the named parameters.
- A forwarder function compares receiver class ID with the potential
targets of the call. When it finds a match, if compares the callee
"shape" with the parameters passed in the call site.
As it compares the shapes it adjusts argument lists:
- Creates default values for missing optional and named arguments
- Reorders the named argument list to match order expected by the
callee
If it can't find a matching class ID and a member with the right name
and shape, it calls `noSuchMethod` on the receiver.
If it finds a matching class ID and a member, it calls the "type
checker" for the member, passing the original receiver and adjusted
argument lists.
- A "type checker" implements argument type checking for a member, and
it's a member of the same class as the member it's checking types
for. This is to allow accessing class-bound type parameters when
generating type checking code.
- Type checking is implemented using `_isSubtype` on arguments in the
lists.
- When type checking is successful a type checker calls the original
member, passing the arguments as expected by the member.
If type checking is unsuccessful it throws a type error.
Most of the changes are for generating Wasm functions that compare
shapes, adjusts argument lists, and checks types.
Changes to members:
- `Translator.dynamics` fields is renamed to `Translator.forwarders`
- New field `Translator.forwarderFunctionType` added for the Wasm
function type of forwarder and type checker functions.
- Two new code gen utilities added:
- `Translator.indexArray`: generates code that indexes a Dart array
- `Translator.getArrayLength`: generates code that gets length of a
Dart array
- New `Reference` extensions added to get type checker function
reference of members
- New library `named_parameters` implements two helper functions for
dealing with named argument lists
- The library `dynamic_dispatch` is rewritten and now consists of two
classes:
- `Forwarders`: maintains mapping from call kind (get, set,
invocation) and member name to forwarder functions.
- `Forwarder`: a single forwarder, implements code generation for
forwarder functions.
- `CodeGenerator` gets 3 new members:
- `_callForwader` generates call to a forwarder
- `generateSetterTypeCheckerMethod` generates code for a type checker
of a setter function.
- `generateInvocationTypeCheckerMethod` generates code for a type
checker of a method.
(TODO: This should be renamed to `generateMethodTypeCheckerMethod`
for consistency)
Fixes dart-lang#50367
Change-Id: I2b9d84237c8517bd217166d8acb67e025f0498fb
This causes a few test failures and I was able to come up with two tiny repros, one causes Wasm trap, one crashes in compile time.
This one fails with Wasm trap:
If I add an optional parameter to the method then it crashes in compile time:
The text was updated successfully, but these errors were encountered: