Add optional message to assert in Dart2js.

No flag guarding the feature.

[Lasse Reichstein Nielsen, 2015-09-01 13:09:17]

lrn@google.com changed reviewers:
+ johnniwinther@google.com

[Lasse Reichstein Nielsen, 2015-09-01 13:09:18]

FYI, WDYT?

[Johnni Winther, 2015-09-01 13:32:05]

Looks good.

https://codereview.chromium.org/1325843003/diff/1/pkg/compiler/lib/src/inferr...
File pkg/compiler/lib/src/inferrer/inferrer_visitor.dart (right):

https://codereview.chromium.org/1325843003/diff/1/pkg/compiler/lib/src/inferr...
pkg/compiler/lib/src/inferrer/inferrer_visitor.dart:785: /// Handle an enabled
assertion of [expression].
Remove as well.

https://codereview.chromium.org/1325843003/diff/1/pkg/compiler/lib/src/resolu...
File pkg/compiler/lib/src/resolution/registry.dart (right):

https://codereview.chromium.org/1325843003/diff/1/pkg/compiler/lib/src/resolu...
pkg/compiler/lib/src/resolution/registry.dart:60:
//mapping.registerAssert();//TODO: SOmething here?
Add a TODO for me to resolve this.

[Lasse Reichstein Nielsen, 2015-09-03 11:23:25]

https://chromiumcodereview.appspot.com/1325843003/diff/1/pkg/compiler/lib/src...
File pkg/compiler/lib/src/inferrer/inferrer_visitor.dart (right):

https://chromiumcodereview.appspot.com/1325843003/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/inferrer/inferrer_visitor.dart:785: /// Handle an enabled
assertion of [expression].
On 2015/09/01 13:32:05, Johnni Winther wrote:
> Remove as well.

Done.

https://chromiumcodereview.appspot.com/1325843003/diff/1/pkg/compiler/lib/src...
File pkg/compiler/lib/src/resolution/registry.dart (right):

https://chromiumcodereview.appspot.com/1325843003/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/resolution/registry.dart:60:
//mapping.registerAssert();//TODO: SOmething here?
On 2015/09/01 13:32:05, Johnni Winther wrote:
> Add a TODO for me to resolve this.

Done.

[Siggi Cherem (dart-lang), 2015-09-03 16:13:24]

sigmund@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2015-09-03 16:13:24]

DBC

Thanks for putting this together Lasse, I like the change to make assert an
actual node.

https://codereview.chromium.org/1325843003/diff/40001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1325843003/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:2611: handleIf(node,
What does the final output endup looking like?

In particular, I wonder if it's worth negating the condition here. I'm not sure
if we will emit this as:

  if (!assertConditionHelper(foo)) throw new AssertionError(message);

or as:

  if (assertConditionHelper(foo)) {
  } else throw new AssertionError(message);


If we are generating the latter, I think it might be worth adding an HNot node
in the condition, or change assertConditionHelpermethod to check for `true !=
condition` and then move the failure to the `then` branch.

[Siggi Cherem (dart-lang), 2015-09-03 16:14:28]

(but otherwise lgtm!)

[sra1, 2015-09-03 18:31:17]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2015-09-03 18:31:18]

I don't understand is how type inference works now there is a new ast node.
Please add a type inference unit test.

----
import 'dart:math';

foo(x, [y]) => y;

main() {
  assert(foo("Hi", true), foo(true));
  foo(1);
}
----

with enableUserAssertions: true
  foo is called with (int+String+bool, bool+null)

with enableUserAssertions: false
  foo is called with (int, null)

Also test that the assert helper argument is type-inferred to [empty] (no
asserts) or  bool+null (asserts enabled).

We have lots of problems if the helpers are not correctly inferred.

https://codereview.chromium.org/1325843003/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1325843003/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:2111: assertThrowMethod =
findHelper('assertThrowHelper');
I would expect that you need to add declarations for these in the mock libraries
used in the dart2js unit tests.

[Lasse Reichstein Nielsen, 2015-09-09 12:02:27]

Addressed comments and added type_inference7_test.

The types for the example above are correct in checked mode, but they are
exactly the same in production mode. I need some way to stop the inference from
seeing the content of an assert in production mode, but it's not clear to me
where that cut-off needs to be. Suggestions welcome.

[Lasse Reichstein Nielsen, 2015-09-09 12:02:46]

https://codereview.chromium.org/1325843003/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1325843003/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:2111: assertThrowMethod =
findHelper('assertThrowHelper');
Done.

https://codereview.chromium.org/1325843003/diff/40001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder.dart (right):

https://codereview.chromium.org/1325843003/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder.dart:2611: handleIf(node,
Ack, it's the latter, only uglier: if (....helper(...)); else ..helper(...);

I'll fix that. The simplest (and shortest) is to push the negation into the
helper function.

[sra1, 2015-09-09 18:04:01]

On 2015/09/09 12:02:27, Lasse Reichstein Nielsen wrote:
> Addressed comments and added type_inference7_test.
> 
> The types for the example above are correct in checked mode, but they are
> exactly the same in production mode. I need some way to stop the inference
from
> seeing the content of an assert in production mode, but it's not clear to me
> where that cut-off needs to be. Suggestions welcome.

Did you forget to add types/types.dart ?
When I run the test I get:

Unhandled exception:
Uncaught Error: Class 'TypesTask' has no instance getter 'emptyType'.

NoSuchMethodError: method not found: 'emptyType'
Receiver: Instance of 'TypesTask'

[sra1, 2015-09-10 01:56:26]

This CL adds +1.3% (11k) to swarm compiled with --checked.

If we make the names shorter  (assertConditionHelper -->  asssertCheck,
assertThrowHelper --> assertThrow) it is better but still 1% larger.
(I think unminified size is a reasonable comparison for debugging builds.)

          H.assertHelper(milliseconds > 0);
-->
          if (H.assertConditionHelper(milliseconds > 0))
            H.assertThrowHelper(null);

-->
          if (H.assertCheck(milliseconds > 0))
            H.assertThrow(null);


With a little work I think most conditions could be optimized (recognized as
returning only true or false) and replace H.assertCheck(e) with !e.

-->
          if (!(milliseconds > 0))
            H.assertThrow(null);

This would get the size down to +0.6%

Maybe we should keep assertHelper and use the new pattern only for asserts that
have a message expression.

https://codereview.chromium.org/1325843003/diff/80001/tests/compiler/dart2js/...
File tests/compiler/dart2js/type_inference7_test.dart (right):

https://codereview.chromium.org/1325843003/diff/80001/tests/compiler/dart2js/...
tests/compiler/dart2js/type_inference7_test.dart:50:
Expect.equals(typesTask.boolType, simplify(mask2.nonNullable(), compiler));
Also need to test the parameters of assertXXXHelper.

[sra1, 2015-09-15 23:52:55]

I'm taking ownership of this CL.

The new CL is at https://codereview.chromium.org/1342213003/
I hope to commit very soon so we can fix spurious inclusion from asserts -
https://github.com/dart-lang/sdk/issues/24355

https://chromiumcodereview.appspot.com/1325843003/diff/100001/sdk/lib/_intern...
File sdk/lib/_internal/js_runtime/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/1325843003/diff/100001/sdk/lib/_intern...
sdk/lib/_internal/js_runtime/lib/js_helper.dart:3329: if (assertTest(condition))
throw new AssertError();
AssertionError()