[kernel] Fix assert message.

[kernel] Fix assert message.

This mimics the behaviour of the source-based pipeline,
i.e. instead of "manually" calling _AssertionError._create and giving
the correct parameters (wrong parameters, actually), use the helper
method _AssertionError.ThrowNew.

BUG=
R=ahe@google.com, vegorov@google.com

Committed: https://github.com/dart-lang/sdk/commit/0a1583aa0480dfc49ee3f589a45bd0c58f4104d9

[jensj, 2017-06-16 06:27:29]

jensj@google.com changed reviewers:
+ ahe@google.com, kmillikin@google.com, vegorov@google.com

[jensj, 2017-06-16 06:27:30]

Source-based assert message:
```
dart --enable_asserts --enable-inlining-annotations
--optimization-counter-threshold=10 --no-background-compilation
/usr/local/google/home/jensj/code/dart-sdk/sdk/bla3.dart
'file:///usr/local/google/home/jensj/code/dart-sdk/sdk/tests/language/vm/regress_27671_other.dart':
Failed assertion: line 9 pos 10: 'f(x) && true': is not true.
```

Previous kernel-based assert message:
```
runWithKernel --enable_asserts --enable-inlining-annotations
--optimization-counter-threshold=10 --no-background-compilation
/usr/local/google/home/jensj/code/dart-sdk/sdk/bla3.dart
'::_check': Failed assertion: line 0: '<no message>': <no message>
```

New kernel-based assert message:
```
runWithKernel --enable_asserts --enable-inlining-annotations
--optimization-counter-threshold=10 --no-background-compilation
/usr/local/google/home/jensj/code/dart-sdk/sdk/bla3.dart
'file:///usr/local/google/home/jensj/code/dart-sdk/sdk/tests/language/vm/regress_27671_other.dart':
Failed assertion: line 9 pos 10: 'f(x) && true': is not true.
```

I've added ahe for fasta/dart-source based changes; Kevin and Slava for the C++
based-changes. I don't know who wrote the original kernel assert stuff, so there
might be a reason I don't know about for not doing it like this?

[jensj, 2017-06-16 06:28:28]

On 2017/06/16 06:27:30, jensj wrote:
> Source-based assert message:
> ```
> dart --enable_asserts --enable-inlining-annotations
> --optimization-counter-threshold=10 --no-background-compilation
> /usr/local/google/home/jensj/code/dart-sdk/sdk/bla3.dart
>
'file:///usr/local/google/home/jensj/code/dart-sdk/sdk/tests/language/vm/regress_27671_other.dart':
> Failed assertion: line 9 pos 10: 'f(x) && true': is not true.
> ```
> 
> Previous kernel-based assert message:
> ```
> runWithKernel --enable_asserts --enable-inlining-annotations
> --optimization-counter-threshold=10 --no-background-compilation
> /usr/local/google/home/jensj/code/dart-sdk/sdk/bla3.dart
> '::_check': Failed assertion: line 0: '<no message>': <no message>
> ```
> 
> New kernel-based assert message:
> ```
> runWithKernel --enable_asserts --enable-inlining-annotations
> --optimization-counter-threshold=10 --no-background-compilation
> /usr/local/google/home/jensj/code/dart-sdk/sdk/bla3.dart
>
'file:///usr/local/google/home/jensj/code/dart-sdk/sdk/tests/language/vm/regress_27671_other.dart':
> Failed assertion: line 9 pos 10: 'f(x) && true': is not true.
> ```
> 
> I've added ahe for fasta/dart-source based changes; Kevin and Slava for the
C++
> based-changes. I don't know who wrote the original kernel assert stuff, so
there
> might be a reason I don't know about for not doing it like this?

(okay, previously kernel didn't contain source nor have token positions, both of
which this uses, so that might be why).

[Vyacheslav Egorov (Google), 2017-06-16 06:30:08]

C++ changes LGTM

[ahe, 2017-06-16 07:39:43]

body_builder.dart lgtm with comments.

https://codereview.chromium.org/2940283002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2940283002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2662: Token
lastTokenInCondition = rightParenthesis.previous;
We want to get rid of "previous". Could you do something like this instead:

Token lastTokenInCondition = leftParenthesis;
while (lastTokenInCondition.next != rightParenthesis) {
  lastTokenInCondition = lastTokenInCondition.next;
}

Or:

Token lastTokenInCondition = rightParenthesis.offset - 1;

[ahe, 2017-06-16 07:41:51]

Rest of dart files LGTM with comments.

https://codereview.chromium.org/2940283002/diff/1/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2940283002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:3167: AssertStatement(this.condition, [this.message]) {
Make message a named argument and the offsets as named arguments as well?

[jensj, 2017-06-16 07:43:30]

