Add support for fixes

Add support for fixes

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

Committed: https://github.com/dart-lang/sdk/commit/91b8b495b3adb1d5508d92d60990e1ca6e91f1a5

[Brian Wilkerson, 2017-06-06 22:17:05]

brianwilkerson@google.com changed reviewers:
+ maxkim@google.com, mfairhurst@google.com, scheglov@google.com

[Brian Wilkerson, 2017-06-06 22:17:06]

I'm not totally happy with this, but thought it was far enough along that we
could start a discussion.

In particular, I don't like the fact that the generator writes the error into
the request object. The error isn't provided by the wire protocol, but the
contributors currently (in the server implementation) take one error at a time.
We could pass in the whole list of error and make the contributors iterate over
them, but that means every contributor has to write the iteration code.

Other than that, I think it's fairly close. Let me know what you think.

[maxkim, 2017-06-07 21:15:25]

> In particular, I don't like the fact that the generator writes the error into
> the request object. The error isn't provided by the wire protocol, but the
> contributors currently (in the server implementation) take one error at a
time.

Are there alternatives to inserting errors directly into the request? I think
this is fine.

> We could pass in the whole list of error and make the contributors iterate
over
> them, but that means every contributor has to write the iteration code.

One alternative is to let the contributors hold a static list/set of Error-codes
that it is interested in and have a uniform function that every collector can
use so that every contributor doesn't have to write the iteration code, but can
later choose to override it if they need to - something like
'Iterable<AnalysisError> _filterErrors(Iterable<AnalysisError> errors)' within
FixCollectorImpl.

[maxkim, 2017-06-07 21:21:55]

I'm also curious as to the need of the 'Collector' model. It's nice since it's
following a similar paradigm as the other plugin features (ex: Autocompletion),
but having 'Contributors' doesn't make as much sense in the context of Fixes. 

Instead of 'Contributor', we could potentially call it something like 'Policy'
that exists for every error. Within Generator, we can create a ErrorCode to
Policy map that allow for quick look up of the Policy that handles a specific
error. 

Just a thought.

[scheglov, 2017-06-07 21:32:53]

LGTM

[Brian Wilkerson, 2017-06-07 21:37:18]

> I'm also curious as to the need of the 'Collector' model.

The primary motivation for the Request / Collector model is that it allows us to
extend the number of inputs and outputs to the collectors without it being a
breaking change.

> It's nice since it's following a similar paradigm as the other plugin
> features (ex: Autocompletion), but having 'Contributors' doesn't
> make as much sense in the context of Fixes.

Do you mean "having multiple contributors" or "having any contributor"? If you
mean the former, then I agree that it's unlikely that plugin authors will create
multiple fix contributors. We could limit it to a single contributor, but I'd
still want a collector for the reason given above (to protect us from breaking
changes), so the limitation wouldn't gain us anything and would introduce an
inconsistency in the API.

> Instead of 'Contributor', we could potentially call it something like
> 'Policy' that exists for every error. Within Generator, we can create
> a ErrorCode to Policy map that allow for quick look up of the Policy
> that handles a specific error.

We could, but I expect that the different policies would end up having enough
overlap that they'd usually just be collapsed into a single class. Take a look
at FixProcessor. Many errors have more than one fix associated with them, and
many fixes are generated for more than one error. I'm not sure it would be
feasible to break FixProcessor up into multiple policy objects.

[maxkim, 2017-06-07 21:45:50]

> The primary motivation for the Request / Collector model is that it allows us
to
> extend the number of inputs and outputs to the collectors without it being a
> breaking change.

Ok - that makes sense. 

> Do you mean "having multiple contributors" or "having any contributor"? If you
> mean the former, then I agree that it's unlikely that plugin authors will
create
> multiple fix contributors. We could limit it to a single contributor, but I'd
> still want a collector for the reason given above (to protect us from breaking
> changes), so the limitation wouldn't gain us anything and would introduce an
> inconsistency in the API.

I meant the former, and I agree with the precautionary approach.

> We could, but I expect that the different policies would end up having enough
> overlap that they'd usually just be collapsed into a single class. Take a look
> at FixProcessor. Many errors have more than one fix associated with them, and
> many fixes are generated for more than one error. I'm not sure it would be
> feasible to break FixProcessor up into multiple policy objects.

If that's the case, then adding 'Policy' types might just overcomplicate it
then. 

LGTM

[Brian Wilkerson, 2017-06-08 14:53:08]

Description was changed from

==========
Add support for fixes
==========

to

==========
Add support for fixes

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

Committed:
https://github.com/dart-lang/sdk/commit/91b8b495b3adb1d5508d92d60990e1ca6e91f1a5
==========

[Brian Wilkerson, 2017-06-08 14:53:09]

Committed patchset #1 (id:1) manually as
91b8b495b3adb1d5508d92d60990e1ca6e91f1a5 (presubmit successful).