Update assert to follow the spec.

Update assert to follow the spec.
Committed: https://code.google.com/p/dart/source/detail?r=446

[John Lenz, 2011-10-13 20:09:40]

http://codereview.chromium.org/8276014/diff/3001/tests/language/src/Assertion...
File tests/language/src/AssertionTest.dart (right):

http://codereview.chromium.org/8276014/diff/3001/tests/language/src/Assertion...
tests/language/src/AssertionTest.dart:8: class AssertionTest {
This is a fork of AssertTest for three change:
- use the "enable_type_checks" flag (as per spec)
- check for AssertionError instead of AssertError (as per spec)
- removes checks for un-spec'd fields on AssertionError.

[John Lenz, 2011-10-13 20:48:58]

ahe for the test file change
jat for everything else

[jat, 2011-10-13 21:03:00]

LGTM

http://codereview.chromium.org/8276014/diff/3001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (left):

http://codereview.chromium.org/8276014/diff/3001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:636:
checkAssignable(conditionNode, boolType, condition);
I know this is pre-existing code, but it seems like this isn't going to properly
handle something like:

var x = () => true;
assert(x);

I don't think we can rely on statically knowing the argument is a function for
this type check.  If you don't want to address it as part of this, please file
an issue to investigate/fix it later.

http://codereview.chromium.org/8276014/diff/3001/tests/language/src/Assertion...
File tests/language/src/AssertionTest.dart (right):

http://codereview.chromium.org/8276014/diff/3001/tests/language/src/Assertion...
tests/language/src/AssertionTest.dart:8: class AssertionTest {
On 2011/10/13 20:09:40, johnlenz wrote:
> This is a fork of AssertTest for three change:
> - use the "enable_type_checks" flag (as per spec)
> - check for AssertionError instead of AssertError (as per spec)
> - removes checks for un-spec'd fields on AssertionError.

Should the other test remain then if it doesn't correspond to the spec?

[ahe, 2011-10-13 21:19:41]

http://codereview.chromium.org/8276014/diff/3001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (left):

http://codereview.chromium.org/8276014/diff/3001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:636:
checkAssignable(conditionNode, boolType, condition);
On 2011/10/13 21:03:01, jat wrote:
> I know this is pre-existing code, but it seems like this isn't going to
properly
> handle something like:
> 
> var x = () => true;
> assert(x);
> 
> I don't think we can rely on statically knowing the argument is a function for
> this type check.  If you don't want to address it as part of this, please file
> an issue to investigate/fix it later.

What is it you think is wrong here? This is the *static* type analyzer.

I tried your example: http://try.dartlang.org/s/TjUa and there are no type
errors. There is an unexpected NullPointerException, so perhaps John should add
this test case.

http://codereview.chromium.org/8276014/diff/3001/tests/language/src/Assertion...
File tests/language/src/AssertionTest.dart (right):

http://codereview.chromium.org/8276014/diff/3001/tests/language/src/Assertion...
tests/language/src/AssertionTest.dart:8: class AssertionTest {
On 2011/10/13 20:09:40, johnlenz wrote:
> This is a fork of AssertTest for three change:
> - use the "enable_type_checks" flag (as per spec)
> - check for AssertionError instead of AssertError (as per spec)
> - removes checks for un-spec'd fields on AssertionError.

If this is a fork, why is there no version history to indicate this?

[jat, 2011-10-13 21:21:52]

http://codereview.chromium.org/8276014/diff/3001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (left):

http://codereview.chromium.org/8276014/diff/3001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:636:
checkAssignable(conditionNode, boolType, condition);
Ok, so if the type isn't known, checkAssignable will just ignore it?  If so,
that's fine.

[John Lenz, 2011-10-13 21:35:43]

http://codereview.chromium.org/8276014/diff/3001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (left):

http://codereview.chromium.org/8276014/diff/3001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:636:
checkAssignable(conditionNode, boolType, condition);
On 2011/10/13 21:03:01, jat wrote:
> I know this is pre-existing code, but it seems like this isn't going to
properly
> handle something like:
> 
> var x = () => true;
> assert(x);
> 
> I don't think we can rely on statically knowing the argument is a function for
> this type check.  If you don't want to address it as part of this, please file
> an issue to investigate/fix it later.

I think that should be fine.  x is dynamic and therefore assignable to bool.
Added a testcase.

http://codereview.chromium.org/8276014/diff/3001/tests/language/src/Assertion...
File tests/language/src/AssertionTest.dart (right):

http://codereview.chromium.org/8276014/diff/3001/tests/language/src/Assertion...
tests/language/src/AssertionTest.dart:8: class AssertionTest {
On 2011/10/13 21:03:01, jat wrote:
> On 2011/10/13 20:09:40, johnlenz wrote:
> > This is a fork of AssertTest for three change:
> > - use the "enable_type_checks" flag (as per spec)
> > - check for AssertionError instead of AssertError (as per spec)
> > - removes checks for un-spec'd fields on AssertionError.
> 
> Should the other test remain then if it doesn't correspond to the spec?

That is what I want Peter to tell me.  Is the original test wrong or is the
spec?

http://codereview.chromium.org/8276014/diff/3001/tests/language/src/Assertion...
tests/language/src/AssertionTest.dart:8: class AssertionTest {
On 2011/10/13 21:19:41, ahe wrote:
> On 2011/10/13 20:09:40, johnlenz wrote:
> > This is a fork of AssertTest for three change:
> > - use the "enable_type_checks" flag (as per spec)
> > - check for AssertionError instead of AssertError (as per spec)
> > - removes checks for un-spec'd fields on AssertionError.
> 
> If this is a fork, why is there no version history to indicate this?

I will correct this (svn cp are a pain to patch). I'm more intertested in the
basic question:
What is the correct exception to throw?
What is the correct flag to pass?
What fields are required on AssertError/AssertionError (not that I'll be adding
them in this change).

[John Lenz, 2011-10-13 21:51:08]

http://codereview.chromium.org/8276014/diff/3001/tests/language/src/Assertion...
File tests/language/src/AssertionTest.dart (right):

http://codereview.chromium.org/8276014/diff/3001/tests/language/src/Assertion...
tests/language/src/AssertionTest.dart:8: class AssertionTest {
On 2011/10/13 21:35:43, John Lenz wrote:
> On 2011/10/13 21:19:41, ahe wrote:
> > On 2011/10/13 20:09:40, johnlenz wrote:
> > > This is a fork of AssertTest for three change:
> > > - use the "enable_type_checks" flag (as per spec)
> > > - check for AssertionError instead of AssertError (as per spec)
> > > - removes checks for un-spec'd fields on AssertionError.
> > 
> > If this is a fork, why is there no version history to indicate this?
> 
> I will correct this (svn cp are a pain to patch). I'm more intertested in the
> basic question:
> What is the correct exception to throw?
> What is the correct flag to pass?
> What fields are required on AssertError/AssertionError (not that I'll be
adding
> them in this change).

In case you were waiting for the "svn cp" to be done. Done.

[ahe, 2011-10-14 10:45:40]

LGTM

http://codereview.chromium.org/8276014/diff/3001/tests/language/src/Assertion...
File tests/language/src/AssertionTest.dart (right):

http://codereview.chromium.org/8276014/diff/3001/tests/language/src/Assertion...
tests/language/src/AssertionTest.dart:8: class AssertionTest {
On 2011/10/13 21:35:43, John Lenz wrote:
> On 2011/10/13 21:19:41, ahe wrote:
> > On 2011/10/13 20:09:40, johnlenz wrote:
> > > This is a fork of AssertTest for three change:
> > > - use the "enable_type_checks" flag (as per spec)
> > > - check for AssertionError instead of AssertError (as per spec)
> > > - removes checks for un-spec'd fields on AssertionError.
> > 
> > If this is a fork, why is there no version history to indicate this?
> 
> I will correct this (svn cp are a pain to patch). I'm more intertested in the
> basic question:
> What is the correct exception to throw?

AssertionError according to the specification.

> What is the correct flag to pass?

I personally think it is bad form to use VM options in this case. I much more
prefer the multi-tests that are used in, for example, ClassLiteralTest.dart.

> What fields are required on AssertError/AssertionError (not that I'll be
adding
> them in this change).

That is a library issue. According to Nicolas, it was a mistake to move
AssertTest.dart from the runtime to language.

http://codereview.chromium.org/8276014/diff/3003/tests/language/src/Assertion...
File tests/language/src/AssertionTest.dart (right):

http://codereview.chromium.org/8276014/diff/3003/tests/language/src/Assertion...
tests/language/src/AssertionTest.dart:4: // VMOptions=--enable_type_checks
I don't think this option belongs in language tests. This is a VM specific
option which is understood by the dartc VM "simulator", DartRunner.

Furthermore, the assert statement should also be tested in production mode.

http://codereview.chromium.org/8276014/diff/3003/tests/language/src/Assertion...
tests/language/src/AssertionTest.dart:29: static testClosure2() {
This method isn't called.

http://codereview.chromium.org/8276014/diff/3003/tests/language/src/Assertion...
tests/language/src/AssertionTest.dart:43: Expect.equals(1, testClosure());
Could you add a few more test cases, for example:

* test that assert(true) works

* test that assert(unknown) works, where unknown is something that cannot be
statically determined without inlining method calls.

[jat, 2011-10-14 14:46:53]

http://codereview.chromium.org/8276014/diff/3003/tests/language/src/Assertion...
File tests/language/src/AssertionTest.dart (right):

http://codereview.chromium.org/8276014/diff/3003/tests/language/src/Assertion...
tests/language/src/AssertionTest.dart:4: // VMOptions=--enable_type_checks
On 2011/10/14 10:45:40, ahe wrote:
> I don't think this option belongs in language tests. This is a VM specific
> option which is understood by the dartc VM "simulator", DartRunner.

It is also understood by Dartc to do the same thing -- enable asserts and
runtime type checking.
 
> Furthermore, the assert statement should also be tested in production mode.

Isn't it specified to be totally ignored in production mode?  Sure, we can have
a test to make sure that it is ignored in production mode but that side effects
in the assert statement still happen, etc -- but that would be a separate test.

[John Lenz, 2011-10-14 19:27:03]

http://codereview.chromium.org/8276014/diff/3001/tests/language/src/Assertion...
File tests/language/src/AssertionTest.dart (right):

http://codereview.chromium.org/8276014/diff/3001/tests/language/src/Assertion...
tests/language/src/AssertionTest.dart:8: class AssertionTest {
On 2011/10/14 10:45:40, ahe wrote:
> On 2011/10/13 21:35:43, John Lenz wrote:
> > On 2011/10/13 21:19:41, ahe wrote:
> > > On 2011/10/13 20:09:40, johnlenz wrote:
> > > > This is a fork of AssertTest for three change:
> > > > - use the "enable_type_checks" flag (as per spec)
> > > > - check for AssertionError instead of AssertError (as per spec)
> > > > - removes checks for un-spec'd fields on AssertionError.
> > > 
> > > If this is a fork, why is there no version history to indicate this?
> > 
> > I will correct this (svn cp are a pain to patch). I'm more intertested in
the
> > basic question:
> > What is the correct exception to throw?
> 
> AssertionError according to the specification.

great, I'll check this in then.

> 
> > What is the correct flag to pass?
> 
> I personally think it is bad form to use VM options in this case. I much more
> prefer the multi-tests that are used in, for example, ClassLiteralTest.dart.

I'll leave that for a general clean up.

> 
> > What fields are required on AssertError/AssertionError (not that I'll be
> adding
> > them in this change).
> 
> That is a library issue. According to Nicolas, it was a mistake to move
> AssertTest.dart from the runtime to language.

ok

[regis, 2011-10-14 20:40:28]

LBTM

Please revert this change breaking the vm build and see the comment below.

Thanks

http://codereview.chromium.org/8276014/diff/3003/tests/language/src/Assertion...
File tests/language/src/AssertionTest.dart (left):

http://codereview.chromium.org/8276014/diff/3003/tests/language/src/Assertion...
tests/language/src/AssertionTest.dart:23: Expect.equals(14, error.column);
John, please do not remove valid testing of the vm functionality. It is fine to
rename the class, but move this test to the vm tree and add your new test here.

Until you do that, please revert this change, since it broke the vm build.

[jat, 2011-10-14 20:44:46]

http://codereview.chromium.org/8276014/diff/3003/tests/language/src/Assertion...
File tests/language/src/AssertionTest.dart (left):

http://codereview.chromium.org/8276014/diff/3003/tests/language/src/Assertion...
tests/language/src/AssertionTest.dart:23: Expect.equals(14, error.column);
On 2011/10/14 20:40:28, regis wrote:
> John, please do not remove valid testing of the vm functionality. It is fine
to
> rename the class, but move this test to the vm tree and add your new test
here.

The test does not match the spec, so it seems reasonable to make the test match
the spec and mark the test as failing on the VM until the VM's behavior matches
the spec.

Also, there are tests like this where the VM is adding additional information
not in the spec, such as fields on exceptions, and in some cases that means we
cannot have a single test that works on both DartC and the VM.  In such cases, I
suggest that the base test be shared, which would not check
implementation-specific fields, and then there would be implementation-specific
tests which are much smaller and only check those implementation details.

I do not understand the point of keeping tests which encode non-compliant
behavior, as you seem to be suggesting.

[regis, 2011-10-14 21:04:41]

http://codereview.chromium.org/8276014/diff/3003/tests/language/src/Assertion...
File tests/language/src/AssertionTest.dart (left):

http://codereview.chromium.org/8276014/diff/3003/tests/language/src/Assertion...
tests/language/src/AssertionTest.dart:23: Expect.equals(14, error.column);
On 2011/10/14 20:44:46, jat wrote:
> On 2011/10/14 20:40:28, regis wrote:
> > John, please do not remove valid testing of the vm functionality. It is fine
> to
> > rename the class, but move this test to the vm tree and add your new test
> here.
> 
> The test does not match the spec, so it seems reasonable to make the test
match
> the spec and mark the test as failing on the VM until the VM's behavior
matches
> the spec.
> 
> Also, there are tests like this where the VM is adding additional information
> not in the spec, such as fields on exceptions, and in some cases that means we
> cannot have a single test that works on both DartC and the VM.  In such cases,
I
> suggest that the base test be shared, which would not check
> implementation-specific fields, and then there would be
implementation-specific
> tests which are much smaller and only check those implementation details.
> 
> I do not understand the point of keeping tests which encode non-compliant
> behavior, as you seem to be suggesting.

We are in agreement. I am not against renaming the class AssertError to
AssertionError to match the spec, or to remove the vm specific part from the
test in the language directory. However, the vm specific test should be
preserved by moving it (after renaming the class) to the vm test directory.

But I am against renaming the class in the test only and not renaming the
corresponding class in the core lib implementation of the vm, thereby breaking
the vm build. At least, the renamed test should be disabled for the vm and a bug
should be filed against the vm.