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>
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>
Sam Rawlins