Implement analysis.updateOptions

Implement analysis.updateOptions

R=paulberry@google.com

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

[Paul Berry, 2014-06-17 20:40:55]

lgtm

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

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:216: void
updateOptions(List<OptionUpdater> optionUpdaters) {
Passing in a list of closures seems like overkill to me.  Why not just maintain
a single AnalysisOptionsImpl object with the current set of options, and when we
receive an analysis.updateOptions request, update it and then propagate it to
each analysisContext?

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:217: // TODO(brianwilkerson)
Figure out how to update the defaults for newly
You should be able to take care of this in
AnalysisServerContextDirectoryManager.addContext(), after constructing the
ContextDirectory (which creates the context as a side effect) and before the
call to analysisServer.schedulePerformAnalysisOperation().

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

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:545: : this(request.id, new
RequestError(-12, 'Unknown analysis option: "$optionName"'));
I have a vague memory that we decided to simply ignore unrecognized options. 
Can't remember why, though.

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/test/dom...
File pkg/analysis_server/test/domain_analysis_test.dart (right):

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/test/dom...
pkg/analysis_server/test/domain_analysis_test.dart:135: // TODO(brianwilkerson)
Figure out how to test that the options were
Personally I'd prefer to see this addressed before the CL gets committed.

[Brian Wilkerson, 2014-06-17 21:30:12]

PTAL

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

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:216: void
updateOptions(List<OptionUpdater> optionUpdaters) {
> Passing in a list of closures seems like overkill to me.  Why not just
maintain
> a single AnalysisOptionsImpl object with the current set of options, and when
we
> receive an analysis.updateOptions request, update it and then propagate it to
> each analysisContext?

That was my first thought as well. Unfortunately, AnalysisOptionsImpl doesn't
allow for specifying a partial set of options, so I couldn't tell whether an
option was implicitly or explicitly set. That meant that every option that
wasn't suppose to be updated was reset to factory defaults.

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:217: // TODO(brianwilkerson)
Figure out how to update the defaults for newly
> You should be able to take care of this in
> AnalysisServerContextDirectoryManager.addContext()...

Thanks. Done, but not tested. This solved one testing question and raised one
more...

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

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:545: : this(request.id, new
RequestError(-12, 'Unknown analysis option: "$optionName"'));
> I have a vague memory that we decided to simply ignore unrecognized options. 
> Can't remember why, though.

We thought it might help with version skew issues, but I think it might be
better to let the client know if they're using a version of the server that
doesn't support an option they're depending on. Let me know if you disagree.

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/test/dom...
File pkg/analysis_server/test/domain_analysis_test.dart (right):

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/test/dom...
pkg/analysis_server/test/domain_analysis_test.dart:135: // TODO(brianwilkerson)
Figure out how to test that the options were
> Personally I'd prefer to see this addressed before the CL gets committed.

And yet you gave it an LGTM...

I added tests.

[Paul Berry, 2014-06-17 22:24:57]

lgtm

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

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:216: void
updateOptions(List<OptionUpdater> optionUpdaters) {
On 2014/06/17 21:30:12, Brian Wilkerson wrote:
> > Passing in a list of closures seems like overkill to me.  Why not just
> maintain
> > a single AnalysisOptionsImpl object with the current set of options, and
when
> we
> > receive an analysis.updateOptions request, update it and then propagate it
to
> > each analysisContext?
> 
> That was my first thought as well. Unfortunately, AnalysisOptionsImpl doesn't
> allow for specifying a partial set of options, so I couldn't tell whether an
> option was implicitly or explicitly set. That meant that every option that
> wasn't suppose to be updated was reset to factory defaults.

Actually I was thinking of something along these lines:

AnalysisServer:
- add field "AnalysisOptionsImpl currentOptions;"
- add method "updateOptions()", which does "context.analysisOptions =
currentOptions;" for all contexts.

AnalysisServerContextDirectoryManager.addContext(...):
- Populate the new context's options using analysisServer.currentOptions

AnalysisDomainHandler.updateOptions(Request request):
- Modify server.currentOptions in place based on request
- Call server.updateOptions()

So there would be no need to tell which options were implicitly or explicitly
set--AnalysisDomainHandler.updateOptions() simply modifies those options that
are mentioned in the request and leaves the rest alone.

Your call, though.  The code as is looks like it implements the spec correctly.

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

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:545: : this(request.id, new
RequestError(-12, 'Unknown analysis option: "$optionName"'));
On 2014/06/17 21:30:12, Brian Wilkerson wrote:
> > I have a vague memory that we decided to simply ignore unrecognized options.

> > Can't remember why, though.
> 
> We thought it might help with version skew issues, but I think it might be
> better to let the client know if they're using a version of the server that
> doesn't support an option they're depending on. Let me know if you disagree.

Now that I think about it again, I don't really have a strong opinion.

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/test/dom...
File pkg/analysis_server/test/domain_analysis_test.dart (right):

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/test/dom...
pkg/analysis_server/test/domain_analysis_test.dart:135: // TODO(brianwilkerson)
Figure out how to test that the options were
On 2014/06/17 21:30:12, Brian Wilkerson wrote:
> > Personally I'd prefer to see this addressed before the CL gets committed.
> 
> And yet you gave it an LGTM...

I assumed that LGTM + comments meant "there's some stuff that I would have done
differently, but I'm not going to be a stickler about it--it's your call whether
to take my advice".  Or maybe I was just being unintentionally vague :)

> 
> I added tests.

They look good, thanks.

You might want to consider adding a test to confirm that if options are supplied
before a context is created, they are correctly applied to the newly created
context.  Your call :)

[Brian Wilkerson, 2014-06-18 17:01:30]

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

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:216: void
updateOptions(List<OptionUpdater> optionUpdaters) {
> Actually I was thinking of something along these lines:
> 
> AnalysisServer:
> - add field "AnalysisOptionsImpl currentOptions;"
> - add method "updateOptions()", which does "context.analysisOptions =
> currentOptions;" for all contexts.

Unfortunately, every context needs its own separate set of options so that we
can set different cache sizes, so we can't just replace all options.

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/test/dom...
File pkg/analysis_server/test/domain_analysis_test.dart (right):

https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/test/dom...
pkg/analysis_server/test/domain_analysis_test.dart:135: // TODO(brianwilkerson)
Figure out how to test that the options were
> You might want to consider adding a test to confirm that if options are
supplied
> before a context is created, they are correctly applied to the newly created
> context.  Your call :)

That would be good, but I don't know how to find the analysis context that owns
the options that need to be tested. I suppose we can expose that information for
testing purposes, but that goes against my training, so I'm hesitant.

[Paul Berry, 2014-06-18 17:07:09]

On 2014/06/18 17:01:30, Brian Wilkerson wrote:
>
https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/lib/src/...
> File pkg/analysis_server/lib/src/analysis_server.dart (right):
> 
>
https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/lib/src/...
> pkg/analysis_server/lib/src/analysis_server.dart:216: void
> updateOptions(List<OptionUpdater> optionUpdaters) {
> > Actually I was thinking of something along these lines:
> > 
> > AnalysisServer:
> > - add field "AnalysisOptionsImpl currentOptions;"
> > - add method "updateOptions()", which does "context.analysisOptions =
> > currentOptions;" for all contexts.
> 
> Unfortunately, every context needs its own separate set of options so that we
> can set different cache sizes, so we can't just replace all options.
> 
>
https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/test/dom...
> File pkg/analysis_server/test/domain_analysis_test.dart (right):
> 
>
https://codereview.chromium.org/335123008/diff/1/pkg/analysis_server/test/dom...
> pkg/analysis_server/test/domain_analysis_test.dart:135: //
TODO(brianwilkerson)
> Figure out how to test that the options were
> > You might want to consider adding a test to confirm that if options are
> supplied
> > before a context is created, they are correctly applied to the newly created
> > context.  Your call :)
> 
> That would be good, but I don't know how to find the analysis context that
owns
> the options that need to be tested. I suppose we can expose that information
for
> testing purposes, but that goes against my training, so I'm hesitant.

Fair enough.

[Brian Wilkerson, 2014-06-18 21:27:57]

Committed patchset #2 manually as r37467 (presubmit successful).