Implementation of the 'edit.getFixes' API in server.

Implementation of the 'edit.getFixes' API in server.

R=paulberry@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=38473

[Paul Berry, 2014-07-22 20:22:06]

This CL includes a huge amount of code motion, which makes it really hard to
review.  Maybe split it up into separate CL's for the code motion and for the
implementation of edit.getFixes?

Also, it looks like you've also implemented analysis.getErrors--at a minimum the
commit message should mention that.  However, I'm not convinced that the
implementation of getErrors is correct (see my inline comments).  Maybe it would
be a good idea to split that out to its own CL as well so we can discuss it
separately.

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:566: return null;
Does this mean that the result will be {..., "result": {"errors": null}} in the
case where there was no analysis context?  That's not allowed according to the
spec; probably we should report an error in this condition.

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:570: return
context.getErrors(source);
Judging from the implementation in AnalysisContextImpl, it looks like this just
gets the currently computed set of errors; it doesn't wait for the errors to be
computed.  According to the spec (and the IntelliJ requirements that motivated
this feature) the server needs to wait until errors have been fully computed
before a response is returned.

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/edit/edit_domain.dart (right):

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/edit/edit_domain.dart:112: return new
Response(request.id)..setResult(FIXES, errorFixesList);
'..' looks weird here.  Just use '.'.

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/edit/edit_domain.dart:151: return null;
Shouldn't we generate an error in the case of an unrecognized request?

[scheglov, 2014-07-22 20:31:31]

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:566: return null;
On 2014/07/22 20:22:06, Paul Berry wrote:
> Does this mean that the result will be {..., "result": {"errors": null}} in
the
> case where there was no analysis context?  That's not allowed according to the
> spec; probably we should report an error in this condition.

It is OK for "edit.getFixes" though.

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:570: return
context.getErrors(source);
On 2014/07/22 20:22:06, Paul Berry wrote:
> Judging from the implementation in AnalysisContextImpl, it looks like this
just
> gets the currently computed set of errors; it doesn't wait for the errors to
be
> computed.  According to the spec (and the IntelliJ requirements that motivated
> this feature) the server needs to wait until errors have been fully computed
> before a response is returned.

It's not an implementation of the "analysis.getErrors" API.
This methods returns just the currently computed set of errors, as needed for
"edit.getFixes".

An implementation of "analysis.getErrors" is in my queue, blocked by this
AnalysisError value object change.

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/edit/edit_domain.dart (right):

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/edit/edit_domain.dart:112: return new
Response(request.id)..setResult(FIXES, errorFixesList);
On 2014/07/22 20:22:06, Paul Berry wrote:
> '..' looks weird here.  Just use '.'.

We use ".." to set a result and still return the Response instance.

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/edit/edit_domain.dart:151: return null;
On 2014/07/22 20:22:06, Paul Berry wrote:
> Shouldn't we generate an error in the case of an unrecognized request?

No, we shouldn't.
It is OK for the Domain to return null as a signal that it does not know how to
handle the given request, so another domain(s) should be asked.

[Paul Berry, 2014-07-22 20:41:43]

lgtm

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:566: return null;
On 2014/07/22 20:31:31, scheglov wrote:
> On 2014/07/22 20:22:06, Paul Berry wrote:
> > Does this mean that the result will be {..., "result": {"errors": null}} in
> the
> > case where there was no analysis context?  That's not allowed according to
the
> > spec; probably we should report an error in this condition.
> 
> It is OK for "edit.getFixes" though.

Oh, ok.  I was confused because the method name sounds like it corresponds to
the "analysis.getErrors" request.  Can you include a comment above the function
to clarify that it's intended for use by "getFixes", and hence doesn't have to
wait for errors to be computed?

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/edit/edit_domain.dart (right):

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/edit/edit_domain.dart:112: return new
Response(request.id)..setResult(FIXES, errorFixesList);
On 2014/07/22 20:31:31, scheglov wrote:
> On 2014/07/22 20:22:06, Paul Berry wrote:
> > '..' looks weird here.  Just use '.'.
> 
> We use ".." to set a result and still return the Response instance.

Ah, ok.  I understand now.

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/edit/edit_domain.dart:151: return null;
On 2014/07/22 20:31:31, scheglov wrote:
> On 2014/07/22 20:22:06, Paul Berry wrote:
> > Shouldn't we generate an error in the case of an unrecognized request?
> 
> No, we shouldn't.
> It is OK for the Domain to return null as a signal that it does not know how
to
> handle the given request, so another domain(s) should be asked.

Ok.

[scheglov, 2014-07-22 20:46:04]

Committed patchset #1 manually as r38473 (presubmit successful).

[scheglov, 2014-07-22 20:53:09]

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/402333006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:566: return null;
On 2014/07/22 20:41:43, Paul Berry wrote:
> On 2014/07/22 20:31:31, scheglov wrote:
> > On 2014/07/22 20:22:06, Paul Berry wrote:
> > > Does this mean that the result will be {..., "result": {"errors": null}}
in
> > the
> > > case where there was no analysis context?  That's not allowed according to
> the
> > > spec; probably we should report an error in this condition.
> > 
> > It is OK for "edit.getFixes" though.
> 
> Oh, ok.  I was confused because the method name sounds like it corresponds to
> the "analysis.getErrors" request.  Can you include a comment above the
function
> to clarify that it's intended for use by "getFixes", and hence doesn't have to
> wait for errors to be computed?

Done.