AnalysisError `hasFix` attr (Implements #23874).

AnalysisError `hasFix` attr (Implements #23874).

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

Committed: https://github.com/dart-lang/sdk/commit/8697392367be57f9f595abca0cd106c2f3e56f3e

[pquitslund, 2015-10-02 00:06:44]

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

[Brian Wilkerson, 2015-10-02 00:17:48]

The changes to the spec look fine, but I'd like to see some timing data to see
what the performance impact will be before we change the spec and then
potentially have to change it back.

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

https://codereview.chromium.org/1385523002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:9: <h1
style="color:#999999">Version <version>1.10.0</version></h1>
We need to bump the version number to 1.11.0, both here and in
AnalysisServer.VERSION.

https://codereview.chromium.org/1385523002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2034: an associated fix (or
fixes).
This should make explicit that the absence of this field implies a value of
'false' (assuming that's true).

[scheglov, 2015-10-02 00:23:31]

LGTM

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

https://codereview.chromium.org/1385523002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:9: <h1
style="color:#999999">Version <version>1.10.0</version></h1>
On 2015/10/02 00:17:48, Brian Wilkerson wrote:
> We need to bump the version number to 1.11.0, both here and in
> AnalysisServer.VERSION.

Note, that I forgot to change AnalysisServer.VERSION, so it's 1.9 now!
Change it to '1.11.0', not just increment :-)

https://codereview.chromium.org/1385523002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2034: an associated fix (or
fixes).
On 2015/10/02 00:17:48, Brian Wilkerson wrote:
> This should make explicit that the absence of this field implies a value of
> 'false' (assuming that's true).

I might worth to document that this flag may be "false negative", i.e. one or
more fixes may exist, but the flag is still "false" or missing.

[devoncarew, 2015-10-02 03:44:18]

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

[devoncarew, 2015-10-02 03:44:19]

Phil, thanks for the quick turn-around!

> but I'd like to see some timing data to see what the performance impact will
be

The implementation is a switch statement, which is about as performant as you
can get.

[pquitslund, 2015-10-02 04:18:06]

On 2015/10/02 03:44:19, devoncarew wrote:
> Phil, thanks for the quick turn-around!
> 
> > but I'd like to see some timing data to see what the performance impact will
> be
> 
> The implementation is a switch statement, which is about as performant as you
> can get.

That was my thinking but I'm more than happy to do some benchmarking.  I'd be
shocked if the cost was not negligible.  Having said that, if you're worried
about future tests being more impactful, we could add a flag/option to server to
make this calculation opt-in.  Your call.

[pquitslund, 2015-10-02 04:19:53]

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

https://codereview.chromium.org/1385523002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:9: <h1
style="color:#999999">Version <version>1.10.0</version></h1>
On 2015/10/02 00:23:31, scheglov wrote:
> On 2015/10/02 00:17:48, Brian Wilkerson wrote:
> > We need to bump the version number to 1.11.0, both here and in
> > AnalysisServer.VERSION.

Good catch!

> Note, that I forgot to change AnalysisServer.VERSION, so it's 1.9 now!
> Change it to '1.11.0', not just increment :-)

:)  Done!

https://codereview.chromium.org/1385523002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2034: an associated fix (or
fixes).
On 2015/10/02 00:17:48, Brian Wilkerson wrote:
> This should make explicit that the absence of this field implies a value of
> 'false' (assuming that's true).

Done.

[Brian Wilkerson, 2015-10-02 13:58:58]

> > The implementation is a switch statement, which is about as performant as
you
> > can get.
> 
> That was my thinking but I'm more than happy to do some benchmarking.  I'd be
> shocked if the cost was not negligible.  Having said that, if you're worried
> about future tests being more impactful, we could add a flag/option to server
to
> make this calculation opt-in.  Your call.

The dumbest implementation, with the most false results, might be a switch
statement (and no, the last time I benchmarked 'if' statements were actually
faster than switch statements), but there *has* to be a limit to the number of
false results we're willing to accept. (What is an acceptable false negative
rate for this feature? If there is no limit, then it's easy to control the
performance cost.)

Given that some errors only have fixes in some contexts, I want to know what the
cost is of doing a good enough job that it doesn't cause UX problems.

And yes, I am concerned that we will have more context sensitive fixes in the
future, or worse yet that we won't add such fixes because of the cost of
supporting this feature, but I *really* don't want to add a flag if we can avoid
it.

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

https://codereview.chromium.org/1385523002/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:2034: an associated fix (or
fixes).
> I might worth to document that this flag may be "false negative", i.e.
> one or more fixes may exist, but the flag is still "false" or missing.

