Work towards https://github.com/dart-lang/sdk/issues/61868
This adds a new notification type, 'PluginPrint'. There are several
fields and variables then named 'pluginPrint' or 'print', and I am
definitely open to changing these names, but this is the best one that
I thought of.
PluginPrint has three fields: The name of the plugin that printed, the
message that was printed, and the timestamp.
We wrap each plugin's AnalysisRule invocations with a zone, so that
the `print` handler can know the name of the plugin. The prints are
caught and sent to the server isolate as Notifications. The
PluginIsolate then stores the collected prints. The Plugins Insights
page can then retrieve them and display them.
Manual testing, with 1000 libraries that get new lint reported once per
file, I did not observe a negative performance impact.
Change-Id: Id800ae41781c15ce89d06b563878c2b0edae43d3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/486827
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
The RegEx engine in the VM was updated in
e443b89f23 which caused the analyzer
analyzing the CFE to use ~150 mio instructions more.
Part of this was an increased cost in ignore comment processing which
relied on regex. Using regex before the updated engine made
`processPrecedingComments` have a cost of ~240 mio instructions,
updating the regex engine took that to ~264 mio instructions.
This CL gets rid of the regex and takes the cost of
`processPrecedingComments` to ~74.4 mio instructions a saving of about
189 mio instructions (all then analyzing the CFE and looking at output
from `valgrind --tool=callgrind`).
Benchmarking with `perf stat` with normal GC gives:
```
task-clock:u: -2.1760% +/- 1.6063% (-265608341.80 +/- 196062087.98) (12206056036.20 -> 11940447694.40)
page-faults:u: 0.2301% +/- 0.0313% (448.20 +/- 60.88) (194764.60 -> 195212.80)
cycles:u: -2.2906% +/- 1.6090% (-1180119481.60 +/- 828994273.75) (51521138476.00 -> 50341018994.40)
instructions:u: -0.3325% +/- 0.0032% (-196547942.60 +/- 1874215.18) (59120651337.60 -> 58924103395.00)
seconds time elapsed: -2.1715% +/- 1.6011% (-0.27 +/- 0.20) (12.21 -> 11.95)
seconds user: -2.2487% +/- 1.7816% (-0.27 +/- 0.21) (11.87 -> 11.60)
Comparing GC data:
'No' GC change.
```
Note that it must push the GC - the savings isn't really 2% in time.
And with GC disabled:
```
instructions:u: -0.4562% +/- 0.0029% (-185499444.00 +/- 1189012.77) (40663084597.80 -> 40477585153.80)
```
So here a saving of ~185 mio which fits okay with the data from
valgrind.
Change-Id: Ib203baeac6a93f5e37c737080fed342dbd0740a7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/487021
Commit-Queue: Jens Johansen <jensj@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Adds the modifiers `abstract`, `base`, `final`, and `interface` to the
API summary output.
This information is an important part of the public API of a package,
because it determines whether a client can:
- Construct an instance of the class,
- Extend the class, or
- Implement the class.
Change-Id: I6a6a6964ba07db1714bc2fcb549cc15230e87058
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/482362
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Fixes two minor bugs with the sorting of members in the API summary
tool:
- The technique for placing getters next to their corresponding
setters was to sort them lexicographically based on
`Element.apiName`, which in the case of setters appends `=`. This
mostly worked, but due to the fact that `=` is between `9` and `A`
in ASCII, it was wrong in a few corner cases. For example, it would
sort `x`, `x=`, `x1`, and `x1=` in the order `x`, `x1`, `x1=`,
`x=`. Fixing this didn't affect any `api.txt` files in practice.
- The technique for sorting constructors also used `Element.apiName`,
which in the case of an unnamed constructor is `new`. This meant
that if a class had both named and unnamed constructors, the unnamed
constructor would not always be sorted before the other
constructors.
The fix for both bugs is to sort by `Element.name` (which does not add
`=` for setters and is the empty string for unnamed constructors), and
then to break ties by explicitly checking whether the element is a
setter.
Change-Id: I6a6a69648fb5915266a9111c5d884531bba4405d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/482361
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
`YamlEditor.update` should never throw `YamlException`, it should also
never throw `AssertionError`, but we know that it has bugs, and in those
cases it does throw `AssertionError`.
Change-Id: I941d88063effc8c46ba270da8531978a72e7a806
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/472562
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Auto-Submit: Jonas Jensen <jonasfj@google.com>
Commit-Queue: Jonas Jensen <jonasfj@google.com>
Added support for the `hosted` field in plugin definition,
so that analyzer plugins can be sourced from pub servers other
than pub.dev.
TEST=pkg/analysis_server/test/src/plugin2/generator_test.dart
TEST=pkg/analyzer/test/src/options/analysis_options_test.dart
Change-Id: Ie99693ab39c77b3d814843963613798f72e27f62
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/475580
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Auto-Submit: Albert Moravec <albert.moravec@gmail.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Remove the analyzer-only `scanner/reader.dart` re-export and refactor
`Scanner` to accept source text directly.
* Delete `pkg/analyzer/src/dart/scanner/reader.dart`, which only re-exported
`_fe_analyzer_shared` reader types.
* Replace the `Scanner(CharacterReader, ...)` factory with
`Scanner(String contents, ...)`, aligning the API with the underlying
`scanString` implementation.
* Update analyzer, analysis_server, analyzer_cli, analyzer_plugin, and scrape
call sites to pass the source string directly instead of constructing
`CharSequenceReader`.
This removes an unnecessary abstraction layer, reduces
imports/indirection, and makes scanner usage simpler and more uniform
across the repo.
Change-Id: I380b365676c8a62971a3700deadb695d1519d653
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/477600
Reviewed-by: Paul Berry <paulberry@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
This release is so we can publish a version of analysis_server_plugin,
and then start depending on that published version in DAS.
In addition, we get a highly requested fix out to analyzer_testing
users.
Change-Id: I694f47aeee59367c1fc066bc8f7a865406b09917
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/476620
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Changes the signature of the following constructors:
- Scanner
- Scanner.fasta
- Parser
So that they accept a `DiagnosticReporter` object rather than a
`DiagnosticListener` object. This brings the scanner and the parser
into alignment with the majority of the rest of the analyzer (which
reports errors using `DiagnosticReporter` rather than by talking to
`DiagnosticListener` directly).
It also makes the `source` parameter of these constructors
unnecessary, because the `source` can be obtained from the
`DiagnosticReporter`.
Change-Id: I6a6a6964607ffb52a8332a6e618dc0c9a1e48c5c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/473442
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Bob Nystrom <rnystrom@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
When we landed null-safety we accidentally disabled internal self-testing in YamlEditor.
We fixed this in: https://github.com/dart-lang/tools/pull/2284
But this was reverted because this test broke. With this we should able to land:
https://github.com/dart-lang/tools/pull/2299
TL;DR: `YamlEditor.update` may throw `AssertionError` if it has an internal error.
Internal errors in `YamlEditor` should not happen, but we have bugs.
There is open PRs to fix some of those bugs, but in practice it's not unlikely
that there will always be bugs. We're modifying YAML source using `SourceSpan`s
from the YAML parser. Changes in the parser, surprising corner cases, complex modifications
or combinations of these can probably cause internal errors.
These internal errors happens when:
* The result YAML output is invalid YAML.
* The resulting YAML doesn't match the same semantic modification on the original YAML structure, when compared with deep equals.
Change-Id: I159b37e9a9f039f92c82881ccac2c4826332f816
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/471460
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Jonas Jensen <jonasfj@google.com>
Changes the initialization logic for `PluginConfiguration` so that it
always populates the `diagnosticConfigs` field with a map that ignores
case when matching keys. This insures that diagnostic codes in a
plug-in configuration are matched in a case-insensitive fashion.
Also changes the logic in `pkg/analysis_server_plugin` so that when it
looks up names in this map, it uses `DiagnosticCode.lowerCaseName`
rather than `DiagnosticCode.name`. This in itself has no effect (since
the map ignores case), but it paves the way for deprecating (and
eventually removing) the `DiagnosticCode.name` getter.
Change-Id: I6a6a6964f8ba149f883d1e9b2c9168a0929f7803
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/466184
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Changes the logic in `pkg/analysis_server_plugin` to use
`DiagnosticCode.lowerCaseName` instead of `DiagnosticCode.name`. This
ensures that diagnostic codes are matched in a case-insensitive
fashion.
This paves the way for deprecating (and eventually removing) the
`DiagnosticCode.name` getter.
Change-Id: I6a6a6964f4733c550f9c1060377e198808190acc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/466129
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Adjusts the logic in the lint `Registry` class so that lint names are
matched in case-insensitive fashion. For the most part this is
accomplished by adding calls to `.toLowerCase()` inside the `Registry`
class, preserving its API.
For the `enabled` method, preserving the API would have been a pain (I
would have had to translate the keys in the `ruleConfigs` parameter to
lower case). So instead I added an assertion to verify that the keys
were lower case, and pushed the responsibility to the callers to
create lower case keys.
This paves the way for a follow-up CL that will translate diagnostic
codes to `lower_snake_case` conventions.
It also solves a longstanding problem with the mixed case lint rules
`no_runtimeType_toString`, `prefer_for_elements_to_map_fromIterable`,
`prefer_iterable_whereType`. Previously, the user had to carefully
imitate the capitalization of the lint rules when specifying them in
the `analysis_options.yaml` file, even though `// ignore:` comments
for the lints matched in case insensitive fashion. With this CL, the
lint rule names in `analysis_options.yaml` are matched in case
insensitive fashion as well.
Change-Id: I6a6a6964d83241e49878bbf96ef9b94cbb12098b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/465964
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Prior to this CL, the analyzer's set of unignorable diagnostic code
names (`AnalysisOptionsImpl.unignorableDiagnosticCodeNames`) was
constructed in the following way:
- For each entry in the `cannot-ignore` section of the
`analysis_options.yaml` file that matches the name of a severity
code, the names of all diagnostic codes with matching severities
were added (1).
- All other entries in the `cannot-ignore` section were converted to
upper case and then added (2).
However, diagnostic codes associated with lints are named using
`lower_snake_case`, while analyzer diagnonstic codes are named using
`UPPER_SNAKE_CASE`.
Some of the logic that consumed
`AnalysisOptionsImpl.unignorableDiagnosticCodeNames` didn't account
for this, resulting in some subtle bugs:
- When the resolved correction producer base class
`_BaseIgnoreDiagnostic` attempted to figure out if a diagnostic was
unignorable, it compared elements of
`unignorableDiagnosticCodeNames` to `DiagnosticCode.name`, which
meant that it would successfully recognize non-lint codes as
unignorable, but it would only recognize that a lint code was
unignorable if it was included in `unignorableDiagnosticCodeNames`
by putting the name of a severity in the `cannot-ignore` section of
the `analysis_options.yaml` file.
- When `LibraryAnalyzer._filterIgnoredDiagnostics` attempted to block
unignorable diagnostics from being ignored, it compared elements of
`unignorableDiagnosticCodeNames` to `DiagnosticCode.name`,
`DiagnosticCode.uniqueName`, and
`DiagnosticCode.name.toUpperCase()`. This worked, however the
comparison to `DiagnosticCode.uniqueName` had no effect. Note that
values of `DiagnosticCode.uniqueName` always take the form
`ClassName.snake_case_diagnostic_code` or
`ClassName.SNAKE_CASE_DIAGNOSTIC_CODE`. It's impossible for any of
the values added to
`AnalysisOptionsImpl.unignorableDiagnosticCodeNames` to ever match
this, because (1) always adds values of `DiagnosticCode.name` (which
never contains a `.`), and (2) always adds strings that have been
converted to upper case.
- When `IgnoreValidator.reportErrors` attempted to report unignorable
and duplicate entries, it compared elements of
`unignorableDiagnosticCodeNames` to `IgnoredDiagnosticName.name`,
which is always lower case. That meant that it would only recognize
that a diagnostic code was unignorable if the diagnostic code was
associated with a lint and was included in
`unignorableDiagnosticCodeNames` by putting the name of a severity
in the `cannot-ignore` section of the `analysis_options.yaml`
file. (Note, however, that this bug was unobservable because the
reporting of the `unignorable_ignore` diagnostic is currently
disabled; I will address this in a follow-up CL.)
- Additionally, the logic to populate
`AnalysisOptionsImpl.unignorableDiagnosticCodeNames` based on a
severity code had a bug in its handling of error processors: if one
or more error processors were used to change the severity of a
diagnostic, then entries would be added to
`AnalysisOptionsImpl.unignorableDiagnosticCodeNames` corresponding
to both the original and the new severity.
These buggy behaviors have been fixed by:
- Streamlining and simplifying the logic that builds
`AnalysisOptionsImpl.unignorableDiagnosticCodeNames`, and ensuring
that all strings added to it are all lower case.
- Changing all logic that checks whether a string is contained in
`AnalysisOptionsImpl.unignorableDiagnosticCodeNames` so that it
first converts that string to lower case.
- Removing the ineffective logic in
`LibraryAnalyzer._filterIgnoredDiagnostics` that attempted to
compare elements of `unignorableDiagnosticCodeNames` to
`DiagnosticCode.uniqueName`.
Change-Id: I6a6a6964d89c139492dbb11d8ba3b2d33c0e2ee8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/462864
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Currently, diagnostic codes associated with lints are named using
`lower_snake_case`, while analyzer diagnonstic codes are named using
`UPPER_SNAKE_CASE`.
However, when the analyzer builds instances of the `ErrorProcessor`
class, it always uses `UPPER_SNAKE_CASE` names.
Some pieces of logic that matched up `ErrorProcessor`s to diagnostic
codes accounted for this difference; others didn't.
This led to a some buggy behaviors:
- If an instance of `ErrorProcessor` got constructed outside of the
analyzer (by an analyzer client using the analyzer public API), and
it supplied a `lower_snake_case` name, then
`ErrorProcessor.appliesTo` would only successfully match if the name
referred to a lint.
- The resolved correction producer base class `_BaseIgnoreDiagnostic`
(which forms the basis for the quick fixes "Ignore '...' in
`analysis_options.yaml`", "Ignore '...' for this line", and "Ignore
'...' for the whole file") would only notice that a diagnostic was
unignorable if the case matched exactly. In practice, this meant
that when operating on instances of `ErrorProcessor` created by the
analyzer, it wouldn't properly handle lints.
These buggy behaviors have been fixed by:
- Changing the `ErrorProcessor` constructor to always convert the
`code` to lower case.
- Changing all references to `ErrorProcessor.code` to assume lower
case.
Change-Id: I6a6a69645284f646e0c070fc2b55c4a90203d74a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/462863
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
No more flag, always parse into the new AST, always visit new AST nodes,
always return them as child entities, parent-child structure reflects
the new AST.
So, use `namePart` and `body` where possible. Deprecate previous
properties.
This is still de jure a breaking change, because `parent` of deprecated
properties changes. De facto this required very few changes in google3.
Once this CL lands, I will publish `analyzer 10.0.0`, migrate everything
to new properties, delete deprecated properties, and publish `analyzer
11.0.0`.
Maybe deprecate `NamedCompilationUnitMember.name` and migrate to
subclass specific `name` or `namePart` properties before publishing
`analyzer 10.0.0`. This part is not breaking per se.
* Deprecations in `ClassDeclaration`:
* Properties `leftBracket`, `members`, `rightBracket` are deprecated, use `body` instead.
* Properties `name`, `typeParameters` are deprecated, use `namePart` instead.
* Deprecations in `EnumDeclaration`:
* Properties `leftBracket`, `constants`, `members`, `rightBracket` are deprecated, use `body` instead.
* Properties `name`, `typeParameters` are deprecated, use `namePart` instead.
* Deprecations in `ExtensionDeclaration`:
* Properties `leftBracket`, `members`, `rightBracket` are deprecated, use `body` instead.
* Deprecations in `ExtensionTypeDeclaration`:
* Properties `leftBracket`, `constants`, `members`, `rightBracket` are deprecated, use `body` instead.
* Properties `constKeyword`, `name`, `representation`, `typeParameters` are deprecated,
use `primaryConstructor` instead.
* **Breaking Change:** While the deprecated members mentioned above still exist in the AST,
their parent nodes have changed. This means that code relying on specific parent-child
relationships for these nodes might break.
Bug: https://github.com/dart-lang/sdk/issues/61701
Change-Id: Ic48104da8b029c9b454bbd2336574b7823025565
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/461841
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Konstantin Shcheglov