Add support for javascript incompatibility warnings.

Add support for javascript incompatibility warnings (work in progress).
For now, warnings are only issued when applicable for type tests, type casts,
and toString.
Fix newly reported lint errors.

R=srdjan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=36001

[srdjan, 2014-05-08 18:11:05]

https://codereview.chromium.org/260713008/diff/20001/runtime/lib/errors_patch...
File runtime/lib/errors_patch.dart (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/lib/errors_patch...
runtime/lib/errors_patch.dart:342: String toString() => _msg;
Maybe: "Javascript Compatibility Error: $_msg"; Then you could eliminate the
'javascript compatibility' text in many cases.

https://codereview.chromium.org/260713008/diff/20001/runtime/lib/object.cc
File runtime/lib/object.cc (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/lib/object.cc#ne...
runtime/lib/object.cc:118: instance.IsSmi() || instance.IsMint() ||
instance.IsBigint();
Why not instance.IsInteger()?

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:821: return error.raw();
Maybe add per_compile_timer.Stop() before return.

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/exceptions.cc...
runtime/vm/exceptions.cc:764: void Exceptions::JSWarning(const ICData& ic_data,
const char* format, ...) {
Try always passing ICData and do the necessary code in the caller.

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/exceptions.h
File runtime/vm/exceptions.h (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/exceptions.h#...
runtime/vm/exceptions.h:35: static bool MayIssueJSWarning(const ICData& ic_data,
bool check_issued);
Please add comment about the arguments.

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/exceptions.h#...
runtime/vm/exceptions.h:39: // is not looked up yet, e.g. when the warning is
issued from native code.
Add comment that it can produce errors if appropriate flag is set.

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:1418: if (!(node->IsLiteralNode() &&
(type.IsIntType() || type.IsDoubleType()))) {
type.IsNumber() ?

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:1434: if (instance.IsSmi() || instance.IsMint()
|| instance.IsBigint()) {
instance.IsInteger()

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.h (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.h:158: PRINTF_ATTRIBUTE(3, 4);
4 spaces indent.

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler.cc:836: !Exceptions::MayIssueJSWarning(ic_data,
false))) {
Please add a brief comment why this is necessary.

https://codereview.chromium.org/260713008/diff/20001/tests/standalone/javascr...
File tests/standalone/javascript_compatibility_errors_test.dart (right):

https://codereview.chromium.org/260713008/diff/20001/tests/standalone/javascr...
tests/standalone/javascript_compatibility_errors_test.dart:14: try { var y = 1
as double; } on CastError catch (e) { }  /// 03: compile-time error
Check that "hello" is int does not produce warnings

[regis, 2014-05-09 21:03:41]

Thanks!

PTAL

https://codereview.chromium.org/260713008/diff/20001/runtime/lib/errors_patch...
File runtime/lib/errors_patch.dart (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/lib/errors_patch...
runtime/lib/errors_patch.dart:342: String toString() => _msg;
On 2014/05/08 18:11:05, srdjan wrote:
> Maybe: "Javascript Compatibility Error: $_msg"; Then you could eliminate the
> 'javascript compatibility' text in many cases.

Done.

https://codereview.chromium.org/260713008/diff/20001/runtime/lib/object.cc
File runtime/lib/object.cc (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/lib/object.cc#ne...
runtime/lib/object.cc:118: instance.IsSmi() || instance.IsMint() ||
instance.IsBigint();
On 2014/05/08 18:11:05, srdjan wrote:
> Why not instance.IsInteger()? 

Done.

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:821: return error.raw();
On 2014/05/08 18:11:05, srdjan wrote:
> Maybe add per_compile_timer.Stop() before return.

As discussed, the timer will stop when leaving the scope as in the other return
above.

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/exceptions.cc...
runtime/vm/exceptions.cc:764: void Exceptions::JSWarning(const ICData& ic_data,
const char* format, ...) {
On 2014/05/08 18:11:05, srdjan wrote:
> Try always passing ICData and do the necessary code in the caller.

Done.

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/exceptions.h
File runtime/vm/exceptions.h (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/exceptions.h#...
runtime/vm/exceptions.h:35: static bool MayIssueJSWarning(const ICData& ic_data,
bool check_issued);
On 2014/05/08 18:11:05, srdjan wrote:
> Please add comment about the arguments.

Done.
I also removed the check_issue argument, which could make the code more error
prone.

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/exceptions.h#...
runtime/vm/exceptions.h:39: // is not looked up yet, e.g. when the warning is
issued from native code.
On 2014/05/08 18:11:05, srdjan wrote:
> Add comment that it can produce errors if appropriate flag is set.

Done.

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:1418: if (!(node->IsLiteralNode() &&
(type.IsIntType() || type.IsDoubleType()))) {
On 2014/05/08 18:11:05, srdjan wrote:
> type.IsNumber() ? 

IsNumberType() checks for exactly 'num' type. I do not think that literals can
be of 'num' type.

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:1434: if (instance.IsSmi() || instance.IsMint()
|| instance.IsBigint()) {
On 2014/05/08 18:11:05, srdjan wrote:
> instance.IsInteger()

Done.

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.h (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.h:158: PRINTF_ATTRIBUTE(3, 4);
On 2014/05/08 18:11:05, srdjan wrote:
> 4 spaces indent.

Done.

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/260713008/diff/20001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler.cc:836: !Exceptions::MayIssueJSWarning(ic_data,
false))) {
On 2014/05/08 18:11:05, srdjan wrote:
> Please add a brief comment why this is necessary.

Done.

https://codereview.chromium.org/260713008/diff/20001/tests/standalone/javascr...
File tests/standalone/javascript_compatibility_errors_test.dart (right):

https://codereview.chromium.org/260713008/diff/20001/tests/standalone/javascr...
tests/standalone/javascript_compatibility_errors_test.dart:14: try { var y = 1
as double; } on CastError catch (e) { }  /// 03: compile-time error
On 2014/05/08 18:11:05, srdjan wrote:
> Check that "hello" is int does not produce warnings

I added a whole new section verifying that no warnings are generated.

[srdjan, 2014-05-09 21:37:35]

LGTM

https://codereview.chromium.org/260713008/diff/40001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/260713008/diff/40001/runtime/vm/exceptions.cc...
runtime/vm/exceptions.cc:749: bool Exceptions::MayCheckForJSWarning(const
ICData& ic_data) {
This method does not seem to be related to Exceptions, could you move it to a
better place? Maybe in class ICData (object.{h, cc})

[regis, 2014-05-09 22:39:30]

Thanks!

https://codereview.chromium.org/260713008/diff/40001/runtime/vm/exceptions.cc
File runtime/vm/exceptions.cc (right):

https://codereview.chromium.org/260713008/diff/40001/runtime/vm/exceptions.cc...
runtime/vm/exceptions.cc:749: bool Exceptions::MayCheckForJSWarning(const
ICData& ic_data) {
On 2014/05/09 21:37:36, srdjan wrote:
> This method does not seem to be related to Exceptions, could you move it to a
> better place? Maybe in class ICData (object.{h, cc})

Done.

[regis, 2014-05-09 22:42:19]

Committed patchset #4 manually as r36001 (presubmit successful).

[Florian Schneider, 2014-05-12 09:25:45]

https://codereview.chromium.org/260713008/diff/50022/runtime/vm/symbols.h
File runtime/vm/symbols.h (right):

https://codereview.chromium.org/260713008/diff/50022/runtime/vm/symbols.h#new...
runtime/vm/symbols.h:257: V(JavascriptCompatibilityError,
"_JavascriptCompatibilityError")             \
DBC: If you want to save time, try appending the new symbol at the end of the
symbols list instead of inserting it here: you won't need to regenerate
issue14236_test.dart, and it shows how broken this test is.

[regis, 2014-05-12 15:37:04]

On 2014/05/12 09:25:45, Florian Schneider wrote:
> https://codereview.chromium.org/260713008/diff/50022/runtime/vm/symbols.h
> File runtime/vm/symbols.h (right):
> 
>
https://codereview.chromium.org/260713008/diff/50022/runtime/vm/symbols.h#new...
> runtime/vm/symbols.h:257: V(JavascriptCompatibilityError,
> "_JavascriptCompatibilityError")             \
> DBC: If you want to save time, try appending the new symbol at the end of the
> symbols list instead of inserting it here: you won't need to regenerate
> issue14236_test.dart, and it shows how broken this test is.

Thanks for the tip!