Proposed spec for DEP for assert with messages.

Proposed spec for DEP for assert with messages.

BUG=

[gbracha, 2015-09-01 18:30:53]

gbracha@google.com changed reviewers:
+ paulberry@google.com

[gbracha, 2015-09-01 18:30:54]

Proposed assert spec changes, for those who prefer latex input to its output :-)

[Paul Berry, 2015-09-01 19:59:49]

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec.tex
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:6460: assert `(' conditionalExpression (`,'
stringLiteral)?  `)' `{\escapegrammar ;}'
Did you mean to say "expression" instead of "stringLiteral"?  The text below
seems to imply that we permit expressions that might not evaluate to strings
("The definition above implies that if $s$ does not evaluate to a
\cd{String}..." as well as "It is a static type warning if the type of $s$ may
not be assigned to \cd{String}.")

Note that in the prototype VM implementation
(https://codereview.chromium.org/1307363005), the message is permitted to be any
expression, and if the runtime type of the message is not a String, it is
automatically converted to a string using Error.safeToString.  Which is
approximately what was discussed on the mailing list
(https://groups.google.com/a/dartlang.org/forum/#!topic/core-dev/yNiTFYmtmwY)
with the exception that the mailing list suggested converting the message to a
string using toString().

[sra1, 2015-09-01 22:46:46]

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

[sra1, 2015-09-01 22:46:47]

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec.tex
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:6472: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression \cd{\THROW{}
\NEW{} AssertionError(s)} is evaluated.
It is best to have a different constructor per purpose.

It is difficult to tell the difference in the constructor between no arguments
and one argument.  Hacks to distinguish the difference don't work well,
especially if the type of the argument does not permit 'out of band' values.

Notice now we added ArgumentError.value() to distinguish the 'message' case from
the 'irritant' case. Without it we could not tell if the argument was a message
or a bad string value, or whether the bad value happened to be null vs there
being no message.

Consider:

  assert(false);
  assert(false, null);

[gbracha, 2015-09-01 23:53:10]

So the constructor is specified in the DEP. The language spec uses it. If the
library code is changed so there are two distinct constructors, we can align the
language spec with that.

[Lasse Reichstein Nielsen, 2015-09-02 07:36:48]

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

[Lasse Reichstein Nielsen, 2015-09-02 07:36:49]

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec.tex
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:6460: assert `(' conditionalExpression (`,'
stringLiteral)?  `)' `{\escapegrammar ;}'
You can always convert an object to a string (or throw trying), so I see no
problem with having any expression here.
We can store the object in the AssertionError object and stringfy it in the
error's toString, or we can convert it to string before storing it, that doesn't
really matter, but it's easier to just write an expression here than having to
wrap it in a string literal expression - "${myexpression}" instead of just
myexpression.

I recommend just requiring "expression".

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:6468: The conditional expression $e$ is evaluated
to an object $o$. If the class of $o$ is a subtype of \code{Function} then let
$r$ be the result of invoking $o$ with no arguments. Otherwise, let $r$ be $o$.
This seems to assume that both the evaluation of expression and the invocation
of o if it's a function, have results. Since both can easily throw, that should
probably be mentioned too - in that case the assert statement throws the same
thing.

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:6471: If the assertion failed, and the assertion
is of the form \code{\ASSERT{}($e$);} then the expression \cd{\THROW{} \NEW{}
AssertionError()} is evaluated.
Don't use simple syntactic rewrites like this, it doesn't account for scopes.
If AssertionError refers to something else than the declaration in dart:core,
then the rewrite won't do what you expect.

Instead write:
  Create an object implementing the class AssertionError from dart:core
  with a null message, and throw that object.

This also avoids specifying the exact constructor to use (which is limiting) and
the exact class to make an instance of (also limiting).
The VM uses a much better subclass of AssertionError which can show the source
code of the assertion that failed.

I agree that the goal is to have checked mode assert(cond, msg) act *something
like*:
  
  _helper(b) => b is Function ? b() : b;
  if (!_helper(cond)) throw new AssertionError(msg);

but specifying it exactly is dangerous because it prevents doing something
better.

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:6472: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression \cd{\THROW{}
\NEW{} AssertionError(s)} is evaluated.
The ArgumentError.value is just there to allow ArgumentError(_) for backwards
compatibility. I'd remove the original and rename ArgumentError.value to
ArgumentError in a heartbeat if it wasn't a breaking change.

I don't have a problem with
  AssertionError([Object message])
as a constructor, and I've already added it in the CL.

That said, it should be:
  Create an object implementing the class AssertionError from dart:core
  for which the "message" getter returns o, and throw that object.

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:6485: It is a static type warning if the type of
$e$ may not be assigned to either  \code{bool} or $() \rightarrow$ \code{bool}. 
It is a static type warning if the type of $s$ may not be assigned to
\cd{String}.
I wouldn't make that requirement. I have no problem with a message that is not a
string.

[sra1, 2015-09-02 18:08:03]

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec.tex
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:6471: If the assertion failed, and the assertion
is of the form \code{\ASSERT{}($e$);} then the expression \cd{\THROW{} \NEW{}
AssertionError()} is evaluated.
On 2015/09/02 07:36:49, Lasse Reichstein Nielsen wrote:
> Don't use simple syntactic rewrites like this, it doesn't account for scopes.
> If AssertionError refers to something else than the declaration in dart:core,
> then the rewrite won't do what you expect.
> 
> Instead write:
>   Create an object implementing the class AssertionError from dart:core
>   with a null message, and throw that object.
> 
> This also avoids specifying the exact constructor to use (which is limiting)
and
> the exact class to make an instance of (also limiting).
> The VM uses a much better subclass of AssertionError which can show the source
> code of the assertion that failed.
> 
> I agree that the goal is to have checked mode assert(cond, msg) act *something
> like*:
>   
>   _helper(b) => b is Function ? b() : b;
>   if (!_helper(cond)) throw new AssertionError(msg);
> 
> but specifying it exactly is dangerous because it prevents doing something
> better.

Well stated. This was exactly my point in the email thread with Gilad when I
said "In the spirit of being useful, there should be no impediment to capturing
more information."

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:6472: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression \cd{\THROW{}
\NEW{} AssertionError(s)} is evaluated.
On 2015/09/02 07:36:49, Lasse Reichstein Nielsen wrote:

> 
> I don't have a problem with
>   AssertionError([Object message])
> as a constructor, and I've already added it in the CL.

I would like to be able to distinguish between

   assert(e);
   assert(e, x);

when x happens to be null.  The intention is clearly different.


> 
> That said, it should be:
>   Create an object implementing the class AssertionError from dart:core
>   for which the "message" getter returns o, and throw that object.

[gbracha, 2015-09-02 19:58:23]

Changed stringLiteral to expression. Haven't changed anything else so far.

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec.tex
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:6471: If the assertion failed, and the assertion
is of the form \code{\ASSERT{}($e$);} then the expression \cd{\THROW{} \NEW{}
AssertionError()} is evaluated.
On 2015/09/02 07:36:49, Lasse Reichstein Nielsen wrote:
> Don't use simple syntactic rewrites like this, it doesn't account for scopes.
> If AssertionError refers to something else than the declaration in dart:core,
> then the rewrite won't do what you expect.

I refer you to section 5 of the spec, which states:

References to otherwise unspecified names of program entities (such as classes
or functions) are interpreted as the names of members of the Dart core library. 

> 
> Instead write:
>   Create an object implementing the class AssertionError from dart:core
>   with a null message, and throw that object.

Ok, two points here. 

(1) Explicit reference to dart:core is unnecessary as I note above. 
(2) You'd like to allow creation of a subclass of AssertionError.  We could do
that. Or we could make AssertionError have factories.

> 
> This also avoids specifying the exact constructor to use (which is limiting)

Using actual syntax has the advantage that a lot of the semantics are already
defined. It avoids duplication, and the risk of inconsistencies due to special
rules (like what happens due to an exception, a type mismatch etc.).

 and
> the exact class to make an instance of (also limiting).

Dart has factories for this reason.

> The VM uses a much better subclass of AssertionError which can show the source
> code of the assertion that failed.
> 
> I agree that the goal is to have checked mode assert(cond, msg) act *something
> like*:
>   
>   _helper(b) => b is Function ? b() : b;
>   if (!_helper(cond)) throw new AssertionError(msg);
> 
> but specifying it exactly is dangerous because it prevents doing something
> better.

I'm not sure I want to give you license to do something better :-).

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:6485: It is a static type warning if the type of
$e$ may not be assigned to either  \code{bool} or $() \rightarrow$ \code{bool}. 
It is a static type warning if the type of $s$ may not be assigned to
\cd{String}.
On 2015/09/02 07:36:49, Lasse Reichstein Nielsen wrote:
> I wouldn't make that requirement. I have no problem with a message that is not
a
> string.

Yes, people have debated that.  There is a trade off between allowing arbitrary
objects for some notion of  convenience and type safety.  Effectively you are
asking for a coercion from anything to a string (as in print). I don't think
this is appropriate here.

[Lasse Reichstein Nielsen, 2015-09-03 05:22:08]

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec.tex
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:6471: If the assertion failed, and the assertion
is of the form \code{\ASSERT{}($e$);} then the expression \cd{\THROW{} \NEW{}
AssertionError()} is evaluated.
On 2015/09/02 19:58:22, gbracha wrote:
> On 2015/09/02 07:36:49, Lasse Reichstein Nielsen wrote:
> > Don't use simple syntactic rewrites like this, it doesn't account for
scopes.
> > If AssertionError refers to something else than the declaration in
dart:core,
> > then the rewrite won't do what you expect.
> 
> I refer you to section 5 of the spec, which states:
> 
> References to otherwise unspecified names of program entities (such as classes
> or functions) are interpreted as the names of members of the Dart core
library. 

Ack. 

> > Instead write:
> >   Create an object implementing the class AssertionError from dart:core
> >   with a null message, and throw that object.
> 
> Ok, two points here. 
> 
> (1) Explicit reference to dart:core is unnecessary as I note above. 
> (2) You'd like to allow creation of a subclass of AssertionError.  We could do
> that. Or we could make AssertionError have factories.

Factories won't work. What the VM does is to provide information that the user
does not have, and that we don't want to provide a constructor for. It has its
own internal AssertionError with more fields and features, and is free to change
it at any time. We don't want to make a public factory constructor creating
that, nor do we want to require dart2js having such a constructor.


> > This also avoids specifying the exact constructor to use (which is limiting)
> 
> Using actual syntax has the advantage that a lot of the semantics are already
> defined. It avoids duplication, and the risk of inconsistencies due to special
> rules (like what happens due to an exception, a type mismatch etc.).
> 
>  and
> > the exact class to make an instance of (also limiting).
> 
> Dart has factories for this reason.

Requiring a specific constructor call is itself limiting because that means the
same constructor must be used by the VM and dart2js.
The VM does what it does internally, and there is no need to restrict that.

> 
> > The VM uses a much better subclass of AssertionError which can show the
source
> > code of the assertion that failed.
> > 
> > I agree that the goal is to have checked mode assert(cond, msg) act
*something
> > like*:
> >   
> >   _helper(b) => b is Function ? b() : b;
> >   if (!_helper(cond)) throw new AssertionError(msg);
> > 
> > but specifying it exactly is dangerous because it prevents doing something
> > better.
> 
> I'm not sure I want to give you license to do something better :-).

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:6472: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression \cd{\THROW{}
\NEW{} AssertionError(s)} is evaluated.
On 2015/09/02 18:08:03, sra1 wrote:
> I would like to be able to distinguish between
> 
>    assert(e);
>    assert(e, x);
> 
> when x happens to be null.  The intention is clearly different.

I can see the reason for that. It will require either a sentinel value
representing "no message" (not being null), an extra boolean or a subclass.
Are we sure that's really necessary? It would be very consistent with function
behavior if assert(e) was exactly the same as assert(e, null).

Dart's asserts are not "stop-the-world" tests, you can catch the thrown error
and continue running - we don't know that the message is immediately printed to
console.

Maybe we should change the syntax to:
 assert expression [':' expression] ';'
instead of making it look like a function call. If so, we need to do so before
adding the message. It would be backwards compatible.

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:6485: It is a static type warning if the type of
$e$ may not be assigned to either  \code{bool} or $() \rightarrow$ \code{bool}. 
It is a static type warning if the type of $s$ may not be assigned to
\cd{String}.
I'm not asking for any coercion. I'd prefer to store the value exactly in the
assertion error.
If you chose to call toString on that error, we will likely chose to include the
message in the result in some way, which may or may not call toString on it. 

The implementations I have done so far store the value directly and use
Error.safeToString in the toString method. That includes strings as you would
expect, but avoids calling toString on untrusted objects.

[sra1, 2015-09-17 19:55:22]

https://codereview.chromium.org/1324933002/diff/20001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/20001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6478: The definition above implies that if $s$
does not evaluate to a \cd{String}, a dynamic error is thrown, because the
argument to the constructor of \cd{AssertionError} is typed as \cd{String} and
we are running in checked mode. It also implies that $s$ is not evaluated if the
assertion succeeded, and that any exception that occurs during evaluation of $s$
is thrown and propagated as usual, in which case the assertion error is lost.
Where is it specified that the argument to AssertionError's constructor is typed
as String?

https://codereview.chromium.org/1324933002/diff/20001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6485: It is a static type warning if the type of
$e$ may not be assigned to either  \code{bool} or $() \rightarrow$ \code{bool}. 
It is a static type warning if the type of $s$ may not be assigned to
\cd{String}.
I think it is a mistake to constrain the message to String.

This will encourage the user to write expressions to generate strings where it
is not really necessary.

Compare:

    assert(age >= 10, '$age is too young');

    assert(age >= 10, '$age');

    assert(age >= 10, age);


The proposal forces the first two, but this is a worse user experience in almost
every way:

1. Less concise.

2. The risk that toString() throws a confounding error, obfuscating the true
problem.

3. The user has to worry about safely generating strings at every assert rather
than have the system take care of it in one place.

4. The string generating expressions take up space in the compiled
representation of the program (e.g. JavaScript)

5. The string generating expressions are largely untestable.

It is often *impossible* to test the assertion messages.  If the assertion
condition expresses a deep invariant then the message is unreachable!  
A disproportionate number of defects are on error paths. Lets help the user not
add more by letting them assert with general values.

[Lasse Reichstein Nielsen, 2015-09-18 12:26:28]

https://codereview.chromium.org/1324933002/diff/20001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/20001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6472: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression \cd{\THROW{}
\NEW{} AssertionError(s)} is evaluated.
I'd recommend against specifying the exact constructor used to create the
AssertionError.

We can't change the AssertionError constructor to a factory at this point
(someone may subclass AssertioneError), so an "AssertionError(s)" constructor
will be generative and have to store the parameter on the object.

Since TypeError extends AssertionError (and we can't change that now, that would
also be a breaking change), any message that we put on the AssertionError class
will be carried over to the TypeError class where it won't make sense. 

If we don't specify the exact constructor to use here, just that it generates an
object that implements AssertionError, then we can use any constructor and
subclass for it - which implementations already do for AssertionError().

Also, you are saying that "throw new AssertionError()" is evaluated, however
that expression is *not part of the program*. When throwing, we capture a stack
trace, but what is the stack trace for code that doesn't have a file or
location? Generally, I'm concerned when non-existing code is explicitly
evaluated. It would at least be better if it the semantics was written as doing
something similar to that expression instead of *being* that expression.

[Lasse Reichstein Nielsen, 2015-09-22 08:41:58]

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec.tex
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:6472: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression \cd{\THROW{}
\NEW{} AssertionError(s)} is evaluated.
On 2015/09/02 07:36:49, Lasse Reichstein Nielsen wrote:
>
> I don't have a problem with
>   AssertionError([Object message])
> as a constructor, and I've already added it in the CL.

That has changed. I now have a problem with it, so if anything, it should be
"AssertionError.withMessage(message)".

[Lasse Reichstein Nielsen, 2015-10-15 09:14:48]

https://codereview.chromium.org/1324933002/diff/20001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/20001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6478: The definition above implies that if $s$
does not evaluate to a \cd{String}, a dynamic error is thrown, because the
argument to the constructor of \cd{AssertionError} is typed as \cd{String} and
we are running in checked mode. It also implies that $s$ is not evaluated if the
assertion succeeded, and that any exception that occurs during evaluation of $s$
is thrown and propagated as usual, in which case the assertion error is lost.
I guess that's implicitly specified by this commentary.

https://codereview.chromium.org/1324933002/diff/20001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6478: The definition above implies that if $s$
does not evaluate to a \cd{String}, a dynamic error is thrown, because the
argument to the constructor of \cd{AssertionError} is typed as \cd{String} and
we are running in checked mode. It also implies that $s$ is not evaluated if the
assertion succeeded, and that any exception that occurs during evaluation of $s$
is thrown and propagated as usual, in which case the assertion error is lost.
This means that a type error in the assertion message will hide the real
assertion error - which is always more important to the user.

We should not throw in the message if it can be avoided. If anything, we should
actively catch errors in the message expression to prevent them from causing the
real assertion error to be lost.

[gbracha, 2015-11-04 00:36:05]

So here is a revision in line with the discussion in the DEP committee. Let me
know what you think.

[Lasse Reichstein Nielsen, 2015-11-04 11:58:46]

https://codereview.chromium.org/1324933002/diff/40001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/40001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6472: If the assertion failed, and the assertion
is of the form \code{\ASSERT{}($e$);} an \cd{AssertionError()} is thrown.
I don't think it needs the "()" after AssertionError. Ditto below.

https://codereview.chromium.org/1324933002/diff/40001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6474: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression $s$ is evaluated
to a value $o_s$. If evaluation of $s$ fails, an \cd{AssertionError()} is
thrown. Otherwise, the \cd{toString()} method is invoked on $o_s$ resulting in a
string object $m$. If the invocation of \cd{toString()} throws an exception, an
\cd{AssertionError()} is thrown. Otherwise,
We could remove the toString call completely, and just use the object o_s as
message.
That avoids the entire problem of handling it if toString throws, and allows the
AssertionError to use Error.safeToString on the object if it wants to.

https://codereview.chromium.org/1324933002/diff/40001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6491: \rationale{Why is this a statement, not a
built in function call? Because it is handled magically so it has no effect and
no overhead in production mode. Also, in the absence of final methods. one could
not prevent it being overridden (though there is no real harm in that).  It
cannot be viewed as a function call that is being optimized away because the
argument might have side effects. And lastly, the optional string message would
have to be evaluated in all cases if \ASSERT{} were a function.
string message -> message

[Paul Berry, 2015-11-04 16:48:17]

lgtm

https://codereview.chromium.org/1324933002/diff/40001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/40001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6474: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression $s$ is evaluated
to a value $o_s$. If evaluation of $s$ fails, an \cd{AssertionError()} is
thrown. Otherwise, the \cd{toString()} method is invoked on $o_s$ resulting in a
string object $m$. If the invocation of \cd{toString()} throws an exception, an
\cd{AssertionError()} is thrown. Otherwise,
On 2015/11/04 11:58:46, Lasse Reichstein Nielsen wrote:
> We could remove the toString call completely, and just use the object o_s as
> message.
> That avoids the entire problem of handling it if toString throws, and allows
the
> AssertionError to use Error.safeToString on the object if it wants to.

I'm not convinced that would be an improvement, for two reasons.

1. It's not uncommon for a complex object to have a toString method which
renders its content in an easily human-readable form.  It seems likely that
users are going to want that custom toString method to be used when generating
assertion failure messages.

2. Most of the time people are probably going to construct assertion messages
using string interpolation, e.g. `assert(x == y, "Unexpected difference between
$x and $y");`.  In this case toString() is going to get used to translate x and
y to strings, no matter what.  The case of using a non-string as the assertion
message is going to be the rarer case, so whatever behavior we choose for it
should be as unsurprising as we can feasibly make it.  I like the current
formulation because it makes the semantics of `assert(..., x);` and `assert(...,
"$x")` exactly the same, and I think that will be the least surprising to users.

So personally I am fine leaving it as is.

[gbracha, 2015-11-04 21:27:30]

https://codereview.chromium.org/1324933002/diff/40001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/40001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6472: If the assertion failed, and the assertion
is of the form \code{\ASSERT{}($e$);} an \cd{AssertionError()} is thrown.
On 2015/11/04 11:58:46, Lasse Reichstein Nielsen wrote:
> I don't think it needs the "()" after AssertionError. Ditto below.

Done.

https://codereview.chromium.org/1324933002/diff/40001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6474: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression $s$ is evaluated
to a value $o_s$. If evaluation of $s$ fails, an \cd{AssertionError()} is
thrown. Otherwise, the \cd{toString()} method is invoked on $o_s$ resulting in a
string object $m$. If the invocation of \cd{toString()} throws an exception, an
\cd{AssertionError()} is thrown. Otherwise,
On 2015/11/04 16:48:17, Paul Berry wrote:
> On 2015/11/04 11:58:46, Lasse Reichstein Nielsen wrote:
> > We could remove the toString call completely, and just use the object o_s as
> > message.
> > That avoids the entire problem of handling it if toString throws, and allows
> the
> > AssertionError to use Error.safeToString on the object if it wants to.
> 
> I'm not convinced that would be an improvement, for two reasons.
> 
> 1. It's not uncommon for a complex object to have a toString method which
> renders its content in an easily human-readable form.  It seems likely that
> users are going to want that custom toString method to be used when generating
> assertion failure messages.
> 
> 2. Most of the time people are probably going to construct assertion messages
> using string interpolation, e.g. `assert(x == y, "Unexpected difference
between
> $x and $y");`.  In this case toString() is going to get used to translate x
and
> y to strings, no matter what.  The case of using a non-string as the assertion
> message is going to be the rarer case, so whatever behavior we choose for it
> should be as unsurprising as we can feasibly make it.  I like the current
> formulation because it makes the semantics of `assert(..., x);` and
`assert(...,
> "$x")` exactly the same, and I think that will be the least surprising to
users.
> 
> So personally I am fine leaving it as is.

I agree with Paul. In 99% of the cases, s will be a string anyway, and in the
rest, you'll want the toString() of the object. I'm leaving this as is.

https://codereview.chromium.org/1324933002/diff/40001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6491: \rationale{Why is this a statement, not a
built in function call? Because it is handled magically so it has no effect and
no overhead in production mode. Also, in the absence of final methods. one could
not prevent it being overridden (though there is no real harm in that).  It
cannot be viewed as a function call that is being optimized away because the
argument might have side effects. And lastly, the optional string message would
have to be evaluated in all cases if \ASSERT{} were a function.
On 2015/11/04 11:58:46, Lasse Reichstein Nielsen wrote:
> string message -> message

Done.

[Lasse Reichstein Nielsen, 2015-11-05 16:22:26]

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6473: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression $s$ is evaluated
to a value $o_s$. If evaluation of $s$ fails, an \cd{AssertionError} is thrown.
Otherwise, the \cd{toString()} method is invoked on $o_s$ resulting in a string
object $m$. If the invocation of \cd{toString()} throws an exception, an
\cd{AssertionError} is thrown. Otherwise,
Calling o_s.toString is not guaranteed to result in a String object. Even in
checked mode, toString can be overwritten to return a non-string.
We need to handle this somehow, perhaps by considering it a failure, or by
allowing the non-string to end up in the AssertionError.

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6474: an \cd{AssertionError} $x$ is created with
error message $m$.
Maybe if evaluation of $s$ fails, we should let the object $o_s$ be the object
thrown by the failed evaluation. That way it isn't lost, and the user has a
chance to figure out why the message is wrong.

Likewise, if $o_s.toString()$ fails, let $m$ be a string derived from the thrown
object (don't specify that too much, then we can use safeToString).

[Brian Wilkerson, 2015-11-05 17:04:36]

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

[Brian Wilkerson, 2015-11-05 17:04:37]

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6473: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression $s$ is evaluated
to a value $o_s$. If evaluation of $s$ fails, an \cd{AssertionError} is thrown.
Otherwise, the \cd{toString()} method is invoked on $o_s$ resulting in a string
object $m$. If the invocation of \cd{toString()} throws an exception, an
\cd{AssertionError} is thrown. Otherwise,
True. We have the same problem for string interpolation. I don't see any
discussion in the spec about what we do in that case. The VM handles this by
throwing an exception of type ArgumentError. Perhaps we should just do the same
here.

[Bob Nystrom, 2015-11-05 17:50:04]

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

[Bob Nystrom, 2015-11-05 17:50:04]

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6473: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression $s$ is evaluated
to a value $o_s$. If evaluation of $s$ fails, an \cd{AssertionError} is thrown.
Otherwise, the \cd{toString()} method is invoked on $o_s$ resulting in a string
object $m$. If the invocation of \cd{toString()} throws an exception, an
\cd{AssertionError} is thrown. Otherwise,
> If evaluation of $s$
       fails, an \cd{AssertionError} is thrown.

I don't think we should mask the original error here. If the message fails to
evaluate, that's a more significant error than my invariant failing. I want to
know that error *first* so I can fix it. Then I'll get back to fixing my
invariant failing.

If we throw an AssertionError then it will guide users down the wrong path. They
will spend their time figuring out why the assertion invariant is failing (which
is certainly an issue). If they fix that *first* then the error will go away
completely before they get a chance to fix the message failure.

I think the right thing to do is just let the original error propagate.

> Otherwise, the \cd{toString()} method 
      is invoked on $o_s$ resulting in a string object $m$.

I don't think shouldn't be done here. Instead, just store the original message
object—whatever its type—in the AssertionError object. When the AssertionError
is printed, *then* that will stringify it.

[gbracha, 2015-11-05 19:25:01]

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6473: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression $s$ is evaluated
to a value $o_s$. If evaluation of $s$ fails, an \cd{AssertionError} is thrown.
Otherwise, the \cd{toString()} method is invoked on $o_s$ resulting in a string
object $m$. If the invocation of \cd{toString()} throws an exception, an
\cd{AssertionError} is thrown. Otherwise,
On 2015/11/05 17:50:04, Bob Nystrom wrote:
> > If evaluation of $s$
>        fails, an \cd{AssertionError} is thrown.
> 
> I don't think we should mask the original error here. If the message fails to
> evaluate, that's a more significant error than my invariant failing. I want to
> know that error *first* so I can fix it. Then I'll get back to fixing my
> invariant failing.

Interestingly, the original spec would ensure that behavior, but people argued
the opposite. If there is a clear consensus, I'm happy to change it. 

> 
> If we throw an AssertionError then it will guide users down the wrong path.
They
> will spend their time figuring out why the assertion invariant is failing
(which
> is certainly an issue). If they fix that *first* then the error will go away
> completely before they get a chance to fix the message failure.
> 
> I think the right thing to do is just let the original error propagate.
> 
> > Otherwise, the \cd{toString()} method 
>       is invoked on $o_s$ resulting in a string object $m$.
> 
> I don't think shouldn't be done here. Instead, just store the original message
> object—whatever its type—in the AssertionError object. When the AssertionError
> is printed, *then* that will stringify it.

This amounts to leaving the semantics to the AssertionError class. It will have
to decide how to print the object (and it should use that object's toString
anyway). And it will decide how to catch any error in toString including whether
it returned a string or not. I don't see any advantage to that. We might as well
decide that behavior here.

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6474: an \cd{AssertionError} $x$ is created with
error message $m$.
On 2015/11/05 16:22:25, Lasse Reichstein Nielsen wrote:
> Maybe if evaluation of $s$ fails, we should let the object $o_s$ be the object
> thrown by the failed evaluation. That way it isn't lost, and the user has a
> chance to figure out why the message is wrong.
> 
> Likewise, if $o_s.toString()$ fails, let $m$ be a string derived from the
thrown
> object (don't specify that too much, then we can use safeToString).

So if we go back to propagating failures in message evaluation, why not specify
that we do a throw(new AssertionError(s)) and be done?

[Paul Berry, 2015-11-05 19:35:26]

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6473: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression $s$ is evaluated
to a value $o_s$. If evaluation of $s$ fails, an \cd{AssertionError} is thrown.
Otherwise, the \cd{toString()} method is invoked on $o_s$ resulting in a string
object $m$. If the invocation of \cd{toString()} throws an exception, an
\cd{AssertionError} is thrown. Otherwise,
On 2015/11/05 17:50:04, Bob Nystrom wrote:
> > If evaluation of $s$
>        fails, an \cd{AssertionError} is thrown.
> 
> I don't think we should mask the original error here. If the message fails to
> evaluate, that's a more significant error than my invariant failing. I want to
> know that error *first* so I can fix it. Then I'll get back to fixing my
> invariant failing.
> 
> If we throw an AssertionError then it will guide users down the wrong path.
They
> will spend their time figuring out why the assertion invariant is failing
(which
> is certainly an issue). If they fix that *first* then the error will go away
> completely before they get a chance to fix the message failure.
> 
> I think the right thing to do is just let the original error propagate.

Personally, I feel the opposite way.  I use assert to verify design invariants
which may be relied upon by other code, possibly including code inside the
implementation of toString().  So if one of my assertions fires, that means that
some invariant has been invalidated, and that *may* cause toString() to fail
(but usually won't).  If violating an invariant causes toString() to fail, I
don't consider that a bug in toString().

So to me, when an invariant is violated, the most important thing is that I want
to be pointed to the line of code where the violation was discovered.  Second
priority is to get my custom assertion message, since that will help me in
debugging.  But if the custom assertion message can't be computed due to an
exception, I don't want to have to go "fix" toString() to harden it against all
possible invariant violations.  I just want to be pointed to the failing assert
statement so I can fix it and get on with my life.

[Bob Nystrom, 2015-11-05 21:10:51]

On 2015/11/05 19:35:26, Paul Berry wrote:
>
https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
> File docs/language/dartLangSpec.tex (right):
> 
>
https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
> docs/language/dartLangSpec.tex:6473: If the assertion failed and the assertion
> is of the form \code{\ASSERT{}($e$, $s$);} then the expression $s$ is
evaluated
> to a value $o_s$. If evaluation of $s$ fails, an \cd{AssertionError} is
thrown.
> Otherwise, the \cd{toString()} method is invoked on $o_s$ resulting in a
string
> object $m$. If the invocation of \cd{toString()} throws an exception, an
> \cd{AssertionError} is thrown. Otherwise,
> On 2015/11/05 17:50:04, Bob Nystrom wrote:
> > > If evaluation of $s$
> >        fails, an \cd{AssertionError} is thrown.
> > 
> > I don't think we should mask the original error here. If the message fails
to
> > evaluate, that's a more significant error than my invariant failing. I want
to
> > know that error *first* so I can fix it. Then I'll get back to fixing my
> > invariant failing.
> > 
> > If we throw an AssertionError then it will guide users down the wrong path.
> They
> > will spend their time figuring out why the assertion invariant is failing
> (which
> > is certainly an issue). If they fix that *first* then the error will go away
> > completely before they get a chance to fix the message failure.
> > 
> > I think the right thing to do is just let the original error propagate.
> 
> Personally, I feel the opposite way.  I use assert to verify design invariants
> which may be relied upon by other code, possibly including code inside the
> implementation of toString().  So if one of my assertions fires, that means
that
> some invariant has been invalidated, and that *may* cause toString() to fail
> (but usually won't).  If violating an invariant causes toString() to fail, I
> don't consider that a bug in toString().

Huh. My feeling is that if you require that invariant inside the toString()
method of the message you use to assert that invariant itself, you've probably
got bigger problems. It's not like this is calling toString() on an arbitrary
object. It's on the thing you explicitly passed to assert().

> 
> So to me, when an invariant is violated, the most important thing is that I
want
> to be pointed to the line of code where the violation was discovered.

Regardless of which error is thrown, you'll still see the right location on the
callstack.

> Second priority is to get my custom assertion message, since that will help me
in
> debugging.  But if the custom assertion message can't be computed due to an
> exception, I don't want to have to go "fix" toString() to harden it against
all
> possible invariant violations.  I just want to be pointed to the failing
assert
> statement so I can fix it and get on with my life.

My concern then is that you'll fix the code that causes the assertion to fail
and never fix the code causing the message's toString() to fail. As soon as you
fix the former, it prevents you from discovering problems in the latter since it
never gets executed.

I look at it like this:

if (someBuggyCodeThatShouldAlwaysReturnFalse) {
  someMoreBuggyCode();
}

I think it's important to fix someMoreBuggyCode() *first*.

- bob

[Lasse Reichstein Nielsen, 2015-11-09 11:59:40]

On 2015/11/05 21:10:51, Bob Nystrom wrote:
> I look at it like this:
> 
> if (someBuggyCodeThatShouldAlwaysReturnFalse) {
>   someMoreBuggyCode();
> }
> 
> I think it's important to fix someMoreBuggyCode() *first*.

You have a point.

I've been looking at it from the perspective of the end-user.
If they see an assert failure somewhere in library code, then they should be
fairly sure that something is wrong with the library. If they see an
ArgumentError, they may think it's their own fault. For someone who isn't
writing the assert, the fact that an assert fails is at least as important as an
error in the assertion's message, and much easier to recognize.

For the person writing the library, it's always their own fault, and they would
want to see the inner error first, because they will want to fix both errors
anyway.

If the library isn't broken, the end user should not be seeing assertion errors,
so the most *commmon* case of an assertion error will be during library
development. For that reason, it may be more sensible to focus on the library
writer, and assume that published libraries won't be failing assertions "most of
the time".

[Lasse Reichstein Nielsen, 2015-11-09 11:59:41]

On 2015/11/05 21:10:51, Bob Nystrom wrote:
> I look at it like this:
> 
> if (someBuggyCodeThatShouldAlwaysReturnFalse) {
>   someMoreBuggyCode();
> }
> 
> I think it's important to fix someMoreBuggyCode() *first*.

You have a point.

I've been looking at it from the perspective of the end-user.
If they see an assert failure somewhere in library code, then they should be
fairly sure that something is wrong with the library. If they see an
ArgumentError, they may think it's their own fault. For someone who isn't
writing the assert, the fact that an assert fails is at least as important as an
error in the assertion's message, and much easier to recognize.

For the person writing the library, it's always their own fault, and they would
want to see the inner error first, because they will want to fix both errors
anyway.

If the library isn't broken, the end user should not be seeing assertion errors,
so the most *commmon* case of an assertion error will be during library
development. For that reason, it may be more sensible to focus on the library
writer, and assume that published libraries won't be failing assertions "most of
the time".

[floitsch, 2015-11-18 21:16:36]

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

[floitsch, 2015-11-18 21:16:37]

I feel bad, advocating for yet another specification of this feature.

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6473: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression $s$ is evaluated
to a value $o_s$. If evaluation of $s$ fails, an \cd{AssertionError} is thrown.
Otherwise, the \cd{toString()} method is invoked on $o_s$ resulting in a string
object $m$. If the invocation of \cd{toString()} throws an exception, an
\cd{AssertionError} is thrown. Otherwise,
Late to the party...
I'm not going to make this easier, but I think this is too complicated.
Imho asserts should be simple:
if (!assert-condition) throw new AssertionError(s);

Except that I don't want to force a specific assertion-error constructor.
Basically, I want the VM/dart2js to have the most flexibility in how it
constructs the assertion error.
It would be considered good practice to have `s` as `reason` field in the
AssertionError, but I don't think that needs to be specified in the language
spec.

I don't think that we should do a toString here. The AssertionError can do that
it its own 'toString' method. (most likely using a 'safeToString').

Something like:
===
If the assertion failed and the assertion is of the form \code{\ASSERT{}($e$,
$r$);} then the expression $r$ is evaluated to a value $o$. If the evaluation of
$o$ succeeds (i.e. no exception), then an \cd{AssertionError} with $o$ as reason
is thrown.

\commentary{
The definition above implies that $r$ is not evaluated if the assertion
succeeded.

The definition above does not specify how the AssertionError is constructed.
This gives embedders the freedom to store more helpful information in the thrown
instance.
===

[Lasse Reichstein Nielsen, 2015-11-20 10:51:58]

On 2015/11/18 at 21:16:37, floitsch wrote:

> Imho asserts should be simple:
> if (!assert-condition) throw new AssertionError(s);
> 
> Except that I don't want to force a specific assertion-error constructor. 

This is what I have already implemented for both dart2js and the VM.
Implementation-wise it's the simplest possible solution.

[gbracha, 2015-11-24 20:26:04]

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
docs/language/dartLangSpec.tex:6473: If the assertion failed and the assertion
is of the form \code{\ASSERT{}($e$, $s$);} then the expression $s$ is evaluated
to a value $o_s$. If evaluation of $s$ fails, an \cd{AssertionError} is thrown.
Otherwise, the \cd{toString()} method is invoked on $o_s$ resulting in a string
object $m$. If the invocation of \cd{toString()} throws an exception, an
\cd{AssertionError} is thrown. Otherwise,
On 2015/11/18 21:16:36, floitsch wrote:
> Late to the party...
> I'm not going to make this easier, but I think this is too complicated.
> Imho asserts should be simple:
> if (!assert-condition) throw new AssertionError(s);
> 
> Except that I don't want to force a specific assertion-error constructor.
> Basically, I want the VM/dart2js to have the most flexibility in how it
> constructs the assertion error.
> It would be considered good practice to have `s` as `reason` field in the
> AssertionError, but I don't think that needs to be specified in the language
> spec.
> 
> I don't think that we should do a toString here. The AssertionError can do
that
> it its own 'toString' method. (most likely using a 'safeToString').
> 
> Something like:
> ===
> If the assertion failed and the assertion is of the form \code{\ASSERT{}($e$,
> $r$);} then the expression $r$ is evaluated to a value $o$. If the evaluation
of
> $o$ succeeds (i.e. no exception), then an \cd{AssertionError} with $o$ as
reason
> is thrown.
> 
> \commentary{
> The definition above implies that $r$ is not evaluated if the assertion
> succeeded.
> 
> The definition above does not specify how the AssertionError is constructed.
> This gives embedders the freedom to store more helpful information in the
thrown
> instance.
> ===

So we are going around in circles. The original specification was given terms of
calling a constructor. It ensures consistent semantics and a minimum of
implementation effort, since everything is done in terms of existing mechanism. 
After endless discussion, it boils down to exactly that except that you don't
want to commit to having a constructor in the library.

As for toString, the whole purpose of it is to allow people to customize how an
object appears. They don't want some generic description generated by the
system. 

Of course, it  is all a tempest in a teapot, because in reality s will be a
string and it would be better to statically check that, but we've given up on
that. I think we should leave it as is at this point and get on with our lives.

[mit, 2015-12-03 11:51:50]

On 2015/11/24 20:26:04, gbracha wrote:
>
https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
> File docs/language/dartLangSpec.tex (right):
> 
>
https://codereview.chromium.org/1324933002/diff/60001/docs/language/dartLangS...
> docs/language/dartLangSpec.tex:6473: If the assertion failed and the assertion
> is of the form \code{\ASSERT{}($e$, $s$);} then the expression $s$ is
evaluated
> to a value $o_s$. If evaluation of $s$ fails, an \cd{AssertionError} is
thrown.
> Otherwise, the \cd{toString()} method is invoked on $o_s$ resulting in a
string
> object $m$. If the invocation of \cd{toString()} throws an exception, an
> \cd{AssertionError} is thrown. Otherwise,
> On 2015/11/18 21:16:36, floitsch wrote:
> > Late to the party...
> > I'm not going to make this easier, but I think this is too complicated.
> > Imho asserts should be simple:
> > if (!assert-condition) throw new AssertionError(s);
> > 
> > Except that I don't want to force a specific assertion-error constructor.
> > Basically, I want the VM/dart2js to have the most flexibility in how it
> > constructs the assertion error.
> > It would be considered good practice to have `s` as `reason` field in the
> > AssertionError, but I don't think that needs to be specified in the language
> > spec.
> > 
> > I don't think that we should do a toString here. The AssertionError can do
> that
> > it its own 'toString' method. (most likely using a 'safeToString').
> > 
> > Something like:
> > ===
> > If the assertion failed and the assertion is of the form
\code{\ASSERT{}($e$,
> > $r$);} then the expression $r$ is evaluated to a value $o$. If the
evaluation
> of
> > $o$ succeeds (i.e. no exception), then an \cd{AssertionError} with $o$ as
> reason
> > is thrown.
> > 
> > \commentary{
> > The definition above implies that $r$ is not evaluated if the assertion
> > succeeded.
> > 
> > The definition above does not specify how the AssertionError is constructed.
> > This gives embedders the freedom to store more helpful information in the
> thrown
> > instance.
> > ===
> 
> So we are going around in circles. The original specification was given terms
of
> calling a constructor. It ensures consistent semantics and a minimum of
> implementation effort, since everything is done in terms of existing
mechanism. 
> After endless discussion, it boils down to exactly that except that you don't
> want to commit to having a constructor in the library.
> 
> As for toString, the whole purpose of it is to allow people to customize how
an
> object appears. They don't want some generic description generated by the
> system. 
> 
> Of course, it  is all a tempest in a teapot, because in reality s will be a
> string and it would be better to statically check that, but we've given up on
> that. I think we should leave it as is at this point and get on with our
lives.

What is the status here? We have a number of work items slated for 1.14 that are
waiting for the spec to be completed (see
https://github.com/dart-lang/sdk/issues/24213)