Only generate assert in checked mode.

Only generate assert in checked mode.
Committed: https://code.google.com/p/dart/source/detail?r=8430

[kasperl, 2012-06-08 05:34:05]

LGTM.

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

https://chromiumcodereview.appspot.com/10540052/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/mock.dart:31: void assert(condition) {
I think this needs a bit more work. Something like this:

   void assert(object) {
     bool condition = (object is Function) 
         ? object() 
         : object;
     // TODO(kasperl): What should we do if condition is
     // null. Doesn't seem to be handled by the spec.
     if (!condition) throw new AssertionError();
   }

Excerpt from the spec
---------------------
The conditional expression e is evaluated to an object o. If the class of o is
a subtype of Function then let r be the result of invoking o with no arguments.
Otherwise, let r be o. It is a dynamic type error if o is not of type bool or
of type Function, or if r is not of type bool. If r is false, we say that the
assertion failed. If r is true, we say that the assertion succeeded. If the
assertion
succeeded, execution of the assert statement is complete. If the assertion
failed,
an AssertionError is thrown.

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

https://chromiumcodereview.appspot.com/10540052/diff/5001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1055: if (target ==
compiler.assertMethod && !node.isCall) {
===

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

https://chromiumcodereview.appspot.com/10540052/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2375: if (element ==
compiler.assertMethod && !compiler.enableUserAssertions) {
===?

[ahe, 2012-06-08 06:56:16]

LGTM, if you address Kasper's comments and change the implementation of
"isCall".

https://chromiumcodereview.appspot.com/10540052/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/10540052/diff/5001/lib/compiler/implem...
lib/compiler/implementation/tree/nodes.dart:274: Token end = getEndToken();
I don't like calling getEndToken to determine this. Isn't this essentially
"!isPropertyAccess"?

[karlklose, 2012-06-08 07:42:42]

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

https://chromiumcodereview.appspot.com/10540052/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2376:
stack.add(graph.addConstantNull());
I guess we add null here because we do not check that assert(...) is an
statement. However, as an expression, a call to something with the name assert
can never resolve to assertMethod. I think have to resolve assert(...)
differently when it is not in a statement expression context.

[ahe, 2012-06-08 07:59:14]

Still LGTM

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

https://chromiumcodereview.appspot.com/10540052/diff/5001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1057: if (!inInstanceContext)
error(node, MessageKind.CANNOT_RESOLVE, [node]);
I'm not sure about this error message.

I think it is more likely that the user is using assert incorrectly, so saying
something like "missing arguments to assert".

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

https://chromiumcodereview.appspot.com/10540052/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2376:
stack.add(graph.addConstantNull());
On 2012/06/08 07:42:42, karlklose wrote:
> I guess we add null here because we do not check that assert(...) is an
> statement. However, as an expression, a call to something with the name assert
> can never resolve to assertMethod. I think have to resolve assert(...)
> differently when it is not in a statement expression context.

I don't think Nicolas needs to worry about this now, we are a few people that
plan to look at negative tests, and it would be fine if Nicolas defers this
issue to us.

[ngeoffray, 2012-06-08 09:40:58]

Thanks all!

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

https://chromiumcodereview.appspot.com/10540052/diff/5001/lib/compiler/implem...
lib/compiler/implementation/lib/mock.dart:31: void assert(condition) {
On 2012/06/08 05:34:05, kasperl wrote:
> I think this needs a bit more work. Something like this:
> 
>    void assert(object) {
>      bool condition = (object is Function) 
>          ? object() 
>          : object;
>      // TODO(kasperl): What should we do if condition is
>      // null. Doesn't seem to be handled by the spec.
>      if (!condition) throw new AssertionError();
>    }
> 
> Excerpt from the spec
> ---------------------
> The conditional expression e is evaluated to an object o. If the class of o is
> a subtype of Function then let r be the result of invoking o with no
arguments.
> Otherwise, let r be o. It is a dynamic type error if o is not of type bool or
> of type Function, or if r is not of type bool. If r is false, we say that the
> assertion failed. If r is true, we say that the assertion succeeded. If the
> assertion
> succeeded, execution of the assert statement is complete. If the assertion
> failed,
> an AssertionError is thrown.

Done.

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

https://chromiumcodereview.appspot.com/10540052/diff/5001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1055: if (target ==
compiler.assertMethod && !node.isCall) {
On 2012/06/08 05:34:05, kasperl wrote:
> ===

Done.

https://chromiumcodereview.appspot.com/10540052/diff/5001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1057: if (!inInstanceContext)
error(node, MessageKind.CANNOT_RESOLVE, [node]);
On 2012/06/08 07:59:14, ahe wrote:
> I'm not sure about this error message.
> 
> I think it is more likely that the user is using assert incorrectly, so saying
> something like "missing arguments to assert".

Done.

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

https://chromiumcodereview.appspot.com/10540052/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2375: if (element ==
compiler.assertMethod && !compiler.enableUserAssertions) {
On 2012/06/08 05:34:05, kasperl wrote:
> ===?

Done.

https://chromiumcodereview.appspot.com/10540052/diff/5001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:2376:
stack.add(graph.addConstantNull());
On 2012/06/08 07:59:14, ahe wrote:
> On 2012/06/08 07:42:42, karlklose wrote:
> > I guess we add null here because we do not check that assert(...) is an
> > statement. However, as an expression, a call to something with the name
assert
> > can never resolve to assertMethod. I think have to resolve assert(...)
> > differently when it is not in a statement expression context.
> 
> I don't think Nicolas needs to worry about this now, we are a few people that
> plan to look at negative tests, and it would be fine if Nicolas defers this
> issue to us.

OK. Note that I think we will keep the the addConstantNull to not complicate the
builder already. The resolver has done all the work, we know the builder will
just remove this constantNull later on.

https://chromiumcodereview.appspot.com/10540052/diff/5001/lib/compiler/implem...
File lib/compiler/implementation/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/10540052/diff/5001/lib/compiler/implem...
lib/compiler/implementation/tree/nodes.dart:274: Token end = getEndToken();
On 2012/06/08 06:56:16, ahe wrote:
> I don't like calling getEndToken to determine this. Isn't this essentially
> "!isPropertyAccess"?

As discussed, changed the code to !isPropertyAccess && !isOperator

[Lasse Reichstein Nielsen, 2012-06-08 10:39:10]

dbc

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

https://chromiumcodereview.appspot.com/10540052/diff/6003/lib/compiler/implem...
lib/compiler/implementation/lib/mock.dart:31: void assert(condition) {
This defines a top-level,i.e., scoped assert function.
If a class defines an assert method, the top-level method will shadow it, which
it shouldn't.

https://chromiumcodereview.appspot.com/10540052/diff/6003/lib/compiler/implem...
lib/compiler/implementation/lib/mock.dart:32: if (condition is Function)
condition = condition();
It's a "dynamic error" if condition isn't a bool here.
Since the exact behavior of dynamic errors aren't specified, I guess it's ok to
throw an AssertionError, but we should probably document the decission here.

[ngeoffray, 2012-06-08 10:50:36]

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

https://chromiumcodereview.appspot.com/10540052/diff/6003/lib/compiler/implem...
lib/compiler/implementation/lib/mock.dart:31: void assert(condition) {
On 2012/06/08 10:39:10, Lasse Reichstein Nielsen wrote:
> This defines a top-level,i.e., scoped assert function.
> If a class defines an assert method, the top-level method will shadow it,
which
> it shouldn't.

I will let the resolver guys fix it :)

https://chromiumcodereview.appspot.com/10540052/diff/6003/lib/compiler/implem...
lib/compiler/implementation/lib/mock.dart:32: if (condition is Function)
condition = condition();
On 2012/06/08 10:39:10, Lasse Reichstein Nielsen wrote:
> It's a "dynamic error" if condition isn't a bool here.
> Since the exact behavior of dynamic errors aren't specified, I guess it's ok
to
> throw an AssertionError, but we should probably document the decission here.

It's a "dynamic type error" and a dynamic type error looks pretty well specified
in the last spec. So I'm not sure what do you want to change here?