Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2918613002:
Add completion support for plugins (Closed)
|
Created:
3 years, 6 months ago by Brian Wilkerson Modified:
3 years, 6 months ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd completion support for plugins
R=maxkim@google.com, scheglov@google.com
Committed: https://github.com/dart-lang/sdk/commit/12398ad9172005559d7beb4bf14dae000eb18833
Patch Set 1 #
Total comments: 24
Messages
Total messages: 16 (2 generated)
brianwilkerson@google.com changed reviewers: + danrubel@google.com, maxkim@google.com, mfairhurst@google.com, scheglov@google.com
This isn't complete, but it's a fairly good sketch of where I think the completion support should go. So mostly at this point I'm interested in design feedback. It doesn't include any way for plugins to report completion performance information. We might need to enhance the wire protocol to support that. It also doesn't make the IDE options available to plugins. I'm not sure yet whether any plugins care. It also doesn't have everything that the angular plugin will need, but the missing pieces can probably be added in a separate CL. In particular it's missing ReplacementRange, TypeMemberContributor, InheritedReferenceContributor, and the relevance constants. Some of these can be moved to analyzer_plugin as is, but the contributors might have to be copied for a while until the missing support that server needs can be added. In terms of design, I've made a couple of changes from the existing support in server. First, contributors take a "collector" and have no return value rather than returning a list. This allows collectors to return more than just a list, including the replacement offset and length (so they don't need to modify the request object). Second, the request object's API is much smaller, especially compared to DartCompletionRequest. I'm happy to widen the API over time (and it won't be a breaking change to do so), but I want to start out with a minimal API until we prove we need the additional support. I think it's currently enough for the angular completer, though I might have missed something. (If this looks good, I'd also like to go back and change the navigation support so that contributors there also take a request / collector pair. They already have a collector, but not a request.)
On 2017/05/31 16:37:27, Brian Wilkerson wrote: > It also doesn't have everything that the angular plugin will need, but the > missing pieces can probably be added in a separate CL. In particular it's > missing ReplacementRange, TypeMemberContributor, InheritedReferenceContributor, > and the relevance constants. Some of these can be moved to analyzer_plugin as > is, but the contributors might have to be copied for a while until the missing > support that server needs can be added. At least for the angular plugin, the only thing we really need are request offset, request source file, and the performance object (we simply create our own right now that is not used). I could see other plugins taking all of the above as being useful. > In terms of design, I've made a couple of changes from the existing support in > server. First, contributors take a "collector" and have no return value rather > than returning a list. This allows collectors to return more than just a list, > including the replacement offset and length (so they don't need to modify the > request object). Not having to modify the request object sounds like a great strategy. Seemed odd previously that we would have to overload the request object. > Second, the request object's API is much smaller, especially compared to > DartCompletionRequest. I'm happy to widen the API over time (and it won't be a > breaking change to do so), but I want to start out with a minimal API until we > prove we need the additional support. I think it's currently enough for the > angular completer, though I might have missed something. I agree - the current Request object looks very compact - although I would say a Performance object should be included in this baseline request object. At least for us, the Performance object takes higher priority than 'result' and 'resourceProvider'.
https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... File pkg/analyzer_plugin/lib/utilities/completion.dart (right): https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:89: await contributor.computeSuggestions(request, collector); Really liking the new structure. Let me see if I'm following the new design pattern: every plugin will be responsible for providing a 'Contributor' which is added into the Generator's list of known contributors. In this sense, every plugin and its contributor is given the collector object to load additions into the suggestions and override the offset and length as necessary? OR, Is the intended strategy for every plugin to have their own generator?
Thanks for the comments! > > ... it's missing ReplacementRange, TypeMemberContributor, > > InheritedReferenceContributor, and the relevance constants. > > At least for the angular plugin, the only thing we really need are request > offset, request source file, and the performance object (we simply create > our own right now that is not used). That list was taken directly from the angular plugin code base (in angular_analyzer_plugin/server_plugin/lib/src/completion.dart, comment out the imports from analysis_server). They are classes and variables that are currently being used, so we either need to move them into analyzer_plugin or you'll need to remove the references to them. (There might be other things we need to port as well that I've missed.) > > Second, the request object's API is much smaller ... > > ... I would say a Performance object should be included in this baseline > request object. > > ... > > At least for us, the Performance object takes higher priority than 'result' > and 'resourceProvider'. Just trying to understand your need: If the performance object isn't being used, why should it be included? Why does it have a high priority? (In case there's a miscommunication on my part, if it isn't included in the API you won't have to create one at all.) https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... File pkg/analyzer_plugin/lib/utilities/completion.dart (right): https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:89: await contributor.computeSuggestions(request, collector); Every plugin is running in a separate isolate, and as such cannot share anything with any other plugin (or with server), so each plugin will create it's own generator and pass in one or more contributors. (The angular plugin is currently using three contributors, two of which are from server: TypeMemberContributor and InheritedReferenceContributor. Hence my comment about needing to get those ported over.)
> That list was taken directly from the angular plugin code base (in > angular_analyzer_plugin/server_plugin/lib/src/completion.dart, comment out the > imports from analysis_server). They are classes and variables that are currently > being used, so we either need to move them into analyzer_plugin or you'll need > to remove the references to them. (There might be other things we need to port > as well that I've missed.) Oh I see what you mean; sorry I was talking about in context of the request object. Wherever you move InheritedReferenceContributor and TypeMemberContributor, it should be fine for us. As for ReplacementRange, it could be nice to have it an abstract class and replace the current implementation as 'DartReplacementRange' since we also do our own replacementRange calculation. However, if we're the only plugin that will do this, it might be overkill. > Just trying to understand your need: If the performance object isn't being used, > why should it be included? Why does it have a high priority? (In case there's a > miscommunication on my part, if it isn't included in the API you won't have to > create one at all.) Poor wording on my part. We create a Performance object to pass into our request to prevent it from crashing and complaining, but we don't use it since we currently never send back our Performance object; just replacement range, length, and suggestions. With the structure above, it would be nice if we can load data into our own performance object, which can be used to obtain metrics of our contributors. Then when all the sugggestions are aggregrated from all plugins, this 'aggregating function' can also record each plugins' individual performance and report it.
lgtm https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... File pkg/analyzer_plugin/lib/utilities/completion.dart (right): https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:89: await contributor.computeSuggestions(request, collector); On 2017/05/31 18:39:43, Brian Wilkerson wrote: > Every plugin is running in a separate isolate, and as such cannot share anything > with any other plugin (or with server), so each plugin will create it's own > generator and pass in one or more contributors. (The angular plugin is currently > using three contributors, two of which are from server: TypeMemberContributor > and InheritedReferenceContributor. Hence my comment about needing to get those > ported over.) Understood. Just wanted to make sure it was clear - but each plugin creating its own generator and needed contributors (created or imported) sounds great.
> As for ReplacementRange, it could be nice to have it an abstract class and > replace the current implementation as 'DartReplacementRange' since we > also do our own replacementRange calculation. Yeah. It appears that you're using ReplacementRange to compute defaults and then replacing those when appropriate. I'm not sure whether an abstract class makes sense, but we should talk more when I get to moving these classes. > With the structure above, it would be nice if we can load data into our own > performance object, which can be used to obtain metrics of our contributors. > Then when all the sugggestions are aggregrated from all plugins, this > 'aggregating function' can also record each plugins' individual performance > and report it. I definitely want to support performance data gathering. In order to aggregate data across plugins we'll need to have a common format for that data (and a wire protocol for reporting it). Probably a separate discussion as to what information we want to be able to gather.
LGTM https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... File pkg/analyzer_plugin/lib/utilities/completion.dart (right): https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:38: void set offset(int offset); Should we throw an exception if a different value is set? So that client's code does not expect that it can set one offset/length, addSuggestion(s), then set another offset/length, add more suggestions.
https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... File pkg/analyzer_plugin/lib/utilities/completion.dart (right): https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:38: void set offset(int offset); > Should we throw an exception if a different value is set? Maybe. I definitely considered that. It would require the angular plugin to change some. It's currently setting default values, then sometimes changing those later. But it would be safer. Minimally I should probably add some clarifying text to the doc comment.
https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/src... File pkg/analyzer_plugin/lib/src/utilities/completion.dart (right): https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/src... pkg/analyzer_plugin/lib/src/utilities/completion.dart:47: final ResolveResult result; I mentioned this elsewhere in an email, this `result` field isn't useful to us if its of type `ResolveResult`. https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... File pkg/analyzer_plugin/lib/utilities/completion.dart (right): https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:38: void set offset(int offset); On 2017/05/31 19:33:20, Brian Wilkerson wrote: > > Should we throw an exception if a different value is set? > > Maybe. I definitely considered that. > > It would require the angular plugin to change some. It's currently setting > default values, then sometimes changing those later. > > But it would be safer. Minimally I should probably add some clarifying text to > the doc comment. We could definitely make this change if necessary. We calculate offsets and regions with one algorithm in one place, so I don't think the defaults are strictly necessary. https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:87: request.checkAborted(); should this be checked at the end of the contributors running too? https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:90: } catch (exception, stackTrace) { Maybe this shouldn't be caught here, but at the layer above, so that `ServerPlugin` can send the notification for this, and this can return a CompletionSuggestionsResult directly without the wrapper class https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:120: ResourceProvider get resourceProvider; What is the purpose of the resource provider? https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... File pkg/analyzer_plugin/lib/utilities/generator.dart (right): https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/generator.dart:38: for (final notification in notifications) { could be `notifications.forEach(channel.sendNotification)`
https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/src... File pkg/analyzer_plugin/lib/src/utilities/completion.dart (right): https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/src... pkg/analyzer_plugin/lib/src/utilities/completion.dart:47: final ResolveResult result; Actually, you need this field because it's needed by the other contributors you're using. The question, I think, is how to allow you to pass along the data you also need in your contributor. https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... File pkg/analyzer_plugin/lib/utilities/completion.dart (right): https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:87: request.checkAborted(); I assume that the time it takes to get the next contributor from the iterator is small enough that the end of the loop is essentially the same as the beginning of the loop, so I don't think that would be useful. https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:90: } catch (exception, stackTrace) { If we catch this in an outer context, then this method wouldn't be able to return any suggestions if one of the contributors throws an exception. I was thinking that it would be better to return partial results than to return no results. https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:120: ResourceProvider get resourceProvider; It's needed by the completion contributor that works inside (import, export and part) directives. We use the resource provider for all file system access. It has the advantage that many of our tests can get data from memory rather than a real file system, which decreases how long it takes to run them.
https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/src... File pkg/analyzer_plugin/lib/src/utilities/completion.dart (right): https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/src... pkg/analyzer_plugin/lib/src/utilities/completion.dart:47: final ResolveResult result; On 2017/05/31 22:51:47, Brian Wilkerson wrote: > Actually, you need this field because it's needed by the other contributors > you're using. The question, I think, is how to allow you to pass along the data > you also need in your contributor. Strictly speaking, I don't think either use this field. It seems to me like in an ideal world, I should be able to provide an AST node, or a ClassElement, to an instance of the right class in order to get the references, and not have to pass in something as heavy duty as a ResolveResult. We've had similar issues before, where we needed to autocomplete a AST snippet with no CompilationUnit, and therefore couldn't use the API even though the contributor never used the CompilationUnit. https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... File pkg/analyzer_plugin/lib/utilities/completion.dart (right): https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:87: request.checkAborted(); On 2017/05/31 22:51:47, Brian Wilkerson wrote: > I assume that the time it takes to get the next contributor from the iterator is > small enough that the end of the loop is essentially the same as the beginning > of the loop, so I don't think that would be useful. I mostly mean that the unrolled loop looks like: checkAbort(); runContributor1(); checkAbort(); runContributor2(); sendNotifications(); rather than checkAbort(); runContributor1(); checkAbort(); runContributor2(); checkAbort(); sendNotifications(); because runContributor2() may take a nontrivial amount of time. But depending on what checkAbort is trying to accomplish it may not matter -- if its only trying to avoid running contributors that don't need to be run, the difference between the two doesn't matter. https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:90: } catch (exception, stackTrace) { On 2017/05/31 22:51:47, Brian Wilkerson wrote: > If we catch this in an outer context, then this method wouldn't be able to > return any suggestions if one of the contributors throws an exception. I was > thinking that it would be better to return partial results than to return no > results. Interesting. Yeah, that makes sense. I guess my main concern is that it will be easy to forget to send the notifications inside the implementation of ServerPlugin. Is there any other way we can make that easier to remember? Like passing a Channel reference in instead of getting a List out? https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:120: ResourceProvider get resourceProvider; On 2017/05/31 22:51:47, Brian Wilkerson wrote: > It's needed by the completion contributor that works inside (import, export and > part) directives. We use the resource provider for all file system access. It > has the advantage that many of our tests can get data from memory rather than a > real file system, which decreases how long it takes to run them. I would say then that it makes more sense for the contributor to hold a reference to the resourceProvider than for the request to hold a reference to it. But then contributor instances are coupled to their resourceProviders....Maybe, since drivers are already coupled to resource providers, that drivers should produce contributors. But then contributors would have to be known by package:analyzer....hmmm....
https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... File pkg/analyzer_plugin/lib/utilities/completion.dart (right): https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:38: void set offset(int offset); Done https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:87: request.checkAborted(); > ... if its only trying to avoid running contributors that don't need to be run ... Yes, that's the purpose. The spec doesn't say what we do if a completion request is received before the last response is sent, but we know that all of our existing clients will discard any results for anything other than the most recent request. We're just avoiding unnecessary work. https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:90: } catch (exception, stackTrace) { > Is there any other way we can make that easier to remember? Your suggestion elsewhere, which I like, of defining a "completion mixin" would solve this problem nicely. https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/completion.dart:120: ResourceProvider get resourceProvider; > I would say then that it makes more sense for the contributor to hold a > reference to the resourceProvider than for the request to hold a reference to it. If the request object holds on to the resource provider, then we don't require every implementation of a contributor to have a field for it and to take a provider as an argument to the constructor. So there's less boiler plate for plugin contributors to duplicate. https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... File pkg/analyzer_plugin/lib/utilities/generator.dart (right): https://codereview.chromium.org/2918613002/diff/1/pkg/analyzer_plugin/lib/uti... pkg/analyzer_plugin/lib/utilities/generator.dart:38: for (final notification in notifications) { Yes, but last I checked, that's quite a bit less efficient.
Description was changed from ========== Add completion support for plugins ========== to ========== Add completion support for plugins R=maxkim@google.com, scheglov@google.com Committed: https://github.com/dart-lang/sdk/commit/12398ad9172005559d7beb4bf14dae000eb18833 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as 12398ad9172005559d7beb4bf14dae000eb18833 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
