Experimental `getDiagnostics` request (#24480).

Experimental `getDiagnostics` request (#24480).

* Adds `getDiagnostics` request with *very* provisional diagnostics payload (primary goal here is to plumb this fully through and iterate).
* Updates codegenerator to skip doc generation for experimental APIs.

Additional context here: https://github.com/dart-lang/sdk/issues/24480.

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

Committed: https://github.com/dart-lang/sdk/commit/82e83bb34d913c1ba171d174e66e60b95bea492a

[pquitslund, 2015-11-05 21:02:28]

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

[scheglov, 2015-11-05 21:22:22]

lgtm

https://codereview.chromium.org/1416093007/diff/1/pkg/analysis_server/tool/sp...
File pkg/analysis_server/tool/spec/from_html.dart (right):

https://codereview.chromium.org/1416093007/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/from_html.dart:529: print('exp');
Debug code?

https://codereview.chromium.org/1416093007/diff/1/pkg/analysis_server/tool/sp...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/1416093007/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2489: The number of work items in
the queue.
It seems to me that it's not very useful metrics.
It's just a number of steps for the current work order.
And it will be changing quickly, but reaching zero does not mean much.
There might be unknown number of work orders to be scheduled later.

https://codereview.chromium.org/1416093007/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2495: Exceptions associated with
cache entries.
If any of these exceptions are going to be used to open issues against analyzer,
I'd like to know the entry target.

[pquitslund, 2015-11-05 22:28:54]

Thanks!

https://codereview.chromium.org/1416093007/diff/1/pkg/analysis_server/tool/sp...
File pkg/analysis_server/tool/spec/from_html.dart (right):

https://codereview.chromium.org/1416093007/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/from_html.dart:529: print('exp');
On 2015/11/05 21:22:22, scheglov wrote:
> Debug code?

Done.

https://codereview.chromium.org/1416093007/diff/1/pkg/analysis_server/tool/sp...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/1416093007/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2489: The number of work items in
the queue.
On 2015/11/05 21:22:22, scheglov wrote:
> It seems to me that it's not very useful metrics.
> It's just a number of steps for the current work order.
> And it will be changing quickly, but reaching zero does not mean much.
> There might be unknown number of work orders to be scheduled later.

Yeah.  I'm not sure.  The idea was that maybe if over time we see that there are
always items queued maybe that would indicate something?

Definitely something to experiment with.

Very open to other ideas!

https://codereview.chromium.org/1416093007/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2495: Exceptions associated with
cache entries.
On 2015/11/05 21:22:22, scheglov wrote:
> If any of these exceptions are going to be used to open issues against
analyzer,
> I'd like to know the entry target.

At this point that's not the intention though it may down the road.  In short
order this is just to give us an indication of health.

If it turns out that Devon wants to do something interesting with these
exceptions, you're right that we should add at least the target for sure.

[Brian Wilkerson, 2015-11-05 22:43:44]

LGTM

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/domain_experimental.dart (right):

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/domain_experimental.dart:51:
exceptions.add(cacheEntry.exception.toString());
Each exception will typically be recorded in multiple cache entries. You
probably want to only add exceptions that you haven't already seen (you can use
identical() for the test).

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/tes...
File pkg/analysis_server/test/domain_experimental_test.dart (right):

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/domain_experimental_test.dart:46:
test('getDiagnostics', () async {
You should also test the case where no analysis roots have been set.

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
File pkg/analysis_server/tool/spec/generated/java/types/ContextInfo.java
(right):

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/generated/java/types/ContextInfo.java:2: *
Copyright (c) 2014, the Dart project authors.
Not for you to solve, but it seems odd to generate a retroactive copyright.

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/spec_input.html:2463: <type name="ContextInfo"
experimental="true">
Given that there's already a class named ContextInfo, it might be better to
rename this. One suggestion would be ContextData.

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/spec_input.html:2474: <field
name="explicitlyAnalyzedFiles">
Maybe explicitFileCount and implicitFileCount? (The current names sound to me
like lists of file paths.)

[pquitslund, 2015-11-05 22:58:11]

Thank you!

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/lib...
File pkg/analysis_server/lib/src/domain_experimental.dart (right):

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/domain_experimental.dart:51:
exceptions.add(cacheEntry.exception.toString());
On 2015/11/05 22:43:43, Brian Wilkerson wrote:
> Each exception will typically be recorded in multiple cache entries. You
> probably want to only add exceptions that you haven't already seen (you can
use
> identical() for the test).

Good catch.  Done.

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/tes...
File pkg/analysis_server/test/domain_experimental_test.dart (right):

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/tes...
pkg/analysis_server/test/domain_experimental_test.dart:46:
test('getDiagnostics', () async {
On 2015/11/05 22:43:43, Brian Wilkerson wrote:
> You should also test the case where no analysis roots have been set.

Done.

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
File pkg/analysis_server/tool/spec/generated/java/types/ContextInfo.java
(right):

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/generated/java/types/ContextInfo.java:2: *
Copyright (c) 2014, the Dart project authors.
On 2015/11/05 22:43:43, Brian Wilkerson wrote:
> Not for you to solve, but it seems odd to generate a retroactive copyright.

Ah, but I couldn't resist.  :)

Fixed.

Asides:

1. I wonder if EPL is still what we want here?
2. Brace for a bunch of micro changes!

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/spec_input.html:2463: <type name="ContextInfo"
experimental="true">
On 2015/11/05 22:43:44, Brian Wilkerson wrote:
> Given that there's already a class named ContextInfo, it might be better to
> rename this. One suggestion would be ContextData.

Done.

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/spec_input.html:2474: <field
name="explicitlyAnalyzedFiles">
On 2015/11/05 22:43:44, Brian Wilkerson wrote:
> Maybe explicitFileCount and implicitFileCount? (The current names sound to me
> like lists of file paths.)

Done.

[devoncarew, 2015-11-05 23:03:59]

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

[devoncarew, 2015-11-05 23:04:00]

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/spec_input.html:1962: <domain name="experimental"
experimental="true">
experimental experimental=true seems redundant. I'm hoping this API or something
like it graduates to stable at some point so we can consume it w/o guarding
against it not existing.

Perhaps move the API to a 'diagnostics' domain? Assuming we think there will be
enough content to justify its own domain.

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/spec_input.html:1971: <request
method="getDiagnostics">
I assume experimental=true is implied here, based on the parent domain? I wonder
if there's value in declaring it here as well, for in case the parent is
refactored.

[pquitslund, 2015-11-05 23:04:48]

On 2015/11/05 22:58:11, pquitslund wrote:
> Thank you!
> 
>
https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/lib...
> File pkg/analysis_server/lib/src/domain_experimental.dart (right):
> 
>
https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/lib...
> pkg/analysis_server/lib/src/domain_experimental.dart:51:
> exceptions.add(cacheEntry.exception.toString());
> On 2015/11/05 22:43:43, Brian Wilkerson wrote:
> > Each exception will typically be recorded in multiple cache entries. You
> > probably want to only add exceptions that you haven't already seen (you can
> use
> > identical() for the test).
> 
> Good catch.  Done.
> 
>
https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/tes...
> File pkg/analysis_server/test/domain_experimental_test.dart (right):
> 
>
https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/tes...
> pkg/analysis_server/test/domain_experimental_test.dart:46:
> test('getDiagnostics', () async {
> On 2015/11/05 22:43:43, Brian Wilkerson wrote:
> > You should also test the case where no analysis roots have been set.
> 
> Done.
> 
>
https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
> File pkg/analysis_server/tool/spec/generated/java/types/ContextInfo.java
> (right):
> 
>
https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
> pkg/analysis_server/tool/spec/generated/java/types/ContextInfo.java:2: *
> Copyright (c) 2014, the Dart project authors.
> On 2015/11/05 22:43:43, Brian Wilkerson wrote:
> > Not for you to solve, but it seems odd to generate a retroactive copyright.
> 
> Ah, but I couldn't resist.  :)
> 
> Fixed.
> 
> Asides:
> 
> 1. I wonder if EPL is still what we want here?
> 2. Brace for a bunch of micro changes!

Yikes.  Even more than I expected.  Well, the band-aid is off!

[pquitslund, 2015-11-05 23:05:25]

Committed patchset #3 (id:40001) manually as
82e83bb34d913c1ba171d174e66e60b95bea492a (presubmit successful).

[pquitslund, 2015-11-05 23:16:49]

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/spec_input.html:1962: <domain name="experimental"
experimental="true">
On 2015/11/05 23:04:00, devoncarew wrote:
> experimental experimental=true seems redundant. I'm hoping this API or
something
> like it graduates to stable at some point so we can consume it w/o guarding
> against it not existing.

Absolutely.  When we're done iterating, we should talk about promoting.

> Perhaps move the API to a 'diagnostics' domain? Assuming we think there will
be
> enough content to justify its own domain.

Let's see how this goes.  Totally open to that.

https://codereview.chromium.org/1416093007/diff/20001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/spec_input.html:1971: <request
method="getDiagnostics">
On 2015/11/05 23:04:00, devoncarew wrote:
> I assume experimental=true is implied here, based on the parent domain? I
wonder
> if there's value in declaring it here as well, for in case the parent is
> refactored.

Right.  Experimentalness passes down.