[M] Change in dart/sdk[main]: [ddc] Updating tearoffs to be evaluated on access.
Mark Zhou (Gerrit)
Mark Zhou added 1 comment![]( "Open in Gerrit")
(error) => error.toString().contains("NoSuchMethodError: 'arity1'"),@nsh...@google.com
The VM outputs this:
```
Unhandled exception:
NoSuchMethodError: Closure call with mismatched arguments: function 'A.arity1'
Receiver: Closure: (int) => String from Function 'arity1':.
Tried calling: A.arity1()
Found: A.arity1(int) => String
```
And actually crashes on this test. Should we just delete 'bound'?
Related details
- Nate Biggs
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Shahan (Gerrit)
Nicholas Shahan added 10 comments![]( "Open in Gerrit")
final List<js_ast.Statement> _functionLinks = [];WDYT about making the name a little more descriptive, `_functionSignatureLinks`?
..._typeRuleLinks,
..._functionLinks,`typeRuleLinks` ended up here because of a bootstrap ordering issue between the `dart:_runtime` and the `dart:_rti` libraries. That should have been noted in a comment at the time. I suspect that is the same reason the signatures are being declared here as well. Could you add a comment for these two?
That said, I'm curious what
_functionLinks.add(_emitFunctionTagged(nameExpr,
p.function.computeThisFunctionType(p.enclosingLibrary.nonNullable))
.toStatement());Is this adding a signature to every top level function in a library? I belive now we only add signatures when the function appears in the const table because it was torn off.
(js_ast.Expression, js_ast.Expression) _emitStaticTargetAsAccessor(
Member target) {I don't love the idea of duplicating the logic here just to package the return value differently. Maybe we should refactor to share the logic more?
tearoff(context, property) {Is this only used for tearoff of static methods? If so, let's update the name and/or comment to make it more clear.
// TODO(markzipan): canonicalize tearoffs so we can use JS object identity
// for 'equals' and 'identical' checks.Add more detail here for us in the future. Ex: What tearoffs still need to be canonicalized?
var tear = JS('', '(...args) => #[#](...args)', context, property);I'm struggling to understand if it is safe to just pass the arguments here. Does this new representation introduce a type check somewhere to catch when the type of the tearoff has changed on a hot reload?
If you call this tearoff passing the arguments that match the oringial type, does the VM catch the problem?
var rtiName = JS('', '#', JS_GET_NAME(JsGetName.SIGNATURE_NAME));It seems like there is no need for the `JS()` here?
JS('', '#._boundMethod = () => #.#', tear, context, property);Why don't we define this as a getter like we do with the signature and type args lazy functions?
(error) => error.toString().contains("NoSuchMethodError: 'arity1'"),@nsh...@google.com
The VM outputs this:
```
Unhandled exception:
NoSuchMethodError: Closure call with mismatched arguments: function 'A.arity1'
Receiver: Closure: (int) => String from Function 'arity1':.
Tried calling: A.arity1()
Found: A.arity1(int) => String
```And actually crashes on this test. Should we just delete 'bound'?
Yeah I think we can delete that because it was an artifact of us returning the result of the JavaScript `bind()` call as our tearoff value.
Related details
- Mark Zhou
- Nate Biggs
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mark Zhou added 9 comments![]( "Open in Gerrit")
final List<js_ast.Statement> _functionLinks = [];WDYT about making the name a little more descriptive, `_functionSignatureLinks`?
Done
..._typeRuleLinks,
..._functionLinks,`typeRuleLinks` ended up here because of a bootstrap ordering issue between the `dart:_runtime` and the `dart:_rti` libraries. That should have been noted in a comment at the time. I suspect that is the same reason the signatures are being declared here as well. Could you add a comment for these two?
That said, I'm curious what
Done - but did you accidentally a comment?
_functionLinks.add(_emitFunctionTagged(nameExpr,
p.function.computeThisFunctionType(p.enclosingLibrary.nonNullable))
.toStatement());Is this adding a signature to every top level function in a library? I belive now we only add signatures when the function appears in the const table because it was torn off.
Oooh right, this is redundant. Good catch - deleted.
(js_ast.Expression, js_ast.Expression) _emitStaticTargetAsAccessor(
Member target) {I don't love the idea of duplicating the logic here just to package the return value differently. Maybe we should refactor to share the logic more?
Done - refactored it to always return a `PropertyAccess` object.
Is this only used for tearoff of static methods? If so, let's update the name and/or comment to make it more clear.
This is used for both static and instance tearoffs. Static tearoffs are called via 'canonicalizedTearoff`, but I can rename that one.
// TODO(markzipan): canonicalize tearoffs so we can use JS object identity
// for 'equals' and 'identical' checks.Add more detail here for us in the future. Ex: What tearoffs still need to be canonicalized?
Done
var tear = JS('', '(...args) => #[#](...args)', context, property);I'm struggling to understand if it is safe to just pass the arguments here. Does this new representation introduce a type check somewhere to catch when the type of the tearoff has changed on a hot reload?
If you call this tearoff passing the arguments that match the oringial type, does the VM catch the problem?
Calling old closures is actually unsound. Beyond CFE-inserted static checks, you can leak in arbitrarily typed values.
var rtiName = JS('', '#', JS_GET_NAME(JsGetName.SIGNATURE_NAME));It seems like there is no need for the `JS()` here?
Done
JS('', '#._boundMethod = () => #.#', tear, context, property);Why don't we define this as a getter like we do with the signature and type args lazy functions?
Good point - I can update this and undo some of the function invocations downstream.
Related details
- Nate Biggs
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 9 comments![]( "Open in Gerrit")
var prerequisiteRtiTypes = [I'm not clear why we now need to do this for every library now. Shouldn't we only need to initialize the rti cache once when the program starts up?
_functionLinks.add(_emitFunctionTagged(nameExpr,I see _emitFunctionTagged takes a `topLevel` optional param. I think that should be passed as true here?
I commented above on you emitting the type rules for all libraries, not just the runtime. Were you were getting infinite loop errors from the rti library because the rti cache didn't contain those base types yet? If so I think passing `topLevel = true` here should make the necessary functions lazily generate the types so you don't hit the RTI cache before its been populated.
(js_ast.Expression, js_ast.Expression) _emitStaticTargetAsAccessor(
Member target) {I don't love the idea of duplicating the logic here just to package the return value differently. Maybe we should refactor to share the logic more?
+1
It looks like all the returns in `_emitStaticTarget` are returning a `js_ast.PropertyAccess`. So if you make the return type of that function a `ProprtyAccess` (you'll have to cast the `global.#.#` return) then you can just get the `receiver` and `selector` from there when you need it.
Then you can delete this copy.
if (JS<bool>('', '# == void 0', constructor)) return name;nit: I think `# == null` is usually the idiomatic way to check null/undefined (probably just because it's less characters).
/// TODO(#3612): We do not canonicalize instance tearoffs to be consistent withThis doesn't seem like a TODO so much as a note for posterity. I read this as there's no further action to be taken here (unless something external changes in the Future).
var tear = JS('', '(...args) => #[#](...args)', context, property);I'm struggling to understand if it is safe to just pass the arguments here. Does this new representation introduce a type check somewhere to catch when the type of the tearoff has changed on a hot reload?
If you call this tearoff passing the arguments that match the oringial type, does the VM catch the problem?
Won't the body of the underlying function include the necessary parametric type checks? So whatever function is bound when this is called should check the arguments are passed to it.
var tear = JS('', '(...args) => #[#](...args)', context, property);Related to Nick's point here, I'm curious about the argument count changing here. I wonder if this can put us in a similar boat to: https://github.com/dart-lang/sdk/issues/59671
Consider:
```
// main.1.dart
void foo(int a) => a + 1;
void main() {
void Function() bar() {
return () => foo(3);
}
final f = bar();
hotReload();
f();
}
``````
// main.2.dart
void foo(int a, int b) => a + b;
void main() {
void Function() bar() {
return () => foo(3, 4);
}
final f = bar();
hotReload();
f(); // <- f is from before hot reload so it only passes 1 argument to foo.
}
```If `f` is still the closure `() => foo(3)` after hot reload, then when you invoke it, it seems like it would pass the wrong # of arguments to the new `foo`. Or is the closure's context going to still have access to the old `foo`?
return existingRti ?? getMethodType(context, property);If the result isn't already cached and we're calculating it from scratch, should we cache that result?
var boundMethod = JS<Object>('!', '#._boundMethod()', this);It seems like this would cause the hashCode not to be stable pre/post hot reload. The result of `this._boundMethod()` will be the current underlying implementation when `hashCode` is called.
How about we pass the closure itself (`#._boundMethod`) to `identityHashCode`?
Basing stable hashcode expectation on `IsolateReload_StaticTearOffRetainsHash`.
Related details
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 1 comment![]( "Open in Gerrit")
Oops I didn't refresh before sending, let me clear out the ones already addressed.
Related details
- Mark Zhou
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 2 comments![]( "Open in Gerrit")
(js_ast.Expression, js_ast.Expression) _emitStaticTargetAsAccessor(
Member target) {Nate BiggsI don't love the idea of duplicating the logic here just to package the return value differently. Maybe we should refactor to share the logic more?
+1
It looks like all the returns in `_emitStaticTarget` are returning a `js_ast.PropertyAccess`. So if you make the return type of that function a `ProprtyAccess` (you'll have to cast the `global.#.#` return) then you can just get the `receiver` and `selector` from there when you need it.
Then you can delete this copy.
Nice glad you came to the same idea!
var boundMethod = JS<Object>('!', '#._boundMethod()', this);It seems like this would cause the hashCode not to be stable pre/post hot reload. The result of `this._boundMethod()` will be the current underlying implementation when `hashCode` is called.
How about we pass the closure itself (`#._boundMethod`) to `identityHashCode`?
Basing stable hashcode expectation on `IsolateReload_StaticTearOffRetainsHash`.
I think now that you're not invoking _boundMethod, this is obsolete.
Related details
- Mark Zhou
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mark Zhou added 5 comments![]( "Open in Gerrit")
var prerequisiteRtiTypes = [I'm not clear why we now need to do this for every library now. Shouldn't we only need to initialize the rti cache once when the program starts up?
We need these in `dart:_rti` as well (which is not part of the runtime) since we tag functions ahead of time. I've updated this to only apply to the SDK, which should suffice.
_functionLinks.add(_emitFunctionTagged(nameExpr,I see _emitFunctionTagged takes a `topLevel` optional param. I think that should be passed as true here?
I commented above on you emitting the type rules for all libraries, not just the runtime. Were you were getting infinite loop errors from the rti library because the rti cache didn't contain those base types yet? If so I think passing `topLevel = true` here should make the necessary functions lazily generate the types so you don't hit the RTI cache before its been populated.
I think that version of `_emitFunctionTagged` is only in the old DDC module system, but we could adopt that late getter pattern for the new module system. We'd still have to tag all functions since they might be updated after a hot reload, but once the differ lands, we could 1) tag all functions lazily and tearoff time and 2) retag functions that were tagged but reloaded.
Thoughts?
if (JS<bool>('', '# == void 0', constructor)) return name;nit: I think `# == null` is usually the idiomatic way to check null/undefined (probably just because it's less characters).
Done
/// TODO(#3612): We do not canonicalize instance tearoffs to be consistent withThis doesn't seem like a TODO so much as a note for posterity. I read this as there's no further action to be taken here (unless something external changes in the Future).
True - rephrased
var tear = JS('', '(...args) => #[#](...args)', context, property);Related to Nick's point here, I'm curious about the argument count changing here. I wonder if this can put us in a similar boat to: https://github.com/dart-lang/sdk/issues/59671
Consider:
```
// main.1.dart
void foo(int a) => a + 1;void main() {
void Function() bar() {
return () => foo(3);
}
final f = bar();
hotReload();
f();
}
``````
// main.2.dart
void foo(int a, int b) => a + b;void main() {
void Function() bar() {
return () => foo(3, 4);
}
final f = bar();
hotReload();
f(); // <- f is from before hot reload so it only passes 1 argument to foo.
}
```If `f` is still the closure `() => foo(3)` after hot reload, then when you invoke it, it seems like it would pass the wrong # of arguments to the new `foo`. Or is the closure's context going to still have access to the old `foo`?
- Nate Biggs
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mark Zhou added 1 comment![]( "Open in Gerrit")
return existingRti ?? getMethodType(context, property);If the result isn't already cached and we're calculating it from scratch, should we cache that result?
Good idea - aded
Related details
- Nate Biggs
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 1 comment![]( "Open in Gerrit")
_functionLinks.add(_emitFunctionTagged(nameExpr,Mark ZhouI see _emitFunctionTagged takes a `topLevel` optional param. I think that should be passed as true here?
I commented above on you emitting the type rules for all libraries, not just the runtime. Were you were getting infinite loop errors from the rti library because the rti cache didn't contain those base types yet? If so I think passing `topLevel = true` here should make the necessary functions lazily generate the types so you don't hit the RTI cache before its been populated.
I think that version of `_emitFunctionTagged` is only in the old DDC module system, but we could adopt that late getter pattern for the new module system. We'd still have to tag all functions since they might be updated after a hot reload, but once the differ lands, we could 1) tag all functions lazily and tearoff time and 2) retag functions that were tagged but reloaded.
Thoughts?
Okay I think this is good now that you're only including the "prerequisiteRtiTypes" in the SDK. My goal here was just to avoid that.
Related details
- Mark Zhou
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mark Zhou added 3 comments![]( "Open in Gerrit")
var tear = JS('', '(...args) => #[#](...args)', context, property);I'm struggling to understand if it is safe to just pass the arguments here. Does this new representation introduce a type check somewhere to catch when the type of the tearoff has changed on a hot reload?
If you call this tearoff passing the arguments that match the oringial type, does the VM catch the problem?
Won't the body of the underlying function include the necessary parametric type checks? So whatever function is bound when this is called should check the arguments are passed to it.
ACK - see previous reply.
var tear = JS('', '(...args) => #[#](...args)', context, property);Mark ZhouRelated to Nick's point here, I'm curious about the argument count changing here. I wonder if this can put us in a similar boat to: https://github.com/dart-lang/sdk/issues/59671
Consider:
```
// main.1.dart
void foo(int a) => a + 1;void main() {
void Function() bar() {
return () => foo(3);
}
final f = bar();
hotReload();
f();
}
``````
// main.2.dart
void foo(int a, int b) => a + b;void main() {
void Function() bar() {
return () => foo(3, 4);
}
final f = bar();
hotReload();
f(); // <- f is from before hot reload so it only passes 1 argument to foo.
}
```If `f` is still the closure `() => foo(3)` after hot reload, then when you invoke it, it seems like it would pass the wrong # of arguments to the new `foo`. Or is the closure's context going to still have access to the old `foo`?
Good catch - let me investigate this and get back to you.
Following up here, this is more of an issue with how we resolve static functions - not necessarily tearoffs. We don't re-resolve type checks when calling static functions within closures. We effectively have to run a type check on all arguments (as in dynamic calls) whenever we resolve a static function. Given how slow this is, it may be better to add this after the differ is in. I can add a test for this behavior in this change, however.
return existingRti ?? getMethodType(context, property);Mark ZhouIf the result isn't already cached and we're calculating it from scratch, should we cache that result?
Good idea - aded
Woops - I had to remove the caching here since Generics need to be resolved elsewhere. We could split this and cache non-generic types, however.
Related details
- Nate Biggs
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 1 comment![]( "Open in Gerrit")
Yeah I was thinking about this and realized it wasn't tear-off specific. If we want to reject changes to parameters until we're able to make this fix, that would be okay too. I'm not sure how trivial adding the checks will be but detecting the diff in parameters should be easy enough.
Related details
- Mark Zhou
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nate Biggs
| Code-Review | +1 |
2 comments
js.call('#', [memberName]));nit: Can we make this `js.string(memberName)`? The interpolation seems unnecessary.
// We check for '_boundMethod' in case tearoffs are returned.Would you mind patching this into a branch in webdev and make sure all the dwds tests are still passing with this change?
Related details
- Mark Zhou
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mark Zhou added 1 comment![]( "Open in Gerrit")
nit: Can we make this `js.string(memberName)`? The interpolation seems unnecessary.
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mark Zhou added 1 comment![]( "Open in Gerrit")
// We check for '_boundMethod' in case tearoffs are returned.Would you mind patching this into a branch in webdev and make sure all the dwds tests are still passing with this change?
Good catch - webdev tests should be passing as of now - bar some tests that seem to require special permissions.
Related details
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs voted and added 6 comments![]( "Open in Gerrit")
Votes added by Nate Biggs
| Code-Review | +1 |
6 comments
? _get<Object?>(object, 'constructor')At this point we should know it's non-nullable.
: _hasConstructor(object)
? _get<Object?>(object, 'constructor')
: object;nit: Can we add some parens here for readability?
_hasConstructor(object) ? _get<Object?>(object, 'constructor') : object;At this point we should know it's non-nullable.
/// Static tearoffs are canonicalized at runtime via `tearoffCace`.typo
var tear = JS('', '(...args) => #[#](...args)', context, property);Related to Nick's point here, I'm curious about the argument count changing here. I wonder if this can put us in a similar boat to: https://github.com/dart-lang/sdk/issues/59671
Consider:
```
// main.1.dart
void foo(int a) => a + 1;void main() {
void Function() bar() {
return () => foo(3);
}
final f = bar();
hotReload();
f();
}
``````
// main.2.dart
void foo(int a, int b) => a + b;void main() {
void Function() bar() {
return () => foo(3, 4);
}
final f = bar();
hotReload();
f(); // <- f is from before hot reload so it only passes 1 argument to foo.
}
```If `f` is still the closure `() => foo(3)` after hot reload, then when you invoke it, it seems like it would pass the wrong # of arguments to the new `foo`. Or is the closure's context going to still have access to the old `foo`?
Good catch - let me investigate this and get back to you.
Nate BiggsFollowing up here, this is more of an issue with how we resolve static functions - not necessarily tearoffs. We don't re-resolve type checks when calling static functions within closures. We effectively have to run a type check on all arguments (as in dynamic calls) whenever we resolve a static function. Given how slow this is, it may be better to add this after the differ is in. I can add a test for this behavior in this change, however.
Yeah I was thinking about this and realized it wasn't tear-off specific. If we want to reject changes to parameters until we're able to make this fix, that would be okay too. I'm not sure how trivial adding the checks will be but detecting the diff in parameters should be easy enough.
Resolving this for now. Added a line item to the project: https://github.com/orgs/dart-lang/projects/96/views/1?pane=issue&itemId=92215326
// TODO(markzipan): If we canonicalize all tearoffs (not just staticSame as above. Since we don't have any intention of canonicalizing all tearoffs right now, there isn't an actionable TODO here so this can just be a note.
Related details
- Mark Zhou
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mark Zhou added 5 comments![]( "Open in Gerrit")
At this point we should know it's non-nullable.
Done
: _hasConstructor(object)
? _get<Object?>(object, 'constructor')
: object;nit: Can we add some parens here for readability?
Done
_hasConstructor(object) ? _get<Object?>(object, 'constructor') : object;At this point we should know it's non-nullable.
Done
/// Static tearoffs are canonicalized at runtime via `tearoffCace`.Mark Zhoutypo
Done
// TODO(markzipan): If we canonicalize all tearoffs (not just staticSame as above. Since we don't have any intention of canonicalizing all tearoffs right now, there isn't an actionable TODO here so this can just be a note.
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nate Biggs
| Code-Review | +1 |
1 comment
/// Static tearoffs are canonicalized at runtime via `tearoffCace`.Mark Zhoutypo
Nate BiggsDone
Reopening, think this one still needs a fix.
Related details
- Mark Zhou
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mark Zhou added 1 comment![]( "Open in Gerrit")
/// Static tearoffs are canonicalized at runtime via `tearoffCace`.Mark Zhoutypo
Nate BiggsDone
Reopening, think this one still needs a fix.
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs voted Code-Review+1![]( "Open in Gerrit")
- Mark Zhou
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Shahan (Gerrit)
Nicholas Shahan added 5 comments![]( "Open in Gerrit")
if (_isBuildingSdk) {Is this change necessary? I believe this will force these types to be created in every `dart:` library instead of once for the `dart:_runtime` library.
..._typeRuleLinks,
..._functionLinks,Mark Zhou`typeRuleLinks` ended up here because of a bootstrap ordering issue between the `dart:_runtime` and the `dart:_rti` libraries. That should have been noted in a comment at the time. I suspect that is the same reason the signatures are being declared here as well. Could you add a comment for these two?
That said, I'm curious what
Done - but did you accidentally a comment?
Yeah sorry, I don't know why that last cutoff sentance was there.
if (member.isStatic && _reifyTearoff(member) && !_isBuildingSdk) {Do static methods in the SDK get types attached some other way when they are torn off?
if (!_isBuildingSdk && p.isStatic) {ditto
var tear = JS('', '(...args) => #[#](...args)', context, property);Nate BiggsI'm struggling to understand if it is safe to just pass the arguments here. Does this new representation introduce a type check somewhere to catch when the type of the tearoff has changed on a hot reload?
If you call this tearoff passing the arguments that match the oringial type, does the VM catch the problem?
Mark ZhouWon't the body of the underlying function include the necessary parametric type checks? So whatever function is bound when this is called should check the arguments are passed to it.
ACK - see previous reply.
Calling old closures is actually unsound. Beyond CFE-inserted static checks, you can leak in arbitrarily typed values.
hmmm interesting. This sounds like it can lead to some confusing behavior in JavaScript but hopefully that would trigger the user to restart the app.
Won't the body of the underlying function include the necessary parametric type checks? So whatever function is bound when this is called should check the arguments are passed to it.
No DDC doesn't insert type checks into the body like dart2js does.
When the expected argument types are statically known but the passed arguments are not statically known to be subtypes DDC will insert the necessary type checks at the call site.
When calling a function dynamically, DDC uses a helper method to lookup the function arity, parameter types and validate the passed arguments but that isn't used here.
Related details
- Mark Zhou
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mark Zhou added 3 comments![]( "Open in Gerrit")
if (_isBuildingSdk) {Is this change necessary? I believe this will force these types to be created in every `dart:` library instead of once for the `dart:_runtime` library.
Unfortunately yes. I added a bit that attaches types to static members at link-time rather than tearoff-time, which means these RTI types need to be "warmed up" sooner.
This is because the 'old' late static tagging was happening too late for dynamic tearoffs of static members to read the type (and another case that escapes my memory). Thoughts?
if (member.isStatic && _reifyTearoff(member) && !_isBuildingSdk) {Do static methods in the SDK get types attached some other way when they are torn off?
We now attach static methods' types during link phase (so ahead of time). So we can assume they're already on their respective member at tear-off time.
if (!_isBuildingSdk && p.isStatic) {- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Shahan (Gerrit)
Nicholas Shahan added 2 comments![]( "Open in Gerrit")
if (_isBuildingSdk) {Mark ZhouIs this change necessary? I believe this will force these types to be created in every `dart:` library instead of once for the `dart:_runtime` library.
Unfortunately yes. I added a bit that attaches types to static members at link-time rather than tearoff-time, which means these RTI types need to be "warmed up" sooner.
This is because the 'old' late static tagging was happening too late for dynamic tearoffs of static members to read the type (and another case that escapes my memory). Thoughts?
This feels counter to the goal of this CL. Tearoff evaluation is being moved to the latest possible point (at the time of call) but the evaluation of their types is being performed even earlier than it was previously.
Would it be possible to delay the evaluation of the type until the first use? I added a comment where I *think* this would go. I'd feel better about that since that means we wont need to spend the time eagerly evaluating the types of every static method in the program before we start running the app.
var existingRti = JS<Object?>('', '#[#][#]', context, property, rtiName);I think in the case of a static methods this is finding the eagerly evaluated Rti object but what if it could be a function that when called creates it now?
Related details
- Mark Zhou
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mark Zhou added 4 comments![]( "Open in Gerrit")
if (_isBuildingSdk) {Mark ZhouIs this change necessary? I believe this will force these types to be created in every `dart:` library instead of once for the `dart:_runtime` library.
Nicholas ShahanUnfortunately yes. I added a bit that attaches types to static members at link-time rather than tearoff-time, which means these RTI types need to be "warmed up" sooner.
This is because the 'old' late static tagging was happening too late for dynamic tearoffs of static members to read the type (and another case that escapes my memory). Thoughts?
This feels counter to the goal of this CL. Tearoff evaluation is being moved to the latest possible point (at the time of call) but the evaluation of their types is being performed even earlier than it was previously.
Would it be possible to delay the evaluation of the type until the first use? I added a comment where I *think* this would go. I'd feel better about that since that means we wont need to spend the time eagerly evaluating the types of every static method in the program before we start running the app.
Ack - added context in sister comment.
var tear = JS('', '(...args) => #[#](...args)', context, property);Nate BiggsI'm struggling to understand if it is safe to just pass the arguments here. Does this new representation introduce a type check somewhere to catch when the type of the tearoff has changed on a hot reload?
If you call this tearoff passing the arguments that match the oringial type, does the VM catch the problem?
Mark ZhouWon't the body of the underlying function include the necessary parametric type checks? So whatever function is bound when this is called should check the arguments are passed to it.
Nicholas ShahanACK - see previous reply.
Calling old closures is actually unsound. Beyond CFE-inserted static checks, you can leak in arbitrarily typed values.
hmmm interesting. This sounds like it can lead to some confusing behavior in JavaScript but hopefully that would trigger the user to restart the app.
Won't the body of the underlying function include the necessary parametric type checks? So whatever function is bound when this is called should check the arguments are passed to it.
No DDC doesn't insert type checks into the body like dart2js does.
When the expected argument types are statically known but the passed arguments are not statically known to be subtypes DDC will insert the necessary type checks at the call site.
When calling a function dynamically, DDC uses a helper method to lookup the function arity, parameter types and validate the passed arguments but that isn't used here.
Yep, so the "solution" here would be to treat all of these functions effectively as dynamic. I'll make a note to only do this for hot-reloaded functions to avoid the extreme slowdown.
var existingRti = JS<Object?>('', '#[#][#]', context, property, rtiName);I think in the case of a static methods this is finding the eagerly evaluated Rti object but what if it could be a function that when called creates it now?
The complication here is that the 'type' of the tearoff needs to be bound at tear-off time even for potentially dynamic tearoffs too. So we might have to pipe the RTI through all `dload` and equivalent calls. I can try to experiment with something here though.
var existingRti = JS<Object?>('', '#[#][#]', context, property, rtiName);I think in the case of a static methods this is finding the eagerly evaluated Rti object but what if it could be a function that when called creates it now?
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mark Zhou added 2 comments![]( "Open in Gerrit")
Mark ZhouIs this change necessary? I believe this will force these types to be created in every `dart:` library instead of once for the `dart:_runtime` library.
Nicholas ShahanUnfortunately yes. I added a bit that attaches types to static members at link-time rather than tearoff-time, which means these RTI types need to be "warmed up" sooner.
This is because the 'old' late static tagging was happening too late for dynamic tearoffs of static members to read the type (and another case that escapes my memory). Thoughts?
Mark ZhouThis feels counter to the goal of this CL. Tearoff evaluation is being moved to the latest possible point (at the time of call) but the evaluation of their types is being performed even earlier than it was previously.
Would it be possible to delay the evaluation of the type until the first use? I added a comment where I *think* this would go. I'd feel better about that since that means we wont need to spend the time eagerly evaluating the types of every static method in the program before we start running the app.
Ack - added context in sister comment.
Done
var existingRti = JS<Object?>('', '#[#][#]', context, property, rtiName);Mark ZhouI think in the case of a static methods this is finding the eagerly evaluated Rti object but what if it could be a function that when called creates it now?
The complication here is that the 'type' of the tearoff needs to be bound at tear-off time even for potentially dynamic tearoffs too. So we might have to pipe the RTI through all `dload` and equivalent calls. I can try to experiment with something here though.
- Nate Biggs
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Shahan (Gerrit)
Nicholas Shahan added 7 comments![]( "Open in Gerrit")
if (_isBuildingSdk) {Now with the delayed evaluation of the types, can we revert this so we only emit the prerequisite types once?
// We emit type rules and function signature rules due to bootstrappingdelete now that that part has been removed.
// TODO(#57049): We tag all static members because we don't know if they'venit: line length
if (member.isStatic && _reifyTearoff(member) && !_isBuildingSdk) {What is this guarding? What happens when we tearoff statics from libraries in the SDK.
// We only need to tag static functions that are torn off at
// compile-time. We attach these at link-time so late-resolved tearoffs
// have access to their types.Revise comment to match the new changes.
// TODO(#57049): We tag all static members because we don't know if they'venit: line length
if (!_isBuildingSdk && p.isStatic && _reifyTearoff(p)) {ditto
- Mark Zhou
- Nate Biggs
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mark Zhou added 7 comments![]( "Open in Gerrit")
Now with the delayed evaluation of the types, can we revert this so we only emit the prerequisite types once?
Done
// We emit type rules and function signature rules due to bootstrappingdelete now that that part has been removed.
Done
// TODO(#57049): We tag all static members because we don't know if they'veMark Zhounit: line length
Done
if (member.isStatic && _reifyTearoff(member) && !_isBuildingSdk) {What is this guarding? What happens when we tearoff statics from libraries in the SDK.
Nothing AFAIK. I'll clean this up.
// We only need to tag static functions that are torn off at
// compile-time. We attach these at link-time so late-resolved tearoffs
// have access to their types.Revise comment to match the new changes.
Done
// TODO(#57049): We tag all static members because we don't know if they'veMark Zhounit: line length
Done
if (!_isBuildingSdk && p.isStatic && _reifyTearoff(p)) {Mark Zhouditto
Done
Related details
- Nate Biggs
- Nicholas Shahan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Shahan (Gerrit)
Nicholas Shahan voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nicholas Shahan
| Code-Review | +1 |
2 comments
Thanks for all the revisions, I left one more comment but LGTM!
/// Tag a generic [closure] with a [type] and its [defaultTypeArgs] values,The names don't match the actual names of the arguments here.
Related details
- Mark Zhou
- Nate Biggs
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mark Zhou added 1 comment![]( "Open in Gerrit")
/// Tag a generic [closure] with a [type] and its [defaultTypeArgs] values,The names don't match the actual names of the arguments here.
- Nate Biggs
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mark Zhou voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Nate Biggs
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
42 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/rtti.dart
Insertions: 3, Deletions: 2.
The diff is too large to show. Please review the diff.
```
Change information
[ddc] Updating tearoffs to be evaluated on access.
Tearoffs are now represented as a closure that resolves an underlying bound context and property on access. `_boundMethod` and RTI getters must also be evaluated late.
Additionally, we now both canonicalize static methods and tag them with their types at class-declaration time (though lazily) - so that late resolved closures have access to their types.
Some tests have been updated to expect simpler errors. DDC traditionally emits slightly different errors that might aid in debugging.
- M pkg/dev_compiler/lib/src/kernel/compiler_new.dart
- M pkg/dev_compiler/lib/src/kernel/expression_compiler.dart
- M pkg/dev_compiler/test/expression_compiler/runtime_debugger_api_test.dart
- M sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/debugger.dart
- M sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart
- M sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/rtti.dart
- A tests/dartdevc/debugger/debugger_ddc_test_golden.txt
- M tests/dartdevc/debugger/debugger_test.dart
- M tests/dartdevc/no_such_method_errors_test.dart
- M tests/hot_reload/tear_off_add_arguments/main.0.dart
- M tests/hot_reload/tear_off_add_arguments/main.1.dart
- M tests/hot_reload/tear_off_add_arguments2/main.0.dart
- M tests/hot_reload/tear_off_add_arguments2/main.1.dart
- A tests/hot_reload/tear_off_add_arguments3/main.0.dart
- A tests/hot_reload/tear_off_add_arguments3/main.1.dart
- M tests/hot_reload/tear_off_instance_equality/main.0.dart
- M tests/hot_reload/tear_off_instance_equality/main.1.dart
Code-Review: +1 by Nicholas Shahan
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |