Replaces "is" tests that refer to classes derived from
`DiagnosticCode` with tests on `DiagnosticCode.type`:
- `x is ScannerErrorCode || x is ParserErrorCode` is replaced with
`x.type == DiagnosticType.SYNTACTIC_ERROR`. This is exactly
equivalent, because `ScannerErrorCode` and `ParserErrorCode` are the
only two `DiagnosticCode`-derived classes whose `type` method returns
`SYNTACTIC_ERROR`.
- `x is TodoCode` is replaced with `x.type ==
DiagnosticType.TODO`. This is exactly equivalent, because `TodoCode`
is the only `DiagnosticCode`-derived class whose `type` method
returns `TODO`.
- In `statement_completion.dart`, `x is! HintCode && x is!
WarningCode` is replaced with `x.type ==
DiagnosticType.SYNTACTIC_ERROR`. The new test is less accepting: for
example, it previously accepted diagnostics of type
`StaticWarningCode`. This test is used to short-cut the generation
of statement completions such as completing an incomplete `do`
statement. These completions are only necessary to generate if there
is a syntax error, so there should be no user-visible behavior
change.
- In `fix_processor.dart`, `x is LintCode || x is HintCode || x is
WarningCode` is replaced with `x.type == DiagnosticType.LINT ||
x.type == DiagnosticType.STATIC_WARNING`. The new test is more
accepting: it now accepts any diagnostic code whose type is
`STATIC_WARNING`, when previously it only accepted warnings of type
`WarningCode` (and rejected warnings of types like
`StaticWarningCode`). This test determines when quick fixes like
"ignore diagnostic on this line" are offered, so the behavior change
is appropriate.
- Also replaces a documentation reference to `TodoCode` (in
`todo_codes.dart`) with a reference to `DiagnosticType.TODO`.
These changes pave the way for a follow-up CL in which I plan to
eliminate these derived classes entirely.
Change-Id: I6a6a69647ff62dfe06b0219d8b292da53427cf0d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/461140
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Changes several references to derived diagnostic types
(`CompileTimeErrorCode`, `WarningCode`, etc.) to simply
`DiagnosticCode`.
This paves the way for a follow-up CL that will change the static
types of the generated analyzer diagnostic code objects to more
general types. This is part of a longer-term effort to simplify the
`DiagnosticCode` class hierarchy.
Change-Id: I6a6a69642561aebf2607e834fa09fa608cb4fa10
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/458587
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Observation 1: The legacy protocol answered completion requests via a
`server.resolveForCompletion` call, and the LSP protocol answered
completion requests via a `server.getResolvedUnit` call where the idea
is that resolving for completion requires less and is therefore faster.
Solution: Make the LSP protocol use `server.resolveForCompletion` too.
Observation 2: Completion requests often come right after change request
making timing important and the `await driver.applyPendingFileChanges()`
call done in the analysis server "pushed" the timing making the
`server.resolveForCompletion` actually finished after it had already
resolved the whole thing.
Solution: Don't do that - the driver adds it to the queue of work, the
work is done later in `performWork` called from
`AnalysisDriverScheduler._run` where `_applyPendingFileChanges` is
always called anyway (which is the call that completes the
`applyPendingFileChanges`call).
Observation 3: If there is no change yet to be processed, a
`resolveForCompletion` call is slower than a `getResolvedUnit` because
the resolved unit is cached (assuming it's a priority file) and the
`resolveForCompletion` call always parses the file again.
Solution: Respond to the `resolveForCompletion` call with the resolved
unit data if it's available in the cache. The cache is always cleared
when changes happen anyway.
Benchmarks on this stuff is a bit weird because it's timing related - so
while I'd say this is overall just better there are also runs where we
get "bad timing" and the runtimes are therefore not better. In an
attempt to clear it up I've run the benchmarks 25 times each, and
attempted to put the data in two different buckets as needed.
*lsp_type_in_big_file of size 16,000*
```
Fully done after last type (ms):
Difference at 95.0% confidence
-1419.2 +/- 322.074
-16.8866% +/- 3.83225%
(Student's t, pooled s = 566.238)
Whole typing time (ms):
Difference at 95.0% confidence
-1418.88 +/- 322.037
-13.9661% +/- 3.16983%
(Student's t, pooled s = 566.172)
Uncancelled completion response time (ms):
Difference at 95.0% confidence
-1590.48 +/- 682.753
-28.0194% +/- 12.028%
(Student's t, pooled s = 1200.35)
```
The `Uncancelled completion response time` has a big "+/-" so attempting
to "good and bad bucketize" it I get:
good bucket:
```
Difference at 95.0% confidence
-1929.8 +/- 347.712
-35.9528% +/- 6.47797%
(Student's t, pooled s = 543.261)
```
bad bucket (though truthfully there wasn't a clear cutoff before):
```
No difference proven at 95.0% confidence
```
which sort of makes sense: If the completion runs before a (new) change
starts processing we now `resolveForCompletion` instead which is faster,
but if completion runs after the change has started processing we
essentially - both before and after - do nothing (except wait for the
calculation to finish) because we just load the data from cache.
*lsp_type_in_big_file_ask_for_completion, 16,000*
```
Completion #1 (ms):
No difference proven at 95.0% confidence
```
Ehh. There's a clear cutoff in the now, so taking the 8 (how the cutoff
happens to be) fastest from each I get
```
Difference at 95.0% confidence
-1034.12 +/- 75.3617
-37.673% +/- 2.74542%
(Student's t, pooled s = 70.2673)
```
Moving on.
```
Completion #2 (ms):
Difference at 95.0% confidence
-1076.28 +/- 178.446
-37.2601% +/- 6.17769%
(Student's t, pooled s = 313.726)
```
here 2 in the "now" has bad timing, removing them from the statistics
gives
```
Difference at 95.0% confidence
-1182.39 +/- 104.543
-40.9334% +/- 3.6192%
(Student's t, pooled s = 179.748)
```
Moving on.
```
Completion #3 (ms):
Difference at 95.0% confidence
-735.6 +/- 292.553
-25.6471% +/- 10.2%
(Student's t, pooled s = 514.336)
and removing the 8 bad ones:
Difference at 95.0% confidence
-1195.69 +/- 99.305
-41.6884% +/- 3.46232%
(Student's t, pooled s = 156.306)
```
Continuing like this:
```
Completion #4 (ms):
Difference at 95.0% confidence
-948.4 +/- 265.704
-32.9887% +/- 9.24214%
(Student's t, pooled s = 467.134)
and removing the 5 bad ones:
Difference at 95.0% confidence
-1252.87 +/- 86.5425
-43.5793% +/- 3.01026%
(Student's t, pooled s = 143.022)
Completion #5 (ms):
Difference at 95.0% confidence
-1067.88 +/- 199.266
-37.0011% +/- 6.90439%
(Student's t, pooled s = 350.329)
and removing the 3 bad ones:
Difference at 95.0% confidence
-1236.76 +/- 69.5776
-42.8527% +/- 2.4108%
(Student's t, pooled s = 118.18)
```
Moving on to the "Completion without change" I realize just now that the
benchmark for the first entry is broken - it doesn't wait until the
previous change has been processed, meaning that in the 3 cases where we
got bad timing in "Completion #5 (ms)" we see about the same result as
before, but in the 22 other cases we see bad results because it has to
wait until the previous change has been processed. For the remaining
(2-5) there is no virtually change which makes sense because both before
and now it just fetches the resolved unit from cache.
*legacy_type_in_big_file_ask_for_completion*
```
Completion #1 (ms):
Difference at 95.0% confidence
-1817.08 +/- 60.6836
-48.0638% +/- 1.60515%
(Student's t, pooled s = 106.688)
Completion #2 (ms):
Difference at 95.0% confidence
-2208.56 +/- 48.4844
-55.4647% +/- 1.21761%
(Student's t, pooled s = 85.2403)
Completion #3 (ms):
Difference at 95.0% confidence
-2159.68 +/- 69.4145
-53.0717% +/- 1.70578%
(Student's t, pooled s = 122.037)
Completion #4 (ms):
Difference at 95.0% confidence
-2264.44 +/- 73.2112
-53.5455% +/- 1.73117%
(Student's t, pooled s = 128.712)
Completion #5 (ms):
Difference at 95.0% confidence
-2147.4 +/- 68.5023
-50.9254% +/- 1.62453%
(Student's t, pooled s = 120.434)
```
The first "Completion without change" suffers from the same as before
and I will skip it here.
```
Completion without change #2 (ms):
Difference at 95.0% confidence
-416.28 +/- 28.2024
-61.3512% +/- 4.15646%
(Student's t, pooled s = 49.5826)
Completion without change #3 (ms):
Difference at 95.0% confidence
-687.24 +/- 21.9848
-92.6499% +/- 2.96387%
(Student's t, pooled s = 38.6514)
Completion without change #4 (ms):
Difference at 95.0% confidence
-637.32 +/- 26.482
-95.2703% +/- 3.95868%
(Student's t, pooled s = 46.5579)
Completion without change #5 (ms):
Difference at 95.0% confidence
-702.32 +/- 18.7277
-95.8301% +/- 2.55535%
(Student's t, pooled s = 32.925)
```
I don't know why there doesn't appear to be any timing related issues
here (maybe sending and receiving the entire big file (in legacy vs in
lsp where a small 'diff' is send) taking more time pushes the timing,
but I'm guessing) - nor do I know why now "Completion without change #2"
is slower (~250 ms) than the subsequent ones (~30 ms).
Change-Id: I4c21d658efccbcf197eedb69f466b2942b78c4b9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/457364
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Jens Johansen <jensj@google.com>
In testing, I saw that _added_ files were not being analyzed at all by
plugins. It turned out that `PluginManager.broadcastWatchEvent` uses
that glob to decide if watch events should be broadcast to a given
plugin isolate. It uses this Glob to determine that answer:
```dart
Glob(
_resourceProvider.pathContext.separator,
pattern,
).matches(filePath)
```
and using `'*.dart'` for that pattern, always answers "no, this file
does not match '*.dart'"; I believe `**.dart` is the correct glob
pattern we should use.
Change-Id: Id99d758151629c485884ba20e4d1f49f574f5889
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/452160
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
I had not run the first fix through manual testing. Manual testing
revealed a second location (!!) where the file path is stored, for
diagnostics. This updates the code to send a notification for each file
path, including parts.
Change-Id: Iee60330c579563f31c8d9a2c65a22c9cde4b64d8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/448881
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Fixes https://github.com/dart-lang/sdk/issues/61449
The previous code, sure enough, only created a DiagnosticReporter for
the defining unit, and only visited the main unit.
The fix is to create a DiagnosticListener for each unit, set the
DiagnosticReporters correctly and the "current unit" as we go, and then
pull the reported diagnostics from each listener.
Change-Id: I6b645284c58e5e35ccc5b89f30f6ffe97f48296c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/448644
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
This renames the old screen, "Legacy plugins" and adds a new screen,
"Plugins". On the new plugins screen, we ask the plugins isolate(s) for
their plugins details, and print the following, for eadh plugin:
* the plugin's name
* the names of the registered lint rules
* the names of the registered warning rules
* the IDs and "messages" of the registered assists
* the IDs and "messages" and associated diagnostic codes of the
registered quick fixes
More to come in follow ups:
* The resolved versions of plugin packages (coming from package_config.json)
Change-Id: Ic3dc4c5bffa64fd4da4097c042a847cc064e41ce
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/447763
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
This week we moved some other "plugin analysis options" code into the
private API. This continues that work, in order to add plugin source
options into the AnalysisOptionsImpl signature code.
The classes are deprecated as far as the public API is concerned,
encoded with a `@Deprecated` export. This is not reflected in api.txt.
Change-Id: Ie95851a7b0d54ef6abb8fb1e2b41294fbc5db836
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/447841
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
This change adds a new parameter `typeParametersInScope` to `DartEditBuilder.writeType`, and related methods to deprecate the previous `methodBeingCopied` parameter, which limited our ability to pass in some type parameters that are in scope.
Also fixes a TODO about testing if the `Record` feature is enabled or not.
And as a bonus, we now always write down type parameters by looking for the closest public type or falling back to `Object?` or `dynamic`.
Bug: https://github.com/dart-lang/sdk/issues/61186
Change-Id: I7a7b1c62adb81aecfdf61ebbaa0d95f1d33bced8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/442282
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Auto-Submit: Felipe Morschel <git@fmorschel.dev>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
This library mostly exports _public_ API, and the to-be-deprecated
NodeLintRegistry class. So we just move private RuleContext classes
into a new library, and remove the exports.
The lint rules then are adjusted, mostly just renaming
NodeLintRegistry -> RuleVisitorRegistry. But also, this removes a lot
of implementation imports; most rules now have _no_ private imports!
Change-Id: I023f7efa979f94d208a6ae7421902145e8761c9a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/443364
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Because ChangeBuilder can be used to modify multiple files, the EOL should come from an individual files edit builder and not from the overall ChangeBuilder.
This change renames `eol` on ChangeBuilder to `defaultEol` (since it's still useful to supply a custom default, for example when moving a class from an existing file to a new file, we want to default to the same EOL as the source file), and adds an EOL to file edit builders which is computed from the file content (and falls back to the default from ChangeBuilder if there are none).
My intention here is to not introduce any breaking changes (yet) so the ChangeBuilder constructor interface still has the "eol" named parameter (marked as deprecated) as well as the new `defaultEol` parameter.
I added a new `StringExtension` for `endOfLine` to both analyzer_plugin and analysis_server_plugin as we need this in both places (but I expect one of those projects will ultimately be dropped). This slightly differs from `CorrectionUtils.endOfLine` because it can return `null` which is a signal to fall back to the ChangeBuilder default instead of forcing its own default. (CorrectionUtils.endOfLine now uses that extension, although because of the null/default, it's likely that some - if not all - uses of this should migrate to the new extension and fall back to a default from a ChangeBuilder).
Change-Id: Ie2e6eaba78c96a1cbd79eba881e12bbeef724aa8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/443122
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Before an assist call on a file with non-windows lineendings would first
search through the entire file for Windows line endings, then search for
the non-Windows line endings. As such Windows line endings was faster.
This CL changes the semantics slightly from "Windows line endings if
any Windows line endings exist" to "whatever line endings are first
used" which should be good.
It speeds up assist calls with non-Windows line endings:
Statistics on 5 runs each:
```
Windows / 2000: No change.
Windows / 4000: No change.
Windows / 8000: No change.
Unix / 2000: 1000 assist calls: -46.9900% +/- 6.1715% (-0.88 +/- 0.12) (1.86 -> 0.99)
Unix / 4000: 1000 assist calls: -59.1561% +/- 6.3982% (-1.79 +/- 0.19) (3.03 -> 1.24)
Unix / 8000: 1000 assist calls: -63.8429% +/- 4.3083% (-3.68 +/- 0.25)(5.76 -> 2.08)
```
Change-Id: I61278ed4ad31b891e87951da0a6162fe37376888
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/437224
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Jens Johansen <jensj@google.com>
The diff makes this change look bigger than it is.
Here's how the code worked before: `compute()` called `_addFromProducers()`. `_addFromProducers()` had a local function, also
called `compute()`, which is called per CorrectionProducer. The local
`compute()` function would unconditionally create a ChangeBuilder and
call each CorrectionProducer's `compute()` method, then call back out
to an instance method called `_addFixFromBuilder()`, which would drop
the computed change on the floor if `fixKind` was `null`.
This CL contains the following changes to the above system:
* The local `compute()` function and the instance method,
`_addFixFromBuilder()` are combined together, into one instance
method, `_addFromProducer()`.
* Then, the `fixKind` check is moved to the very top of that
method, bailing out _before_ computing changes if `fixKind == null`.
Change-Id: I6c61059fc36ec1587b15ca01b43830a5b179da9a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417326
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Work towards #60635
In this change, we rework the AnalysisErrorListener deprecation to
better support users who have their own class that implements
AnalysisErrorListener. This change introduces a sealed supertype,
DiagnosticOrErrorListener, with the old implementation,
AnalysisErrorListener, and the new implementation, DiagnosticListener,
as its sole direct subclasses. Users who have implemented
AnalysisErrorListener should be able to instead implement
DiagnosticListener, and their class is an acceptable instance of
DiagnosticOrErrorListener, wherever that is needed.
In a breaking change we can drop AnalysisErrorListener and deprecate
DiagnosticOrErrorListener, and in the next breaking change, we can drop
DiagnosticOrErrorListener.
For reference, see the first API difference when deprecating AnalysisErrorListener and introducing DiagnosticListener: https://github.com/dart-lang/sdk/commit/903d77cc8229972a424941dcb7b7b79741e833eb#diff-dec15868961d7eadcd009f49d129bebcdb747aaa8dbfde5f5a08884e0cf11e32
Change-Id: I3ccf11d54b41fbca98d020d89978d250c16b4c04
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/436480
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Work towards https://github.com/dart-lang/sdk/issues/60635
I tried to keep this minimal but still sensible:
* Rename AnalysisErrorListener and all subtypes to use 'Diagnostic'.
* Rename all instantiations of such classes if they previously
contained the word 'error'.
* Rename `RecordingDiagnosticListener.errors` to `.diagnostics`.
* Rename some _testing_ instance members that had the word 'error'
to instead use 'diagnostic'.
Change-Id: I3948e27ba28ac2494092e04f4e1d201a20cc1135
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/433004
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Konstantin Shcheglov