Also, we should document whether or not we will ever produce a false positive
(saying that a fix exists when it doesn't). I think we should disallow that
case.

[pquitslund, 2015-10-02 15:18:35]

> Given that some errors only have fixes in some contexts, I want to know what
the
> cost is of doing a good enough job that it doesn't cause UX problems.

+1.  The good news is that this is a UX-driven request and we have a very
engaged customer that will be quick to let us know if we're missing the mark. :)

> And yes, I am concerned that we will have more context sensitive fixes in the
> future, or worse yet that we won't add such fixes because of the cost of
> supporting this feature, 

That's a legitimate concern but I'd hate to gate this behind a hypothetical.

> but I *really* don't want to add a flag if we can avoid
> it.

Fair enough.  Is there a middle ground?  How about adding a way to opt-in when
clients subscribe to error notifications?

> 
>
https://codereview.chromium.org/1385523002/diff/1/pkg/analysis_server/tool/sp...
> File pkg/analysis_server/tool/spec/spec_input.html (right):
> 
>
https://codereview.chromium.org/1385523002/diff/1/pkg/analysis_server/tool/sp...
> pkg/analysis_server/tool/spec/spec_input.html:2034: an associated fix (or
> fixes).
> > I might worth to document that this flag may be "false negative", i.e.
> > one or more fixes may exist, but the flag is still "false" or missing.
> 
> Also, we should document whether or not we will ever produce a false positive
> (saying that a fix exists when it doesn't). I think we should disallow that
> case.

Sure.  I think disallowing false positives is reasonable.  I'll update.

[Brian Wilkerson, 2015-10-02 15:23:05]

>  Is there a middle ground?  How about adding a way to opt-in when
> clients subscribe to error notifications?

I'd really like to get some concrete data to know whether or not there is a
problem before we (a) change the API to something we might not be able to
support or (b) try to solve a problem that might not exist.

[pquitslund, 2015-10-02 15:30:34]

On 2015/10/02 15:23:05, Brian Wilkerson wrote:
> >  Is there a middle ground?  How about adding a way to opt-in when
> > clients subscribe to error notifications?
> 
> I'd really like to get some concrete data to know whether or not there is a
> problem before we (a) change the API to something we might not be able to
> support or (b) try to solve a problem that might not exist.

What concrete data?  Performance of switch vs if/else? Number of
false-negatives?

FWIW: the proposed middle ground is to satisfy the vague worry of expensive
checks that may arrive in the *future*.  No way for me to deliver data on those!
;)

As for concrete data on UX (and specifically the threshold for false negatives),
I'd like to do this experimentally and get customer feedback.  For that to
happen we need to plumb this feature through.

[pquitslund, 2015-10-02 20:25:17]

OK. So I did some micro-benchmarking over lunch. When run, warmed up, over
100,000 error creations, I'm seeing an impact of something like 0.02
microseconds per error creation.  IOW: on the order of machine instructions.

[pquitslund, 2015-10-02 20:26:55]

On 2015/10/02 20:25:17, pquitslund wrote:
> OK. So I did some micro-benchmarking over lunch. When run, warmed up, over
> 100,000 error creations, I'm seeing an impact of something like 0.02
> microseconds per error creation.  IOW: on the order of machine instructions.

Oh, and FWIW that's the WORST case (where the error has no fix).  Also note that
I did in fact change the switch to an "if" but that's mainly because the code I
had the first time failed at runtime. :)

[Brian Wilkerson, 2015-10-02 22:00:49]

Thanks for doing the performance measurements! LGTM

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

https://codereview.chromium.org/1385523002/diff/60001/pkg/analysis_server/too...
pkg/analysis_server/tool/spec/spec_input.html:2042: fact associated hints.
"hints" --> "fixes"

[pquitslund, 2015-10-05 16:44:21]

On 2015/10/02 22:00:49, Brian Wilkerson wrote:
> Thanks for doing the performance measurements! LGTM

No problem.  It'll be great to have more of this in place (in a proper framework
even).

> 
>
https://codereview.chromium.org/1385523002/diff/60001/pkg/analysis_server/too...
> File pkg/analysis_server/tool/spec/spec_input.html (right):
> 
>
https://codereview.chromium.org/1385523002/diff/60001/pkg/analysis_server/too...
> pkg/analysis_server/tool/spec/spec_input.html:2042: fact associated hints.
> "hints" --> "fixes"

Good catch.  Done

[pquitslund, 2015-10-05 16:46:17]

Committed patchset #6 (id:100001) manually as
8697392367be57f9f595abca0cd106c2f3e56f3e (presubmit successful).