Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3003573002:
Add minimal support for sending notifications (Closed)
|
Created:
3 years, 4 months ago by Brian Wilkerson Modified:
3 years, 3 months ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd minimal support for sending notifications
R=scheglov@google.com
Committed: https://github.com/dart-lang/sdk/commit/0a50effce5c00a853604d0150f4a3ac684d8163d
Patch Set 1 #
Total comments: 4
Patch Set 2 : Removed optional parameter #
Messages
Total messages: 14 (2 generated)
brianwilkerson@google.com changed reviewers: + mfairhurst@google.com, scheglov@google.com
I would like to extend the base plugin class in order to provide some of the boilerplate needed by plugins that wish to support notifications. I thought some concrete code might make the discussion easier. Mike: Will the use of `ResolveResult` create problems for angular? My primary motive for including it is to improve performance in the case where we are responding to having just computed results and need to use those results to send the notifications.
This looks great. This is a great idea.
The resolve result will be tricky for us. An easy solution could be to make it
"covariant Object resolveResult" and then users could write "sendErrors(String
path, ResolveResult result)" no problem, while we would write "sendErrors(String
path, DirectivesResult result)".
I do sometimes feel like we're overusing "covariant", though. Its kind of like a
backdoor to the typesystem, a way of reducing generics bookkeeping when it gets
very complicated in the face of large interrelated class hierarchies.
For our still mostly simple hierarchy, we could get away with having a few
generic parameters. Maybe if we want to have that generic parameter available
but not affect those who don't need it, maybe we can do like:
class LanguageSpecificAnalyzerPlugin<LanguageResult> {...}
class AnalyzerPlugin extends LanguagSpecificAnalyzerPlugin<DartResult> {...}
class AngularAnalyzerPlugin extends
LanguageSpecificAnalyzerPlugin<DirectivesResult> {...}
We also already have like
class CompletionSupportMixin {...}
class DartCompletionSupportMixin extends CompletionSupportMixin {...}
class AngularPlugin with CompletionSupportMixin {...}
maybe that too could be
class CompletionSupportMixin<LanguageResult> {...}
class DartCompletionSupportMixin extends CompletionSupportMixin<DartResult>
{...}
class AngularPlugin with CompletionSupportMixin<DirectivesResult> {...}
At the end of the day, whether this API would be more easily versioned, or
friendlier to use, or more discoverable, than the covariant approach, I'm not
sure, I don't spend as much time hardening APIs as you do :)
But I do know that if we did this here, we'd be able to enforce that all who
extend AnalyzerPlugin will have to implement `sendErrors(ResolveResult result)`
and that we would still be able to ourselves implement
`sendErrors(DirectivesResult result)`.
And in the mixins, if we (or others with plugins that misbehave as much as
ours!!) accidentally used a Dart mixin instead of customizing the base class for
our DirectivesResult, then we'd get compile time errors that we don't get right
now since everything is happy as "covariant".
The length of my explanation there itself shows that it is *not* the simplest
solution. I wonder if there's still a simpler, cleaner, better solution out
there, that would let us have dart users deal with ResolveResults and us deal
with something else.
> I do sometimes feel like we're overusing "covariant", though. Its kind of like a backdoor to the typesystem ... I agree. Especially in the case of `covariant Object`. One question to ask in a situation like this is whether a single type (which is all that's allowed by a type variable) will always be right for clients. Will the angular plugin ever want to provide additional data for notifications related to Dart files (and hence need a different result class), or will all of the notifications always be for Html files? There's a third way to solve this, which I played around with briefly. That's to _not_ pass the result through any of these methods. The implication of that is that every `sendFooNotification` method would need to explicitly get the analysis results needed to provide the data. (Of course, that's already the case when the results are `null`, which is always the case for the `sendNotificationsForSubscriptions` code path, although we could fix that.) The downside is that if the results are not being cached somewhere then that could cause performance issues. Which means that plugin implementors need to cache some analysis results without caching everything, and caching is hard to get right. But as I said, we kind of have the same issue either way. The upside is that each method could get exactly and only the level of analysis required. Some notifications, for example, might only require a parse result, while others need fully resolved results. I'm not sure that this is a big upside, though, as I expect most plugins will need fully resolved results for at least one notification, so they might as well be made available to all of them.
Konstantin: I'm curious to hear your thoughts on the notion of requiring every `sendFooNotification` method to explicitly get analysis results. I know that we cache results for priority files, and my guess is that these notifications are usually only used for priority files, so perhaps the performance hit will be minimal. (Plugins that don't use AnalysisDriver will need to perform a similar kind of caching, though.)
https://codereview.chromium.org/3003573002/diff/1/pkg/analyzer_plugin/lib/plu... File pkg/analyzer_plugin/lib/plugin/plugin.dart (right): https://codereview.chromium.org/3003573002/diff/1/pkg/analyzer_plugin/lib/plu... pkg/analyzer_plugin/lib/plugin/plugin.dart:448: Future<Null> sendHighlightsNotification(String path, {ResolveResult result}) { I think `result` must be a required parameter in all specific sendXxxNotification() methods. If we have to pull this result, we will do it once, in the enclosing `sendNotificationsForFile` invocation. I guess also that computing notification is synchronous, so we might not need asynchrony here.
Mike: If we do need to keep the `results` parameters, is there a class that we can use as the upper bound for these APIs? (For example, something like `AnalysisResult` from `analyzer/lib/dart/analysis/results.dart`.) I don't really want to use `Object` everywhere. https://codereview.chromium.org/3003573002/diff/1/pkg/analyzer_plugin/lib/plu... File pkg/analyzer_plugin/lib/plugin/plugin.dart (right): https://codereview.chromium.org/3003573002/diff/1/pkg/analyzer_plugin/lib/plu... pkg/analyzer_plugin/lib/plugin/plugin.dart:448: Future<Null> sendHighlightsNotification(String path, {ResolveResult result}) { > I think `result` must be a required parameter in all specific > sendXxxNotification() methods. For performance reasons? In spite of the fact that we're caching results for priority files? > If we have to pull this result, we will do it > once, in the enclosing `sendNotificationsForFile` invocation. If it's a required parameter in `sendXxxNotification` methods, then I'd make it a required parameter in `sendNotificationsForFile`. The place where we'd have to get a result is in `sendNotificationsForSubscriptions`, and we don't currently have a way to get results, so we'd either need to add a `getAnalysisResults` method or leave it an abstract method. > I guess also that computing notification is synchronous, so we might not need > asynchrony here. If we never need to compute results, that should be true.
LGTM https://codereview.chromium.org/3003573002/diff/1/pkg/analyzer_plugin/lib/plu... File pkg/analyzer_plugin/lib/plugin/plugin.dart (right): https://codereview.chromium.org/3003573002/diff/1/pkg/analyzer_plugin/lib/plu... pkg/analyzer_plugin/lib/plugin/plugin.dart:448: Future<Null> sendHighlightsNotification(String path, {ResolveResult result}) { On 2017/08/22 18:33:15, Brian Wilkerson wrote: > > I think `result` must be a required parameter in all specific > > sendXxxNotification() methods. > > For performance reasons? In spite of the fact that we're caching results for > priority files? Well, you're probably right. It's OK with me to keep these "result" parameters optional. We will pull only when user switches between editor tabs, so we get new subscriptions. This should be rare enough, so it does not matter. And for "push" from AnalysisDriver.results we will work efficiently. And in theory plugins could optimize some notifications by using something other than full ResolveResult. Although given that the most useful notifications are navigation and semantic highlighting, and they require full analysis, if we cannot provide them quickly, we have a problem and must fix it anyway. > > > If we have to pull this result, we will do it > > once, in the enclosing `sendNotificationsForFile` invocation. > > If it's a required parameter in `sendXxxNotification` methods, then I'd make it > a required parameter in `sendNotificationsForFile`. The place where we'd have to > get a result is in `sendNotificationsForSubscriptions`, and we don't currently > have a way to get results, so we'd either need to add a `getAnalysisResults` > method or leave it an abstract method. > > > I guess also that computing notification is synchronous, so we might not need > > asynchrony here. > > If we never need to compute results, that should be true. Never mind, keep it async :-)
https://codereview.chromium.org/3003573002/diff/1/pkg/analyzer_plugin/lib/plu... File pkg/analyzer_plugin/lib/plugin/plugin.dart (right): https://codereview.chromium.org/3003573002/diff/1/pkg/analyzer_plugin/lib/plu... pkg/analyzer_plugin/lib/plugin/plugin.dart:448: Future<Null> sendHighlightsNotification(String path, {ResolveResult result}) { > It's OK with me to keep these "result" parameters optional. But we need to keep them? If I've understood Mike correctly, we would need to change all of the signatures to something like Future<Null> sendHighlightsNotification(String path, {Object result}); because the angular plugin need to pass a DirectivesResult rather than a ResolveResult and Object is the only common supertype. If it were efficient enough that each sendXxxNotification method could create its own result (and I think we decided that it *is* or they couldn't be optional) then we could get rid of the optional parameter. Obviously, I like it better with the parameter because it seems wasteful to re-access data we could already have, but I don't like the idea of declaring it to be of type Object. The other option (that Mike mentioned) is to make it be of type 'R', where R is a type parameter for the class. We could define a convenience class for "normal" plugin developers: abstract class DartServerPlugin extends ServerPlugin<ResolveResult> {} but I'm concerned about the number of type parameters we'd need in order to make everything as flexible as it might need to be. That said, I kind of like this better than using Object, but removing the argument seems even cleaner.
On 2017/08/23 14:06:43, Brian Wilkerson wrote: > https://codereview.chromium.org/3003573002/diff/1/pkg/analyzer_plugin/lib/plu... > File pkg/analyzer_plugin/lib/plugin/plugin.dart (right): > > https://codereview.chromium.org/3003573002/diff/1/pkg/analyzer_plugin/lib/plu... > pkg/analyzer_plugin/lib/plugin/plugin.dart:448: Future<Null> > sendHighlightsNotification(String path, {ResolveResult result}) { > > It's OK with me to keep these "result" parameters optional. > > But we need to keep them? > > If I've understood Mike correctly, we would need to change all of the signatures > to something like > > Future<Null> sendHighlightsNotification(String path, {Object result}); > > because the angular plugin need to pass a DirectivesResult rather than a > ResolveResult and Object is the only common supertype. > > If it were efficient enough that each sendXxxNotification method could create > its own result (and I think we decided that it *is* or they couldn't be > optional) then we could get rid of the optional parameter. > > Obviously, I like it better with the parameter because it seems wasteful to > re-access data we could already have, but I don't like the idea of declaring it > to be of type Object. > > The other option (that Mike mentioned) is to make it be of type 'R', where R is > a type parameter for the class. We could define a convenience class for "normal" > plugin developers: > > abstract class DartServerPlugin extends ServerPlugin<ResolveResult> {} > > but I'm concerned about the number of type parameters we'd need in order to make > everything as flexible as it might need to be. That said, I kind of like this > better than using Object, but removing the argument seems even cleaner. I think you're right and accessing results should be cheap. So, let's remove the optional parameter altogether. But I hope plugin authors will profile often to fix any inefficiencies here and elsewhere in their code.
ptal. I have removed the optional parameter.
LGTM
Description was changed from ========== Add minimal support for sending notifications ========== to ========== Add minimal support for sending notifications R=scheglov@google.com Committed: https://github.com/dart-lang/sdk/commit/0a50effce5c00a853604d0150f4a3ac684d8163d ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 0a50effce5c00a853604d0150f4a3ac684d8163d (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
