This CL makes three changes:
- moves the message scheduler into a new directory,
- splits the implementation into two files,
- and renames `MessageObject` to `ScheduledMessage`.
It does not make any semantic changes to the code.
The expectation is that we will need to make the message scheduler more
complex as part of improving the performance of the server, and that it
will be easier to do this if the code is structured differently.
Change-Id: I3f8741cadceb34c69fd45c1bf21f331d15824565
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420500
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
A few statements around the package read:
> Producers used in bulk fixes must not modify the FixKind during
> computation.
In fact, this `add_null_check` correction producer seems to be the
_only_ remaining producer that violates this statement. In order to
fix it, such that `fixKind` is set during constructor initialization,
we must do some poking around the AST.
So we convert `_computeTarget` to be static (and change it to also
compute and return any possible null-aware token, renaming it to
`_computeTargetAndNullAwareToken`). We also convert `_isNullAware` to
be static. Then the two public constructors are converted into
factory constructors that determine the fix kind, and store the
`_target` and `_nullAwareToken` in fields, so as to avoid re-computing
them during `compute()`.
Change-Id: Ic66319ef764dd9bd69f0e1059347acf174debb25
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/419420
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
The `DartFixContext.librariesWithExtensions` utility function accepts
a `memberName` parameter but never did anything with it. This change
adds filtering in that function based on the member name.
This code depends on the set of exported extensions, which is provided
in an extension (`LibraryElementExtensions.exportedExtensions`) in the
analysis_server package, which must be moved to the analyzer package.
Change-Id: I06c8a9a0e11ceb70e75517b30c0b048321d6b024
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/418641
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
For troubleshooting, this allows the client to override this flag.
I'd originally planned to use ClientConfiguration for this and allow it to be changed on-the-fly, however this can result in a mix of requests and therefore I decided it best to only support setting once during startup (which is part of initialization, and therefore before general requests start being sent).
This will require some Dart-Code work to provide a value here (and without a value, it will always be the servers default).
See https://github.com/dart-lang/sdk/issues/60440
Change-Id: Ie9843543d6d491afb046f3d1106211b7db852605
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/419541
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
The legacy_many_files_in_flutter_set_subscriptions benchmark shows how
"flutter.setSubscriptions" calls can make the analyzer slower to
respond.
What happens is this:
* The user opens a new file in the IDE.
* The IDE sends the `flutter.setSubscriptions` request which equates to
a call to `getResolvedUnit` for each file in the request. If this is,
say, 300 files it's 300 calls to `getResolvedUnit`.
* The IDE sends a `edit.getAssists` request for the newly opened file.
This request starts processing, reaches `getResolvedLibrary(file)`
which calls `getUnitElement` ultimately adding the path to
`_unitElementRequestedFiles` which in `performWork` is done _after_
`_requestedFiles`, meaning it has to do all the flutter requested
files first.
* The user might then request completion for instance, but because the
analyzer only processes one request at a time it has to wait for the
`edit.getAssists` request to finish first, which had to wait for the
files from the `flutter.setSubscriptions` request to process.
All in all it's a lot of waiting for the user.
This CL adds a `interactive` option to the `getResolvedUnit` call. It
defaults to true in which case files are still added to
`_requestedFiles` and processed the same. If it's false it will instead
be added to a newly introduced list instead and processed at a lower
priority. Subscription requests are changed to pass `false` to
`interactive`, avoiding the scenario above.
Comparing before this CL with this CL on the
"legacy_many_files_in_flutter_set_subscriptions" benchmark with 100
files / CodeType.ImportChain these are the statistics on the changes
based on 5 runs each:
```
Completion after open of new file: -81.6652% +/- 7.7564% (-3.70 +/- 0.35) (4.53 -> 0.83)
getAssists call: -96.6315% +/- 0.9307% (-3.61 +/- 0.03) (3.74 -> 0.13)
peak virtual memory size: -5.6786% +/- 3.2964% (-139.00 +/- 80.69) (2447.80 -> 2308.80)
total program size (virtual): -4.6387% +/- 3.8146% (-110.80 +/- 91.11) (2388.60 -> 2277.80)
```
Even when https://github.com/flutter/flutter-intellij/issues/7980 is
hopefully fixed I think it is a fair change to de-prioritize a
non-interactive request.
Change-Id: Icba2faebf12f9913cf24db7cb90fdc6f4c74164e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/418020
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Jens Johansen <jensj@google.com>
The motivation here was to remove the local duplicate variables like
`final_foo` or `foo_final`. These variables only existed because the
variables they duplicate lose their promoted types inside the
`builder.addDartFileEdit` closure. They lose their promoted type
because they are multiply assigned in loops or not always promoted to
be non-null.
Often the fix is to replace a `if (x == null) return` with
`} else { return; }`. I think this code more directly represents the
flow of code, rather than relying on the nullity of a variable, like
"is this variable still null? Oh then we must not have entered any
of the situations above, so we should return."
Change-Id: I9dd6686b7f4d9c6caf59c171179b129a5a957ba8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/418160
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
The motivation here was to remove the local duplicate variables,
`isEmptyIdentifier_final`, `context_final`, and `returnType_final`.
These variables only existed because the variables they
duplicate lose their promoted types inside the `builder.addDartFileEdit`
closure. They lose their promoted type because they are multiply
assigned.
This CL simplifies each method by taking advantage of early returns
and implicit late assignment.
Change-Id: I81ff248d97e875a5b75c7d20b8e95f1b0c08ea24
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417701
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
The motivation here was to remove the local duplicate variable,
`final_propertiesBuilderName`. This variable only existed because the
variable it duplicates loses its promoted type inside the `builder.addDartFileEdit` closure. It loses its promoted type because
it is multiply assigned in a for loop.
This CL extracts a portion of the `compute()` method, which is solely
concerned with computing the real "target". The diff looks large
because of how that portion is moved into a helper method that is
_below_ `compute()`.
I think each of these changes then improves readability: extracting
part of a 180-line long method into one helper, and avoiding a local
duplicate variable.
Change-Id: I96b6ccdbd627d3cf3cc4b7f7bbcba7ee5dd9a561
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417700
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
For convenience, we usually include Inline Values for the "current" line so that when stopped at an `if` statement, you can see their values:
```
if (foo == 1) { // [foo = 2]
```
We have an experiment that includes inline values for getters (since although they can have side-effects, users tend to expect to see them in debug views, especially when the getter/setter are implicit), but triggering them before the code ever would have may be more unexpected.
This change uses two different ranges so that only variables are included for the current line.
Change-Id: I19a129a28f7b5296281dbee139855c738cd37194
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/418340
Commit-Queue: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
The motivation here was to remove the local duplicate variable,
`node_final`. This variable only existed because the variable it
duplicates loses its promoted type inside the `builder.addDartFileEdit`
closure. It lose its promoted type because they are multiply assigned
in a while loop.
This CL extracts the first portion of the `compute()` method, which
is solely concerned with computing the real "target". The diff looks
large because of how that portion is moved into a helper method that
is _below_ the second half of `compute()`.
I think each of these changes then improves readability: extracting
part of a 140-line long method into one helper, and avoiding a local
duplicate variable.
Change-Id: Ic209e4be49863657f969eadf909ab05d13e9a254
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417680
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
To support this feature:
* Remove `DartAssistContextImpl` as it has no differences from
`DartAssistContext`. Thus the getters `DartAssistContext` are made
into fields, and all callers now call `DartAssistContext()`.
* Add a top level `computeAssists` function, similar to the top-level
`computeFixes` function.
Change-Id: I3ef5fd9ee9430150004b29c11ab972939b85b214
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417360
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Currently, the `AnalysisContextCollectionImpl` constructor has a
callback parameter, `updateAnalysisOptions2`, which has 3 required
parameters. The caller can then pass in a closure which accepts those
parameters, and updates analysis options.
I find this to be a bit of an awkward API, and a little bit complex, so
it would be good if we can simplify it. It is all private impl, but it
is used in a few codebases, I believe. Dartdoc is one.
So, it looked to me like none of the callers _use_ the `ContextRoot`
parameter, so it would be good to simplify here, and remove it from
the callback signature.
This is _not_ the final API I'm looking for. Just an incremental step,
and since it is private API, it doesn't need to be in a major release
of analyzer.
Change-Id: Iad9da9f36e817add0bd8c49975ecd14311588661
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/392204
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
The motivation here was to remove the local duplicate variables,
`isExpression_final` and `binary_final`. This variables only existed
because the variables they duplicate lose their promoted types inside
the `builder.addDartFileEdit` closure. They lose their promoted types
because they are multiply assigned.
This CL extracts the first portion of each `compute()` method, which
is solely concerned with computing the real "target". The diff looks
large because of how that portion is moved into a helper method that
is _below_ the second half of `compute()`.
I think each of these changes then improves readability: extracting
part of a long method into one helper, and avoiding a local duplicate
variable.
Change-Id: I6919b04a39dfc02f3b2f7b74727d3f576c71d37b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417022
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
In this one, we can extract out the `elementText` computation so that
it is assigned once, and we can avoid duplication with the
`elementText_final` variable.
Change-Id: I62e54835e36d1a896589e4e9b69ce6ea1fea4370
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/416885
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
The motivation here was to remove the local duplicate variable,
`target_final`. This variable only existed because the variable it
duplicates, `target`, loses its promoted type inside the
`builder.addDartFileEdit` closure. It loses it's promoted type because
it is assigned late.
This CL extracts the first ~60 lines of the `compute()` method, which
is solely concerned with computing the real "target". The diff looks
large because of how the first ~60 lines of `compute()` is moved into
a helper method that is _below_ the second half of `compute()`.
I think each of these changes then improves readability: extracting
part of a 170-line long method into one helper, and avoiding a local
duplicate variable.
Change-Id: I20e2eb3479f53810d4dfcab9b88ebacb446e0f62
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/416884
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Recently a bug was fixed in the formatter, regarding how declarations
preceded by an end-of-line comment are formatted. Good to just bundle
the formatting changes into one CL.
Change-Id: Ieab8ec3263932505f8b8da77fbe637dffc61b46d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417040
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
This lint rule is a core lint rule; we have suppressed it only for
pre-existing code reasons.
There are a few individual files which simply have a consistent pattern
of including underscores in some names, so I add inline ignores there.
Change-Id: I89e6010203868fc10fda12b15353de41881d9b15
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/416900
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
The motivation here was to remove the local duplicate variable,
`target_final`. This variable only existed because the variable it
duplicates, `target`, loses its promoted type inside the
`builder.addDartFileEdit` closure. It loses it's promoted type because
it is multiply assigned.
This CL extracts the first half of the `compute()` method, which is
solely concerned with computing the real "target" and "from type" and
"to type" into a separate method. The diff looks large because of how
the first half of `compute()` is moved into a helper method that is
_below_ the second half of `compute()`.
I think each of these changes then improves readability: extracting
part of a 140-line long method into one helper, and avoiding a local
duplicate variable.
Change-Id: I543bf39e17f0e44011da8741b5f12380e1425f3f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/416883
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
The PluginServer class needs access to AssistProcessor in order to
compute assists. That class needs access to a few other classes, which
then must also be moved:
* assist_core.dart - the Assist class
* assist_dart.dart - the DartAssistContext class
* assist_generators.dart - the registeredAssistGenerators variable
* assist_performance.dart - the AssistPerformance and
GetAssistsPerformance classes
* assist_processor.dart - the AssistProcessor class with it's
singular API, `compute()`
* performance.dart - the ProducerRequestPerformance class
* the `addCaretAtOffset` helper function, refactored into a
`withCaretAt` extension method
This change is functionally a no-op.
Change-Id: Ic883d21e9cc8c3db1f6093f830f01ec6eb9a0976
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/416680
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
This change means that during `dart analyze`, an exception caught in a plugin isolate will be printed to the terminal, and the process will
exit (similar to the support for when the isolate has static errors).
Change-Id: I31b1ebe7a71a331274d4f1dc1ea1b94f33e2329b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/415981
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
This map is a map of _assists_ and the lint rules that can _fix_, when
used as a _fix_ producer. A rather specific set of information. It was
being passed around everywhere, but now with the instance of
`_RegisteredAssistGenerators` to hang onto, we can leave the map there,
and remove it from a handful of other classes which all ultimately just
passed the map back to AssistProcessor.
Change-Id: Ib2aa4ea1fdb23bbaf55b3377232f2205b7f91447
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/415460
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Brian Wilkerson