Error reporting for spans.

Error reporting for spans.

R=brianwilkerson@google.com, scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/0881b53a3bf73e1b7671ea337f05d2dcf9aba80b

[pquitslund, 2015-10-21 23:37:20]

pquitslund@google.com changed reviewers:
+ brianwilkerson@google.com, scheglov@google.com

[scheglov, 2015-10-21 23:47:21]

lgtm

https://codereview.chromium.org/1418973002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/error.dart (right):

https://codereview.chromium.org/1418973002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error.dart:2674: _source, span.start.offset,
span.length, errorCode, arguments));
I'd use reportErrorForOffset() as other reportErrorForX() do.

https://codereview.chromium.org/1418973002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/all_the_rest_test.dart (right):

https://codereview.chromium.org/1418973002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/all_the_rest_test.dart:7316: YamlNode bazNode =
node.nodes['zap'];
I think YAML adds extra complexity here.
Why not create SourceSpan directly and pass it in?

[pquitslund, 2015-10-21 23:56:58]

Thanks!

https://codereview.chromium.org/1418973002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/error.dart (right):

https://codereview.chromium.org/1418973002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error.dart:2674: _source, span.start.offset,
span.length, errorCode, arguments));
On 2015/10/21 23:47:21, scheglov wrote:
> I'd use reportErrorForOffset() as other reportErrorForX() do.

Done.

https://codereview.chromium.org/1418973002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/all_the_rest_test.dart (right):

https://codereview.chromium.org/1418973002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/all_the_rest_test.dart:7316: YamlNode bazNode =
node.nodes['zap'];
On 2015/10/21 23:47:21, scheglov wrote:
> I think YAML adds extra complexity here.
> Why not create SourceSpan directly and pass it in?

Mainly because I want to ensure that the spans that are created by yaml parsing
use offsets that agree with our expectations (e.g., not 1-based).  If I create
the span myself I won't be able to test that.   I guess I'm kind of sneaking
some integration testing in here....  If you have other ideas for where I should
move this, I'm all ears!

[Brian Wilkerson, 2015-10-22 00:13:00]

LGTM

https://codereview.chromium.org/1418973002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/all_the_rest_test.dart (right):

https://codereview.chromium.org/1418973002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/all_the_rest_test.dart:7307: new NonExistingSource(
It's not a problem for this test because you're not actually accessing the
content, but we generally only use NonExistingSource when we care about the
source throwing an exception when we try to access the content. Otherwise we
generally use TestSource.

https://codereview.chromium.org/1418973002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/all_the_rest_test.dart:7316: YamlNode bazNode =
node.nodes['zap'];
Technically, testing whether the yaml parser produces the right offsets should
be in the yaml package :-) If you really don't trust the yaml parser, then let's
create a directory for tests of code from other packages (like
"test/dependencies/yaml" and put them in there.

[pquitslund, 2015-10-22 15:36:59]

On 2015/10/22 00:13:00, Brian Wilkerson wrote:
> LGTM
> 
>
https://codereview.chromium.org/1418973002/diff/1/pkg/analyzer/test/generated...
> File pkg/analyzer/test/generated/all_the_rest_test.dart (right):
> 
>
https://codereview.chromium.org/1418973002/diff/1/pkg/analyzer/test/generated...
> pkg/analyzer/test/generated/all_the_rest_test.dart:7307: new
NonExistingSource(
> It's not a problem for this test because you're not actually accessing the
> content, but we generally only use NonExistingSource when we care about the
> source throwing an exception when we try to access the content. Otherwise we
> generally use TestSource.

Updated.  Thanks for the tip.

> 
>
https://codereview.chromium.org/1418973002/diff/1/pkg/analyzer/test/generated...
> pkg/analyzer/test/generated/all_the_rest_test.dart:7316: YamlNode bazNode =
> node.nodes['zap'];
> Technically, testing whether the yaml parser produces the right offsets should
> be in the yaml package :-) If you really don't trust the yaml parser, then
let's
> create a directory for tests of code from other packages (like
> "test/dependencies/yaml" and put them in there.

That sounds like a good plan.  Having thought about it, I'm less convinced that
we need to test this. (At one point I was using column info for the linter and
that *did* require an adjustment between 0-based and 1-based indexing and that's
I think what got me concerned.)  Anyway, I think an offset is an offset so I'm
good using spans directly.  Down the road we can add test/dependencies as you
suggest if we deem it valuable.

Thanks!

[pquitslund, 2015-10-22 15:37:32]

Committed patchset #3 (id:40001) manually as
0881b53a3bf73e1b7671ea337f05d2dcf9aba80b (presubmit successful).