Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11973018:
Improve decoding of JS TypeError. (Closed)
|
Created:
7 years, 11 months ago by ahe Modified:
7 years, 5 months ago CC:
reviews_dartlang.org, bakster Visibility:
Public. |
DescriptionImprove decoding of JS TypeError.
R=kasperl@google.com, ngeoffray@google.com, sra@google.com
Committed: https://code.google.com/p/dart/source/detail?r=25262
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Rebased #Patch Set 3 : Nicified #Patch Set 4 : Add more patterns for possible optimizations #Patch Set 5 : Fix problems discovered during testing #
Total comments: 37
Patch Set 6 : Merged with r25254 #Patch Set 7 : Tested on all modern browsers #Patch Set 8 : Update status files #
Total comments: 8
Patch Set 9 : Address comments #
Messages
Total messages: 16 (0 generated)
I'd like you initial reaction to my changes to unwrapException in
js_helper.dart.
Before, V8 version 3.15.11.8:
NoSuchMethodError : method not found: '<unknown>'
Receiver: null
Arguments: []
NoSuchMethodError : method not found: '<unknown>'
Receiver: null
Arguments: []
NoSuchMethodError : method not found: '<unknown>'
Receiver: ""
Arguments: []
Before, jsshell:
NoSuchMethodError : method not found: '<unknown>'
Receiver: null
Arguments: []
NoSuchMethodError : method not found: '<unknown>'
Receiver: null
Arguments: []
NoSuchMethodError : method not found: '<unknown>'
Receiver: ""
Arguments: []
After, V8 version 3.15.11.8:
NullError: Cannot call "fisk$0" on null
NullError: Cannot call "fisk$0" on null
NoSuchMethodError: Cannot call "fisk$0" on "#<Object>" (Object #<Object> has no
method 'fisk$0')
After, jsshell:
NullError: t1.x_0 is null
NullError: t1.v_2 is undefined
NoSuchMethodError: Cannot call "fisk$0" (t1.z_1.fisk$0 is not a function)
Using this test program:
main() {
var x = null;
var z = new Object();
var v = new List(1)[0];
[].forEach((y) => x = z = v = y);
try {
x.fisk();
} catch (e) {
print(e);
}
try {
v.fisk();
} catch (e) {
print(e);
}
try {
z.fisk();
} catch (e) {
print(e);
}
}
https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (left):
https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:989: var name
= JS('var', r'#.arguments ? #.arguments[0] : ""', ex, ex);
These fields have been removed from V8 (and Chrome).
The current version of V8 in Chrome dev is 3.16.4. The one in the Dart repo is
3.13.7.5 which still has support for type and argument, it is gone in 3.15.11.8
which I just tested locally. There is no stable release of 3.16 yet.
I like the better error messages a lot! https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com... File dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right): https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:966: static final noSuchMethodPattern = I find that this method is really hard to read and grok. Maybe there's a way to improve that through formatting and helper methods or named constant strings?
PTAL
LGTM -- much, much easier to grok now. Is it possibly to test this separately or do we already have enough coverage in the system?
On 2013/01/29 12:57:20, kasperl wrote: > LGTM -- much, much easier to grok now. Is it possibly to test this separately or > do we already have enough coverage in the system? Now that you ask me to state my opinion, I got a new idea for testing this. I think I can create a test in dart2js_native which imports js_helper can thus test that the exceptions thrown have the correct implementation type. If that doesn't work, I can create a dart2js_test which tests the toString message.
On 2013/01/29 13:03:37, ahe wrote: > On 2013/01/29 12:57:20, kasperl wrote: > > LGTM -- much, much easier to grok now. Is it possibly to test this separately > or > > do we already have enough coverage in the system? > > Now that you ask me to state my opinion, I got a new idea for testing this. I > think I can create a test in dart2js_native which imports js_helper can thus > test that the exceptions thrown have the correct implementation type. If that > doesn't work, I can create a dart2js_test which tests the toString message. But first lunch :-)
PTAL. Turns out the world was slightly more complicated. See patch sets 4 and 5. I think I have added comments that should explain what the new stuff does. I also added a test.
lgtm https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com... File dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right): https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:966: static final noSuchMethodPattern = On 2013/01/21 13:54:09, kasperl wrote: > I find that this method is really hard to read and grok. Maybe there's a way to > improve that through formatting and helper methods or named constant strings? In addition to Kaspers suggestions, don't forget that documentation can also make this more comprehensible. It took me a long time to understand that this code forces an error with known components in order to generate a pattern for matching errors. A comment to that effect would have saved me 30 minutes in trying to understand what is going on here. https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:973: var function = JS('', r"""function($expr$) { Why are these names $name$? If they are not substituted, I'd rather they be 'normal'. I'm confused. See comment above. https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:979: // Subtle bug/feature in V8: it no longer calls toString. it? Can you be more specific about what no longer calls toString? V8 or a certian operation? https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:981: .replace(String({}), '$receiver$') Surely there are other receivers? Try with some DOM elements, e.g. obfuscate(window).frazzle(); where obfuscate() is a dynamic-returning function that is actually the identity function but too confusing to the compiler to be inlined or inferenced to return the input. https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:995: r"#.replace('\\$arguments\\$', '(.*)')" It will be slightly better if emitted JavaScript strings have double quotes. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... File dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right): https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:766: final String _pattern; Perhaps put _pattern first and add comments to fields to say that it is the index of the arguments within the _pattern's substring matches. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:873: // Since we want to create a new regular expression from an uknown uNknown https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:884: // Find the location of the matches. This will help us extract Perhaps: Find the positions within the substring matches of the error message components. https://codereview.chromium.org/11973018/diff/14001/dart/tests/compiler/dart2... File dart/tests/compiler/dart2js_native/type_error_decode_test.dart (right): https://codereview.chromium.org/11973018/diff/14001/dart/tests/compiler/dart2... dart/tests/compiler/dart2js_native/type_error_decode_test.dart:18: [].forEach((y) => f.field = nul = s = x = z = v = y); Comment here. I think that the VM and dart2js should provided a function called 'inscrutable' that is guaranteed to be opaque to analysis. Then var x = inscrutable(null); would do the job.
This is very nice! I'm confused about the string buffer -> string interpolation changes though. Why did you have to do this with this CL? https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... File dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right): https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:487: String selectorName = ''; Why this change? The string buffer has some state string interpolation can not have, so it seems a Dart VM would perform better with a string buffer. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:808: /// * arguments: The arguments as formatted by the JavaScript Please provide an example. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:809: /// engine. No browser are known to provide this information. So why do we have this field? https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:812: /// code). Firefox provides this information. Please provide an example. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:815: /// code). Firefox provides this information. ditto https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:825: var match = JS('', 'new RegExp(#).exec(#)', _pattern, message); Should that be JS("=List|Null", ...) ? https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:854: /// r"$receiver$". Please add a comment on how this method is different than the one above. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:871: message = JS('String', r"#.replace(String({}), '$receiver$')", message); Should you put '$receiver$' in a static const? https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:873: // Since we want to create a new regular expression from an uknown uknown -> unknown https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:913: /// The error is provoked so all know variable content can be know -> known https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:950: : this._method = JS('', '#.method', match); One line? https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1037: if (ieErrorCode == 438 && ieFacilityNumber == 10) { Please add a comment on why you still need this. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1676: String result = ''; Same comment for the String buffer. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... File dart/sdk/lib/_internal/compiler/implementation/lib/regexp_helper.dart (right): https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/regexp_helper.dart:36: String flags = ''; Same string buffer vs string interpolation comment.
https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com... File dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right): https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:966: static final noSuchMethodPattern = On 2013/01/29 20:54:16, sra1 wrote: > On 2013/01/21 13:54:09, kasperl wrote: > > I find that this method is really hard to read and grok. Maybe there's a way > to > > improve that through formatting and helper methods or named constant strings? > > In addition to Kaspers suggestions, don't forget that documentation can also > make this more comprehensible. > > It took me a long time to understand that this code forces an error with known > components in order to generate a pattern for matching errors. A comment to > that effect would have saved me 30 minutes in trying to understand what is going > on here. Stephen, I'm sorry you spent so much time reviewing this version. I guess I uploaded this version while you had an older version open. I hope you find the newer version easier to understand. https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:973: var function = JS('', r"""function($expr$) { On 2013/01/29 20:54:16, sra1 wrote: > Why are these names $name$? > If they are not substituted, I'd rather they be 'normal'. > > I'm confused. See comment above. I haven't documented why I use "$name$", not "name". I will do so. The reason is that I'm trying to inject something known into something unknown. The unknown entity is the error message that the browser provides with a TypeError. It is a human readable message, possibly localized in a language I don't understand. I'm guessing that $name$ would never naturally occur in a human readable error message, yet it is easy to decode. For example, if I evaluate this in V8 version 3.13.7.6: var $expr$ = null; $expr$.$method$() The VM throws an instance of TypeError whose message property contains "Cannot call method '$method$' of null". I'm reasonable sure that if the string contains $method$, that's where the method name will be in general. So I turn it into this regular expression: "Cannot call method '(.*)' of null" Similarly, if I evaluate: var $expr$ = {toString: function() { return '$receiver$'; }}; $expr$.$method$() I get this message: "Object $receiver$ has no method '$method$'" Which I turn into this regular expression: "Object (.*) has no method '(.*)'" Firefox/jsshell is slightly different, it tries to include the source code that caused the exception, so I get this message: "$expr$.$method$ is not a function" which I turn into this regular expression: "(.*)\\.(.*) is not a function" https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:979: // Subtle bug/feature in V8: it no longer calls toString. On 2013/01/29 20:54:16, sra1 wrote: > it? Can you be more specific about what no longer calls toString? V8 or a > certian operation? I looked at my new comment. I can still clarify it a bit. Later versions of V8 no longer calls toString on the receiver, so instead of this message: "Object $receiver$ has no method '$method$'" I get: "Object [object Object] has no method '$method$'" This is actually quite weird, because if I had not provided a toString method, it uses a different logic that gives a much better message. For example: var $expr$ = {}; $expr$.$method$() Gives: "Object #<Object> has no method '$method$'" And function Foo() {}; var $expr$ = new Foo(); $expr$.$method$() Gives: "Object #<Foo> has no method '$method$'" https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:981: .replace(String({}), '$receiver$') On 2013/01/29 20:54:16, sra1 wrote: > Surely there are other receivers? > Try with some DOM elements, e.g. obfuscate(window).frazzle(); > > where obfuscate() is a dynamic-returning function that is actually the identity > function but too confusing to the compiler to be inlined or inferenced to return > the input. I don't understand what you're saying. https://codereview.chromium.org/11973018/diff/2001/dart/sdk/lib/_internal/com... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:995: r"#.replace('\\$arguments\\$', '(.*)')" On 2013/01/29 20:54:16, sra1 wrote: > It will be slightly better if emitted JavaScript strings have double quotes. Why is that? https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... File dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right): https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:487: String selectorName = ''; On 2013/01/29 20:59:54, ngeoffray wrote: > Why this change? It's unrelated. I'll move it to a separate CL. > The string buffer has some state string interpolation can not > have, so it seems a Dart VM would perform better with a string buffer. When would the Dart VM ever execute this code? https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:766: final String _pattern; On 2013/01/29 20:54:17, sra1 wrote: > Perhaps put _pattern first and add comments to fields to say that it is the > index of the arguments within the _pattern's substring matches. Will do. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:808: /// * arguments: The arguments as formatted by the JavaScript On 2013/01/29 20:59:54, ngeoffray wrote: > Please provide an example. Will do. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:809: /// engine. No browser are known to provide this information. On 2013/01/29 20:59:54, ngeoffray wrote: > So why do we have this field? Because you never know what they will put in there. I'm trying to make sure that everything that can possibly vary has a know value. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:825: var match = JS('', 'new RegExp(#).exec(#)', _pattern, message); On 2013/01/29 20:59:54, ngeoffray wrote: > Should that be JS("=List|Null", ...) ? I'll try. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:854: /// r"$receiver$". On 2013/01/29 20:59:54, ngeoffray wrote: > Please add a comment on how this method is different than the one above. Will do. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:871: message = JS('String', r"#.replace(String({}), '$receiver$')", message); On 2013/01/29 20:59:54, ngeoffray wrote: > Should you put '$receiver$' in a static const? Not as it is right now. That constant ends up in the generated code. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:950: : this._method = JS('', '#.method', match); On 2013/01/29 20:59:54, ngeoffray wrote: > One line? I think I'm following the style guide on this. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1037: if (ieErrorCode == 438 && ieFacilityNumber == 10) { On 2013/01/29 20:59:54, ngeoffray wrote: > Please add a comment on why you still need this. Good point. I actually wanted to move that code up. An error code is much more reliable than pattern matching. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1676: String result = ''; On 2013/01/29 20:59:54, ngeoffray wrote: > Same comment for the String buffer. I'll create a new CL.
https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... File dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right): https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:487: String selectorName = ''; On 2013/01/29 22:06:46, ahe wrote: > On 2013/01/29 20:59:54, ngeoffray wrote: > > Why this change? > > It's unrelated. I'll move it to a separate CL. > > > The string buffer has some state string interpolation can not > > have, so it seems a Dart VM would perform better with a string buffer. > > When would the Dart VM ever execute this code? Good point. So you're assuming dart2js will change string interpolation into JS '+'?
Note to self: Make sure to test for https://code.google.com/p/dart/issues/detail?id=3240 as well.
PTAL. Tested on: Mac Chrome 30.0.1570.0 canary. Mac Chrome 29.0.1547.22 beta Mac Firefox 22.0 Mac Safari 6.0.5 (8536.30.1) Windows 7 Internet Explorer 9 Windows 7 Internet Explorer 10 https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... File dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right): https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:812: /// code). Firefox provides this information. On 2013/01/29 20:59:54, ngeoffray wrote: > Please provide an example. Done. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:815: /// code). Firefox provides this information. On 2013/01/29 20:59:54, ngeoffray wrote: > ditto Done. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:825: var match = JS('', 'new RegExp(#).exec(#)', _pattern, message); On 2013/01/29 22:06:46, ahe wrote: > On 2013/01/29 20:59:54, ngeoffray wrote: > > Should that be JS("=List|Null", ...) ? > > I'll try. Done. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:854: /// r"$receiver$". On 2013/01/29 22:06:46, ahe wrote: > On 2013/01/29 20:59:54, ngeoffray wrote: > > Please add a comment on how this method is different than the one above. > > Will do. Done. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:873: // Since we want to create a new regular expression from an uknown On 2013/01/29 20:59:54, ngeoffray wrote: > uknown -> unknown Done. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:884: // Find the location of the matches. This will help us extract On 2013/01/29 20:54:17, sra1 wrote: > Perhaps: Find the positions within the substring matches of the error message > components. Done. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:913: /// The error is provoked so all know variable content can be On 2013/01/29 20:59:54, ngeoffray wrote: > know -> known Done. https://codereview.chromium.org/11973018/diff/14001/dart/sdk/lib/_internal/co... dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1037: if (ieErrorCode == 438 && ieFacilityNumber == 10) { On 2013/01/29 22:06:46, ahe wrote: > On 2013/01/29 20:59:54, ngeoffray wrote: > > Please add a comment on why you still need this. > > Good point. I actually wanted to move that code up. An error code is much more > reliable than pattern matching. Done.
LGTM! https://codereview.chromium.org/11973018/diff/30001/dart/sdk/lib/_internal/li... File dart/sdk/lib/_internal/lib/js_helper.dart (right): https://codereview.chromium.org/11973018/diff/30001/dart/sdk/lib/_internal/li... dart/sdk/lib/_internal/lib/js_helper.dart:1005: // This function is carefully created to maximize the possibility The following comment applies to all provokeCallErrorOn* methods. Maybe move it out of this method, and rephrase with "The following functions". https://codereview.chromium.org/11973018/diff/30001/dart/sdk/lib/_internal/li... dart/sdk/lib/_internal/lib/js_helper.dart:1210: // Using JS to give type hints to the compiler to help tree-shaking. Type inference should now be able to know. https://codereview.chromium.org/11973018/diff/30001/dart/tests/compiler/dart2... File dart/tests/compiler/dart2js_native/native_null_closure_frog_test.dart (right): https://codereview.chromium.org/11973018/diff/30001/dart/tests/compiler/dart2... dart/tests/compiler/dart2js_native/native_null_closure_frog_test.dart:5: // Test that exception unwrapping handle cases like ({foo:null}).foo(). Does this test really help you knowing? There could be different reasons why you get a JsNoSuchMethodError. Don't you want to test the fields of the error? https://codereview.chromium.org/11973018/diff/30001/dart/tests/compiler/dart2... File dart/tests/compiler/dart2js_native/type_error_decode_test.dart (right): https://codereview.chromium.org/11973018/diff/30001/dart/tests/compiler/dart2... dart/tests/compiler/dart2js_native/type_error_decode_test.dart:44: [].forEach((y) => f.field = nul = s = x = z = v = y); What is this line for? Please add a comment.
Thank you, Nicolas. https://codereview.chromium.org/11973018/diff/30001/dart/sdk/lib/_internal/li... File dart/sdk/lib/_internal/lib/js_helper.dart (right): https://codereview.chromium.org/11973018/diff/30001/dart/sdk/lib/_internal/li... dart/sdk/lib/_internal/lib/js_helper.dart:1005: // This function is carefully created to maximize the possibility On 2013/07/22 09:08:56, ngeoffray wrote: > The following comment applies to all provokeCallErrorOn* methods. Maybe move it > out of this method, and rephrase with "The following functions". Done. https://codereview.chromium.org/11973018/diff/30001/dart/sdk/lib/_internal/li... dart/sdk/lib/_internal/lib/js_helper.dart:1210: // Using JS to give type hints to the compiler to help tree-shaking. On 2013/07/22 09:08:56, ngeoffray wrote: > Type inference should now be able to know. I'll try that in another CL. https://codereview.chromium.org/11973018/diff/30001/dart/tests/compiler/dart2... File dart/tests/compiler/dart2js_native/native_null_closure_frog_test.dart (right): https://codereview.chromium.org/11973018/diff/30001/dart/tests/compiler/dart2... dart/tests/compiler/dart2js_native/native_null_closure_frog_test.dart:5: // Test that exception unwrapping handle cases like ({foo:null}).foo(). On 2013/07/22 09:08:56, ngeoffray wrote: > Does this test really help you knowing? There could be different reasons why you > get a JsNoSuchMethodError. Don't you want to test the fields of the error? I just tried to fix an existing test. This is all being tested more throughly in other tests, in particular, type_error_decode_test.dart. As for testing the fields, JsNoSuchMethodError has no public fields. Due to differences between various browsers, there is no information we can reliably provide, especially after Chrome/V8 removed all the additional informational fields from TypeError. https://codereview.chromium.org/11973018/diff/30001/dart/tests/compiler/dart2... File dart/tests/compiler/dart2js_native/type_error_decode_test.dart (right): https://codereview.chromium.org/11973018/diff/30001/dart/tests/compiler/dart2... dart/tests/compiler/dart2js_native/type_error_decode_test.dart:44: [].forEach((y) => f.field = nul = s = x = z = v = y); On 2013/07/22 09:08:56, ngeoffray wrote: > What is this line for? Please add a comment. Done.
Message was sent while issue was closed.
Committed patchset #9 manually as r25262 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
