Change assert implementation to not depend on a top-level function called 'assert'.

Removed/moved "assert" function to core_patch.dart as "_assert".
Changed resolution of assert to only accepting assert in a statement position.
Added more assert tests.

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

[Johnni Winther, 2012-09-05 09:07:05]

lgtm

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
File lib/compiler/implementation/scanner/keyword.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
lib/compiler/implementation/scanner/keyword.dart:43: // And "Dynamic".
Is there no const for Dynamic? Don't we need it?

https://chromiumcodereview.appspot.com/10915083/diff/1/tests/standalone/asser...
File tests/standalone/assert_test.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/1/tests/standalone/asser...
tests/standalone/assert_test.dart:187: testAssert(non);
I see no tests for when an assert method is in lexical scope.

[ngeoffray, 2012-09-05 10:37:10]

+ahe

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
File lib/compiler/implementation/lib/core_patch.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
lib/compiler/implementation/lib/core_patch.dart:19: * Helper function for
implementing asserts. The compiler treats this specially.
Should that be in js_helper then? I don't think it belongs to a patch file, nor
it should be private.

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:1140: bool isAssertSyntax(Expression
expression) {
I'm not sure about this code. I wish we could avoid decomposing an Expression
like this. As discussed, we should probably revisit the spec for assert.

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
File lib/compiler/implementation/scanner/keyword.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
lib/compiler/implementation/scanner/keyword.dart:43: // And "Dynamic".
What is this comment for?

[ahe, 2012-09-05 11:09:54]

Please don't submit as is.

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:1153: if
(isAssertSyntax(node.expression)) {
I'm going to express myself unambiguously. I do not mean to offend anyone :-)

But... this is simply wrong.

The resolver works bottom up, and this mode of operation is crucial for
performance and sanity.

If it isn't clear what I mean by bottom-up, please do not hesitate to discuss it
in person.

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
File lib/compiler/implementation/scanner/keyword.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
lib/compiler/implementation/scanner/keyword.dart:39: // Built-in identifiers.
Why is it necessary to change this file?

[Lasse Reichstein Nielsen, 2012-09-05 12:51:20]

Please take another look.

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
File lib/compiler/implementation/lib/core_patch.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
lib/compiler/implementation/lib/core_patch.dart:19: * Helper function for
implementing asserts. The compiler treats this specially.
That would make sense. Will move it.

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:1140: bool isAssertSyntax(Expression
expression) {
I've moved this into visitSend now, so it only deconstructs one level (well, two
for the identifier).

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:1153: if
(isAssertSyntax(node.expression)) {
Try seeing if the new version is better.

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
File lib/compiler/implementation/scanner/keyword.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
lib/compiler/implementation/scanner/keyword.dart:39: // Built-in identifiers.
It isn't. 
I keep getting confused by this "pseudo keyword" name which doesn't appear in
the specification. We don't actually handle built-in identifiers correctly, but
I still like to know which ones are which.

If you don't like it, I can revert it.

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
lib/compiler/implementation/scanner/keyword.dart:43: // And "Dynamic".
Apparently not, to both questions.
This was just a reordering I did for my own sake. We don't handle built-in
keywords properly in the compiler, so it currently doesn't matter if we detect
them. I guess we detect 'Dynamic' magically in the type resolver.

https://chromiumcodereview.appspot.com/10915083/diff/1/lib/compiler/implement...
lib/compiler/implementation/scanner/keyword.dart:43: // And "Dynamic".
Reminder that "Dynamic" is a built-in identifier, i.e., the list of declarations
is not complete.

https://chromiumcodereview.appspot.com/10915083/diff/1/tests/standalone/asser...
File tests/standalone/assert_test.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/1/tests/standalone/asser...
tests/standalone/assert_test.dart:187: testAssert(non);
True, let's add that too. I'll add a lexicalAssert on the superclasses.
This only tests that a superclass-scoped assert is correctly called except for
the exact assert syntax.

Also moved this to language.

[ahe, 2012-09-05 13:12:24]

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

https://chromiumcodereview.appspot.com/10915083/diff/4002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1347: if (selector.source.stringValue
!== "assert") return false;
I would think this should be the first thing to check for.

https://chromiumcodereview.appspot.com/10915083/diff/4002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1353: if (isAssert(node)) {
I still think you're fighting the system. Let's chat about this tomorrow or
assign this bug to me.

[Lasse Reichstein Nielsen, 2012-09-06 08:21:36]

I'm ready to chat, and in anticipation, I have made another, and I think better,
version.

[ahe, 2012-09-06 08:50:41]

Much much much better! Haven't done a full review yet.

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

https://chromiumcodereview.appspot.com/10915083/diff/2002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1209: for (Scope scope = this.scope;
scope !== null; scope = scope.parent) {
Could we make this an operation on the scope instead? For example,
lexicalLookup?

https://chromiumcodereview.appspot.com/10915083/diff/2002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1212: if (local.isAbstractField()) {
I don't believe this implementation is correct:

assert(c) {}
class Fisk {
  set assert(x) {} // This should hide assert in this lexical scope.
  void test() {
    assert(false); // This is an assert statement, no assert in lexical scope.
  }
}

Otherwise, the scope rules are wrong.

[Lasse Reichstein Nielsen, 2012-09-06 09:12:36]

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

https://chromiumcodereview.appspot.com/10915083/diff/2002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1209: for (Scope scope = this.scope;
scope !== null; scope = scope.parent) {
On 2012/09/06 08:50:42, ahe wrote:
> Could we make this an operation on the scope instead? For example,
> lexicalLookup?

How about
 for (Scope.scope in this.scope.chain) { 
? 
I.e., make a getter called chain that returns an iterable that iterates the
Scope chain?

https://chromiumcodereview.appspot.com/10915083/diff/2002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1212: if (local.isAbstractField()) {
I queried Gilad, and this interpretation of the specification is correct and
intentional.

The setter does not hide the outer declaration in this loop because of the
'continue' in the case where the getter is null. I.e., only a getter can shadow
the outer declaration.
And it shouldn't, just as a Send lookup would not be stopped by only a setter
without a getter (or a SendSet by a getter without a setter).

The dart2js scope lookup doesn't account for this, which is why I stopped using
"lexicalLookup" and wrote a manual scope traversal.

[ngeoffray, 2012-09-06 09:37:31]

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

https://chromiumcodereview.appspot.com/10915083/diff/2002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1205: * This does not find setters,
since their name would be "assert=".
what is this assert=? We don't create new names to setters.

[Lasse Reichstein Nielsen, 2012-09-06 11:16:14]

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

https://chromiumcodereview.appspot.com/10915083/diff/2002/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1205: * This does not find setters,
since their name would be "assert=".
That's from the specification.
The name of the declaration "get foo {}" is "foo", and the name of the
declaration "set foo(x){}" is "foo=".

The specification of assert says to test whether there is a declaration named
"assert" in the lexical scope. A setter does not match this.

[Lasse Reichstein Nielsen, 2012-09-06 12:49:07]

Reverted to lexical lookup on the scope.
We don't handle setters correctly, but neither does any other code, so it should
be fixed uniformly.

[ahe, 2012-09-06 22:25:52]

I'd like to do one thing at a time, removing mock.dart is not a good header for
this CL that significantly improves our handling of assert.

I would be much more comfortable looking at this if it focused on assert, also,
the other changes should be easy to land in their own CL.

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/lib/mock.dart (left):

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/lib/mock.dart:10: class InternalError {
How is this related to improving assert in dart2js?

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/lib/mock.dart:17: class StaticResolutionException
implements Exception {}
How is this related to improving assert in dart2js?

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:1210: /** Check whether [node] is the
body of the current expression statement. */
body -> expression.

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:1211: bool
isExpressionStatementBody(Node node) {
I don't like calling this a "body". isExpressionStatementExpression is more
accurate.

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:1220: // If this send is the body of
an expression statement, and is on the
body -> expression

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/scanner/keyword.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/scanner/keyword.dart:39: // Built-in identifiers.
I still don't understand why you have modified this file. Could you please
revert this change?

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/universe.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/universe.dart:169: name == const
SourceString("assert") &&
name.stringValue === 'assert'

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/universe.dart:308: Selector otherSelector = other;
This change is unnecessary and doesn't improve readability of the code.

https://chromiumcodereview.appspot.com/10915083/diff/13001/pkg/dartdoc/dartdo...
File pkg/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/13001/pkg/dartdoc/dartdo...
pkg/dartdoc/dartdoc.dart:1734: class InternalError {
How is this related to fixing assert in dart2js?

[Lasse Reichstein Nielsen, 2012-09-07 10:18:57]

I'll split this CL in two, and upload the non-assert part separately.

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/lib/mock.dart (left):

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/lib/mock.dart:10: class InternalError {
It's not.
It's related to moving stuff out of mock.dart (which included "assert", but that
one move got out of hand).

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/lib/mock.dart:17: class StaticResolutionException
implements Exception {}
It's not.

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:1210: /** Check whether [node] is the
body of the current expression statement. */
On 2012/09/06 22:25:53, ahe wrote:
> body -> expression.

Done.

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:1211: bool
isExpressionStatementBody(Node node) {
On 2012/09/06 22:25:53, ahe wrote:
> I don't like calling this a "body". isExpressionStatementExpression is more
> accurate.

Done.

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:1220: // If this send is the body of
an expression statement, and is on the
On 2012/09/06 22:25:53, ahe wrote:
> body -> expression

Done.

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/scanner/keyword.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/scanner/keyword.dart:39: // Built-in identifiers.
Sure.
The modification was only for my own ease-of-reading, so I didn't have to look
up in the specification which identifiers are built-in identifiers.

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/universe.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/universe.dart:169: name == const
SourceString("assert") &&
On 2012/09/06 22:25:53, ahe wrote:
> name.stringValue === 'assert'

Done.

https://chromiumcodereview.appspot.com/10915083/diff/13001/lib/compiler/imple...
lib/compiler/implementation/universe.dart:308: Selector otherSelector = other;
On 2012/09/06 22:25:53, ahe wrote:
> This change is unnecessary and doesn't improve readability of the code.

Done.

https://chromiumcodereview.appspot.com/10915083/diff/13001/pkg/dartdoc/dartdo...
File pkg/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/13001/pkg/dartdoc/dartdo...
pkg/dartdoc/dartdoc.dart:1734: class InternalError {
Never claimed it was.
Fixing assert wasn't actually the goal, but since it needed reimplementation
when moved, the new implementation might as well be correct.

https://chromiumcodereview.appspot.com/10915083/diff/13001/tests/language/ass...
File tests/language/assert_lexical_scope_test.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/13001/tests/language/ass...
tests/language/assert_lexical_scope_test.dart:137: class SetAssert {
I'll remove this again until there is agreement on whether this is the correct
semantics.

[ahe, 2012-09-07 11:38:05]

LGTM!

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

https://chromiumcodereview.appspot.com/10915083/diff/9019/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1236: bool
isExpressionStatementBody(Node node) {
I still don't like the word "body" in this context.

https://chromiumcodereview.appspot.com/10915083/diff/9019/tests/language/asse...
File tests/language/assert_lexical_scope_test.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/9019/tests/language/asse...
tests/language/assert_lexical_scope_test.dart:3: // BSD-style license that can
be found in the LICENSE file.
General comment: there is a bunch of documentation comments in this file that
should use /** ... */

https://chromiumcodereview.appspot.com/10915083/diff/9019/tests/language/asse...
tests/language/assert_lexical_scope_test.dart:17: // (Super)classes that declare
an "assert" member.
classes -> class.

"assert" -> [assert].

https://chromiumcodereview.appspot.com/10915083/diff/9019/tests/language/asse...
tests/language/assert_lexical_scope_test.dart:202: void assert(x) => poly(x);
I don't understand this.

[Lasse Reichstein Nielsen, 2012-09-07 12:00:28]

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

https://chromiumcodereview.appspot.com/10915083/diff/9019/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:1236: bool
isExpressionStatementBody(Node node) {
Crap, I changed that.
One ctrl-Z too many I guess.

https://chromiumcodereview.appspot.com/10915083/diff/9019/tests/language/asse...
File tests/language/assert_lexical_scope_test.dart (right):

https://chromiumcodereview.appspot.com/10915083/diff/9019/tests/language/asse...
tests/language/assert_lexical_scope_test.dart:3: // BSD-style license that can
be found in the LICENSE file.
Some changed, some added.

https://chromiumcodereview.appspot.com/10915083/diff/9019/tests/language/asse...
tests/language/assert_lexical_scope_test.dart:17: // (Super)classes that declare
an "assert" member.
This comment applies to the four following classes.
Rewritten for clarity.

https://chromiumcodereview.appspot.com/10915083/diff/9019/tests/language/asse...
tests/language/assert_lexical_scope_test.dart:202: void assert(x) => poly(x);
Comment added.