https://codereview.chromium.org/2940283002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2940283002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2662: Token
lastTokenInCondition = rightParenthesis.previous;
On 2017/06/16 07:39:42, ahe wrote:
> We want to get rid of "previous". Could you do something like this instead:
> 
> Token lastTokenInCondition = leftParenthesis;
> while (lastTokenInCondition.next != rightParenthesis) {
>   lastTokenInCondition = lastTokenInCondition.next;
> }
> 
> Or:
> 
> Token lastTokenInCondition = rightParenthesis.offset - 1;

I could. But:
1) I suppose an assert can be arbitrarily long so that could potentially be
slow.
2) This could theoretically include a lot of linebreaks and whatnot at the end,
when we want to extract the source code between the start and the end. It might
not be a big deal through, I'm not sure.

What do you think?

https://codereview.chromium.org/2940283002/diff/1/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2940283002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:3167: AssertStatement(this.condition, [this.message]) {
On 2017/06/16 07:41:51, ahe wrote:
> Make message a named argument and the offsets as named arguments as well?

Will do.

[ahe, 2017-06-16 07:53:35]

https://codereview.chromium.org/2940283002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2940283002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2662: Token
lastTokenInCondition = rightParenthesis.previous;
On 2017/06/16 07:43:29, jensj wrote:
> On 2017/06/16 07:39:42, ahe wrote:
> > We want to get rid of "previous". Could you do something like this instead:
> > 
> > Token lastTokenInCondition = leftParenthesis;
> > while (lastTokenInCondition.next != rightParenthesis) {
> >   lastTokenInCondition = lastTokenInCondition.next;
> > }
> > 
> > Or:
> > 
> > Token lastTokenInCondition = rightParenthesis.offset - 1;
> 
> I could. But:
> 1) I suppose an assert can be arbitrarily long so that could potentially be
> slow.
> 2) This could theoretically include a lot of linebreaks and whatnot at the
end,
> when we want to extract the source code between the start and the end. It
might
> not be a big deal through, I'm not sure.
> 
> What do you think?


On second thought, I actually think this would be better:

statement.conditionStartOffset = leftParenthesis.offset + 1;
statement.conditionEndOffset = rightParenthesis.offset - 1;

This way, you also get any comments included, and line breaks could easily be
removed with a call to "trim()" in runtime/lib/errors_patch.dart.

[jensj, 2017-06-16 09:28:25]

Thanks!
Comments addressed. PTAL.

https://codereview.chromium.org/2940283002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2940283002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2662: Token
lastTokenInCondition = rightParenthesis.previous;
On 2017/06/16 07:53:35, ahe wrote:
> On 2017/06/16 07:43:29, jensj wrote:
> > On 2017/06/16 07:39:42, ahe wrote:
> > > We want to get rid of "previous". Could you do something like this
instead:
> > > 
> > > Token lastTokenInCondition = leftParenthesis;
> > > while (lastTokenInCondition.next != rightParenthesis) {
> > >   lastTokenInCondition = lastTokenInCondition.next;
> > > }
> > > 
> > > Or:
> > > 
> > > Token lastTokenInCondition = rightParenthesis.offset - 1;
> > 
> > I could. But:
> > 1) I suppose an assert can be arbitrarily long so that could potentially be
> > slow.
> > 2) This could theoretically include a lot of linebreaks and whatnot at the
> end,
> > when we want to extract the source code between the start and the end. It
> might
> > not be a big deal through, I'm not sure.
> > 
> > What do you think?
> 
> 
> On second thought, I actually think this would be better:
> 
> statement.conditionStartOffset = leftParenthesis.offset + 1;
> statement.conditionEndOffset = rightParenthesis.offset - 1;
> 
> This way, you also get any comments included, and line breaks could easily be
> removed with a call to "trim()" in runtime/lib/errors_patch.dart.

Actually not -1 on the end as that would could off the last char.

[ahe, 2017-06-16 10:18:53]

lgtm

Awesome!

[jensj, 2017-06-16 10:28:24]

Description was changed from

==========
[kernel] Fix assert message.

This mimics the behaviour of the source-based pipeline,
i.e. instead of "manually" calling _AssertionError._create and giving
the correct parameters (wrong parameters, actually), use the helper
method _AssertionError.ThrowNew.

BUG=
==========

to

==========
[kernel] Fix assert message.

This mimics the behaviour of the source-based pipeline,
i.e. instead of "manually" calling _AssertionError._create and giving
the correct parameters (wrong parameters, actually), use the helper
method _AssertionError.ThrowNew.

BUG=
R=ahe@google.com, vegorov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/0a1583aa0480dfc49ee3f589a45bd0c58f4104d9
==========

[jensj, 2017-06-16 10:28:25]

Committed patchset #2 (id:20001) manually as
0a1583aa0480dfc49ee3f589a45bd0c58f4104d9 (presubmit successful).