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>
This is a big CL; no code is really "changed." We basically move 3
components into the public API, which can all be reviewed concisely
in the `api.txt` file.
* The AnalysisRule classes: `AbstractAnalysisRule` (which is good to
make public as a lot of the public API is specified and documented
here), `AnalysisRule`, `MultiAnalysisRule`.
* The Pubspec classes, available for linting pubspec files:
`PubspecDependency`, `PubspecDependencyList`, `PubspecEntry`,
`PubspecEnvironment`, `PubspecGitRepo`, `PubspecHost`, `PubspecNode`,
`PubspecNodeList`, `PubspecVisitor`.
* The `RuleVisitorRegistry` class. This class is needed by analysis
rule authors, and is part of the public API of AnalysisRule.
Change-Id: Ib1803180de9469f4ff39cf1778f96787f5f74b14
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/432363
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
For the analyzer plugin APIs, these classes should not have the word
'Lint' in them, as they are used for all analysis rules, whether they
report lints or warnings.
* LinterContext -> RuleContext
* LinterContextWithParsedResults -> RuleContextWithParsedResults
* LinterContextWithResolvedResults -> RuleContextWithResolvedResults
* LintRuleUnitContext -> RuleUnitContext
I'm open to better names for these. Some of these suffer from a
chain-of-nouns name (a "RuleUnitContext" is a Context for a Unit for
a Rule). So we could go with "UnitContextForRule" and "ContextForRule"
or other ideas like that.
Only RuleContext and RuleUnitContext will be public API.
Change-Id: I5550736beaba7e39b36e7b5584ca6d023aa4d02b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/431703
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Copying change builders in order to enable reverting changes from a
single correction producer when there's an exception is expensive.
This CL replaces that implementation with a transactional model.
Clients are not required to start a transaction (that happens
automatically), but are required to signal the end of a transaction by
invoking either `commit` or `revert`. (Actually, `commit` is assumed
if neither method is invoked before computing the `SourceChange`.)
There is some information in the Dart file edit builder related to
imports that isn't correctly handled. The reason for this is that too
much of the import computation is done up-front. I don't think this
will be a problem in practice (or at least not often), but we should
come back at some point to change the implementation so that we retain
abstract data longer and perform more processing at the very end (where
we'll have complete information and probably be able to do a better
job anyway).
This CL improved the performance of the benchmark that adds and applies
fixes for 10,000 lint violations.
Before it took an average of 11216.0 ms.
After it took an average of 4200.2 ms.
Which is about a 62% improvement.
Change-Id: I12710606455a04e34a82308ede5d9fba1c68b972
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/428060
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Before this change, we had an enforcement that either 'get lintCode' or
'get lintCodes' is implemented, but it was a runtime enforcement. And it
was a little roundabout.
The new system is simpler and more typical:
* `AbstractLintRule` is the parent type, and it's sealed. It provides
`get lintCodes` as an interface. Code outside of the lint rules
requires this getter, in registering codes, etc.
* `LintRule` is the new subclass which is the "90% case." It declares
a `get lintRule`, and offers simpler reporting methods for reporting
a single code everywhere.
* `MultiLintRule` is the new subclass which represents that last
"10% case." It only declares the reporting methods which all require
a LintCode parameter, since there is no concept of a "default lint
code among all my lint codes."
Work towards https://github.com/dart-lang/sdk/issues/50986
Change-Id: I63cbbfc1d936bb4428aa0b8f5fb0638c9b61b3dc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/426284
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
This allows us to traverse the children without needing to create lists.
It also allows us to iterate over lists of nodes more efficiently.
Tested against the script that computes fixes for 10,000 diagnostics,
I'm seeing the following result.
Before this CL the average time was 10692.4 ms.
After this CL the average time was 1900.0 ms.
That's just over an 82% improvement.
Change-Id: Ideff745288c8990e9948b22c5204d176af3cac6e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/426904
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Before this CL, if one has a file like
```
Map<String, String> foo = {
"1": "1",
"2": "2",
"3": "3",
[...]
```
with many violations of the prefer_single_quotes lint, using VSCode and
selecting all would lock up the analysis server, e.g. in
https://dart-review.googlesource.com/c/sdk/+/425503 I showed 400 such
lines taking ~36 seconds on my machine (and 800 such lines taking ~247
seconds).
This is because for each error (lint) in range (i.e. all of them) it
calculates fixes for all of them. Once it's done with that it
deduplicates and throws most of the data away again.
This CL instead only calculates the "fixes for all" for each
combination of error-type and generator, skipping lots of work,
making the whole thing be much faster.
In the data send to the client (VSCode in this instance) the
"diagnostics" for
"Convert to single quoted strings everywhere in file"
contains fewer elements (only 1, vs all before), but it's unclear when
(or if) this is used. Note that the "edit"s does contain all, and
applying it still changes all instances.
I now get these runtimes for the select all case (via the benchmark)
(with cpu governor "performance" which likely wasn't the case for
the ~36 seconds and ~247 seconds above, although it doesn't matter
much with these differences):
400: 0.736783
800: 1.502978
1600: 3.704314
3200: 10.701988
Change-Id: I2b69a77525c0e1ed720c2b3d1de70ccc1fc5e94c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/425861
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Jens Johansen <jensj@google.com>
Work towards https://github.com/dart-lang/sdk/issues/60635
* `AnalysisError._contextMessages` unnecessarily backed the public
`contextMessages` getter; the field is final so it can be public
itself.
* `AnalysisError._correctionMessage` unnecessarily backed the public
`correctionMessage` getter; the field is final so it can be public
itself.
Change-Id: If269d4ed590ef7df81d9b9e3be03766601526d7f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/425620
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
Sometimes - on responding to `edit.getFixes` requests, the method
`getTopLevelDeclarations` is called several times with the same
parameter and caching has then sometimes sped things up.
I then also noticed that it caused `reportAnalysisAnalytics` to be
called many times (e.g. 73 times for one `edit.getFixes` request)
each time spending time collecting data which - on passing to
`analyticsManager.analysisComplete` - was just thrown away because it
only saves the first one.
Combined these changes have been observed to reduce the response time
on `edit.getFixes` by up (down?) to ~20% (e.g. from 5 seconds
to 1 second).
In response to http://b/407797012 where more data is also available.
Unfortunately I haven't been able to reproduce this is a benchmark
(yet anyway). I don't know if it needs slower slower reads, an extreme
amount of files, several contexts or something else entirely.
Change-Id: Id214b0e1b7d8bbef7f6d956408f17fab45618b78
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420323
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Johnni Winther <johnniwinther@google.com>
Reviewed-by: 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>
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>
This is a reland of commit b9b77058a9
Original change's description:
> Migrate to use pub workspace
>
> Use `pub get` to generate `.dart_tool/package_config.json` on gclient sync.
>
> All pkg/ (and a few third_party) packages that are developed inside the sdk repo are included in the workspace from the root `pubspec.yaml`.
>
> All dependencies that are pulled in via DEPS are added as path dependencies via `dependency_overrides` in the root `pubspec.yaml`.
>
> Bug: https://github.com/dart-lang/sdk/issues/56220
> Change-Id: I38c12b608c68da54c57821116cf9aa6696936746
> Tested: relies on CQ of existing tests. Should have no effect on functionality
> CoreLibraryReviewExempt: only core library change is adding a `// ignore:` comment. Should have no influence on functionality
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/397164
> Commit-Queue: Sigurd Meldgaard <sigurdm@google.com>
> Reviewed-by: Alexander Thomas <athom@google.com>
Bug: https://github.com/dart-lang/sdk/issues/56220
Change-Id: I29afabade2d2447dea05121cb87ff50bb21a4b76
Cq-Include-Trybots: luci.dart.try:flutter-linux-try,flutter-web-try
Tested: relies on CQ of existing tests. Should have no effect on functionality
CoreLibraryReviewExempt: only core library change is adding a `//
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/415561
Commit-Queue: Sigurd Meldgaard <sigurdm@google.com>
Reviewed-by: Alexander Thomas <athom@google.com>
Sam Rawlins