Proposed spec for DEP for assert with messages.

docs/language/dartLangSpec.tex

Index: docs/language/dartLangSpec.tex
diff --git a/docs/language/dartLangSpec.tex b/docs/language/dartLangSpec.tex
index 3850f200dfcfc00dea26f2a7b80fdf71a2c7f3a8..fd64edac6dc943df9b2cdf19e806e424ca7f5dc3 100644
--- a/docs/language/dartLangSpec.tex
+++ b/docs/language/dartLangSpec.tex
@@ -6457,24 +6457,34 @@ An {\em assert statement} is used to disrupt normal execution if a given boolean
 
 \begin{grammar}
 {\bf assertStatement:}
-   assert `(' conditionalExpression `)' `{\escapegrammar ;}'
+   assert `(' conditionalExpression (`,' stringLiteral)?  `)' `{\escapegrammar ;}'

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

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().

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

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".

       .
 \end{grammar}
       
 \LMHash{}
-The assert statement has no effect in production mode. In checked mode, execution of an assert statement \code{\ASSERT{}($e$);} proceeds as follows:
+The assert statement has no effect in production mode. In checked mode, execution of an assert statement \code{\ASSERT{}($e$);} or \code{\ASSERT{}($e$, $s$);} proceeds as follows:
 
 \LMHash{}
 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$. 

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

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.

-It is a dynamic type error if $o$ is not of type \code{bool} or of type \code{Function}, or if $r$ is not of type \code{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 \code{AssertionError} is thrown.
+It is a dynamic type error if $o$ is not of type \code{bool} or of type \code{Function}, or if $r$ is not of type \code{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, and the assertion is of the form \code{\ASSERT{}($e$);} then the expression \cd{\THROW{} \NEW{} AssertionError()} is evaluated.

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

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.

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

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."

[gbracha, 2015/09/02 19:58:22]

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. 
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.
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
Dart has factories for this reason.
I'm not sure I want to give you license to do something better :-). 

[Lasse Reichstein Nielsen, 2015/09/03 05:22:07]

Ack. 
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.
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.

+ 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.

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

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);

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

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.

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

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.

[Lasse Reichstein Nielsen, 2015/09/03 05:22:07]

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.

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

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

+
+\rationale {
+The optional argument $s$ is intended to allow a suitable message to be associated with the assertion. An implementation should endeavour to display the message in a manner that is useful to the developer.
+}
+\commentary{
+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.
+}
 
 %\Q{Might be cleaner to define it as \code{if (!$e$) \{\THROW{} \NEW{} AssertionError();\}} (in checked mode only).
 %What about an error message as part of the assert?}
 
 \LMHash{}
- 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 $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}.

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

I wouldn't make that requirement. I have no problem with a message that is not a
string.

[gbracha, 2015/09/02 19:58:22]

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:07]

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.

 
-\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.
+\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.
 }
 
 %If a lexically visible declaration named \code{assert} is in scope, an assert statement