Start implementing checked mode and tools support for using it.

Start implementing checked mode and tools support for using it.
Committed: https://code.google.com/p/dart/source/detail?r=7375

[floitsch, 2012-05-07 09:50:07]

First round of comments.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:826: String name =
property.substring(3, property.length);
explain what is cut off.
maybe assert that the property starts with the expected prefix.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:844:
listSuperNativeTypeCheck(value, property) {
Explain what this does.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:852: propertyTypeCheck(value,
property) {
ditto.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:859: callTypeCheck(value,
property) {
ditto.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1045: if
(original.guaranteedType.union(convertedType) === convertedType) {
==

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:441: if
(!left.propagatedType.canBeNull()) {
I would prefer switching to the new semantics in a separate CL.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:536: &&
!expressionType.canBeNull()
you could move that check into the 'true' section.
if the is-check would be false, then 'null' can't change that.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/ssa/types.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/types.dart:8: factory HType.fromBoundedType(Type
type,
/***/ doc explaining what boundedType means.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/types.dart:43: return null;
Please don't return 'null' from a factory.
I would prefer having a special Type node.

[kasperl, 2012-05-07 10:42:39]

Quick comments:

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/lib/mock.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/mock.dart:11: String msg;
final?

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/mock.dart:12: TypeError(this.msg);
const?

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1040: if (type !== null) {
Bailout style? 

   if (type === null) return original;
   ...

[floitsch, 2012-05-07 11:06:56]

LGTM after all the comments have been addressed.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:847: if (JS('bool', '#.#()',
value, property)) return value;
#.# won't work.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:862: && JS('bool', '#.#()',
value, property)) {
#.# won't work.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1902: if (node.checked) {
not sure I like that all of this is in the codegen.

[ahe, 2012-05-07 11:59:57]

DBC

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/apiimpl.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/apiimpl.dart:24: bool checkedMode = false;
Developer mode, please.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/apiimpl.dart:28: checkedMode =
options.indexOf('--enable-checked-mode') !== -1;
I don't like this flag name. I think it should be something like:

--developer-mode

I'm fine having aliases for compatibility with the VM. But I think we need to
*think* about flag names, instead of just doing something. This is the most
fundamental interface to the compiler, and it should be "designed".

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/apiimpl.dart:102: bool get isCheckedModeEnabled() {
Developer mode, please.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:826: String name =
property.substring(3, property.length);
On 2012/05/07 09:50:07, floitsch wrote:
> explain what is cut off.
> maybe assert that the property starts with the expected prefix.

Be careful about asserting anything in this code. The assertion will affect the
user's code.

[ngeoffray, 2012-05-07 13:15:42]

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/apiimpl.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/apiimpl.dart:24: bool checkedMode = false;
On 2012/05/07 11:59:57, ahe wrote:
> Developer mode, please.

Renamed to enableTypeAssertions.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/apiimpl.dart:28: checkedMode =
options.indexOf('--enable-checked-mode') !== -1;
On 2012/05/07 11:59:57, ahe wrote:
> I don't like this flag name. I think it should be something like:
> 
> --developer-mode
> 
> I'm fine having aliases for compatibility with the VM. But I think we need to
> *think* about flag names, instead of just doing something. This is the most
> fundamental interface to the compiler, and it should be "designed".

As discussed, keeping enable-checked-mode only for now.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/apiimpl.dart:102: bool get isCheckedModeEnabled() {
On 2012/05/07 11:59:57, ahe wrote:
> Developer mode, please.

I removed the getter.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:826: String name =
property.substring(3, property.length);
On 2012/05/07 11:59:57, ahe wrote:
> On 2012/05/07 09:50:07, floitsch wrote:
> > explain what is cut off.
> > maybe assert that the property starts with the expected prefix.
> 
> Be careful about asserting anything in this code. The assertion will affect
the
> user's code.

Very good point.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:844:
listSuperNativeTypeCheck(value, property) {
On 2012/05/07 09:50:07, floitsch wrote:
> Explain what this does.

Done.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:847: if (JS('bool', '#.#()',
value, property)) return value;
On 2012/05/07 11:06:56, floitsch wrote:
> #.# won't work.

Done.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:852: propertyTypeCheck(value,
property) {
On 2012/05/07 09:50:07, floitsch wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:859: callTypeCheck(value,
property) {
On 2012/05/07 09:50:07, floitsch wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/js_helper.dart:862: && JS('bool', '#.#()',
value, property)) {
On 2012/05/07 11:06:56, floitsch wrote:
> #.# won't work.

Done.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/lib/mock.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/mock.dart:11: String msg;
On 2012/05/07 10:42:39, kasperl wrote:
> final?

Done.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/mock.dart:12: TypeError(this.msg);
On 2012/05/07 10:42:39, kasperl wrote:
> const?

Done.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1040: if (type !== null) {
On 2012/05/07 10:42:39, kasperl wrote:
> Bailout style? 
> 
>    if (type === null) return original;
>    ...

Done.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:1045: if
(original.guaranteedType.union(convertedType) === convertedType) {
On 2012/05/07 09:50:07, floitsch wrote:
> ==

Done.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/codegen.dart:1902: if (node.checked) {
On 2012/05/07 11:06:56, floitsch wrote:
> not sure I like that all of this is in the codegen.

As discussed, I will live it here, but we can revisit once the HType API
stabilizes.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:441: if
(!left.propagatedType.canBeNull()) {
On 2012/05/07 09:50:07, floitsch wrote:
> I would prefer switching to the new semantics in a separate CL.

This should have been if (leg.propagatedType.isPrimitive()) ...

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:536: &&
!expressionType.canBeNull()
On 2012/05/07 09:50:07, floitsch wrote:
> you could move that check into the 'true' section.
> if the is-check would be false, then 'null' can't change that.

Done.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/ssa/types.dart (right):

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/types.dart:8: factory HType.fromBoundedType(Type
type,
On 2012/05/07 09:50:07, floitsch wrote:
> /***/ doc explaining what boundedType means.

Done.

https://chromiumcodereview.appspot.com/10353014/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/types.dart:43: return null;
On 2012/05/07 09:50:07, floitsch wrote:
> Please don't return 'null' from a factory.
> I would prefer having a special Type node.

I plan on changing that. I added a TODO here and where I do a null check.