FEDX-165 Upgrade to analyzer 5 by greglittlefield-wf · Pull Request #835 · Workiva/over_react

greglittlefield-wf · 2023-09-05T19:42:42Z

Motivation

The analyzer dependency in over_react and its analyzer plugin are outdated and need to be upgraded. We want to jump from analyzer 2 to analyzer 5.

Changes

Update CI's analyzer matrix, start running on 2.19

Root over_react package

Update to analyzer 5 and update related dependencies
Address analyzer breakages, notably:
- Removal of ClassOrMixinDeclaration
  - In most cases this was resolved by leveraging the existing ClassishDeclaration utility class that adapts ClassDeclaration, MixinDeclaration, and ClassTypeAlias (class Foo = Bar with Baz) into a single API
- Change of name from Identifier to Token in many cases
- Change from TypeName to NamedType
- Removal of getMethod
Update comment placement in generated code so that it formats consistently in dart_style versions resolved to in Dart 2.18 and 2.19
- This made it easier for us to verify in CI that there are no generated changes that aren't checked in, and also the placement is an improvement over the old placement

Analyzer plugin subpackage

Update to analyzer 5 and update related dependencies (notably including analyzer_plugin)
Address analyzer breakages, notably:
- Change of name from Identifier to Token in many cases
  - This affected NodeLocator/NodeLocator2 which were copied from the analyzer package, so those implementations were synced with their analyzer 5 versions
- Removal of ClassOrMixinDeclaration
- Elements for mixins are no longer ClassElement, but rather its supertype, InterfaceElement
- References to types are no longer visited as Identifier, but rather non-Expression NamedType
  - This mainly affected exhaustive_deps, which needed to be updated to handle both of these cases
Fix some built-in analysis warnings in test cases
Address analyzer_plugin breakages.

Mainly, they moved away from having ServerPlugin subclasses instantiate their own AnalysisDrivers, in favor of the base class managing its own AnalysisContextCollection (link to main changes). This caused our analyzer setup both in the main plugin and in tests to become invalid, and needed to be refactored.

Changes to address this:
- Remove invalid createAnalysisDriver override, and instead use the AnalysisContextCollection provided by the base class
- Wire up diagnostics to the new analyzeFile hook, which also seems to be called for non-"priority" files, allowing us to remove the workarounds we had in place to ensure diagnostics were run on all files within a package (see _updatePriorityFiles removal).
- Refactor tests to use a slimmed down copy of over_react_codemod's SharedAnalysisContext. More details:
  
  Our tests were pretty tightly coupled to that old implementation, and involved creating our own analysis driver. This setup involved a mock Dart SDK, a memory-based resource provider that required copying code needed for resolved analysis (like over_react and transitive dependencies) and a custom package config.
  
  This setup was pretty complicated, involved src imports, and also needed to be migrated in response to Dart SDK changes. As opposed to trying to update it, I opted to refactor these to use the approach we use in over_react_codemod's tests, which involves creating real AnalysisContextCollections and sharing them across tests for performance.

Release Notes

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review:

QA Checklist

Tests were updated and provide good coverage of the changeset and other affected code
Manual testing was performed if needed
- Steps from PR author:
  - Verify in playground that:
    - Diagnostics, fixes, and assists work at a base level
    - Diagnostics, fixes, and assists work after when modifying files, to ensure the plugin isn't using stale analysis results. This is best tested by adding new code to an existing file.
  - Verify in a larger repo that diagnostics show for all files, and not just "priority files" (viewed by opening Analyzer Diagnostics from your IDE, selecting the "Contexts" tab, and scrolling down to the "Priority files" section)
- Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

A Frontend Frameworks Design member has reviewed these changes
There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
For release PRs - Version metadata in Rosie comment is correct

aviary-wf · 2023-09-05T19:43:46Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

…ces in CI This comment used to go at the end of the line, b but that caused formatting difference between dart_style 2.2.5 and 2.3.2 when the name was prefixed, causing CI failures due to the output sometimes being different, so we'll put it on the previous line for now as an easy fix.

…in breakages

This code doesn't currently run, so this implementation may not be correct.

Before this change, each test would spin up a new AnalysisContextCollection, which was much too slow

greglittlefield-wf · 2023-09-08T19:06:17Z

tools/analyzer_plugin/lib/src/plugin.dart


-abstract class OverReactAnalyzerPluginBase extends ServerPlugin
-    with
+mixin OverReactAnalyzerPluginBase


I made this into a mixin so that we could extend from a stubbed version of ServerPlugin in tests

greglittlefield-wf · 2023-09-08T19:21:20Z

tools/analyzer_plugin/lib/src/diagnostic/exhaustive_deps.dart

Whitespace-agnostic diff is helpful for this file.

robbecker-wf · 2023-09-08T19:20:21Z

pubspec.yaml

-  # Pin mockito to work around https://github.com/dart-lang/mockito/issues/552
-  # until we can get to 5.3.1, which requires a newer analyzer.
-  mockito: '>=5.0.0 <5.3.0'
+  mockito: ^5.3.1


dep changes all look right

robbecker-wf · 2023-09-08T19:38:02Z

lib/src/builder/parsing/ast_util.dart

+/// An extension that supports APIs that changed from [Identifier] to [Token],
+/// in order to cut down on diffs in the analyzer 5 upgrade (and subsequent
+/// merge conflicts with the null-safety branch.
+///
+/// TODO remove this and inline the [name] member.
+extension NameIdentifierTokenCompat on Token {
+  String get name => lexeme;
+}
+


oh, good trick! So, then after this merges there's follow up work to inline the lexeme according to that TODO a few lines up?

robbecker-wf · 2023-09-08T19:41:50Z

lib/src/builder/parsing/declarations.dart

-        (a) => a.nodeHelper.mixins.map((name) => name.name).toList(),
-        (b) => [b.name],
+  List<String> get allPropsMixins => this.switchCase(
+        (a) => a.nodeHelper.mixins.map((name) => name.name.name).toList(),


Wow ... name.name.name. Not much to do about that but it's funny.

For some reason, the call to getResolvedUnitResult within DartNavigationMixin results in an InconsistentAnalysisException only in VS Code and not in JetBrains IDES.

robbecker-wf · 2023-09-08T22:54:25Z

QA+1

tested in both VS Code and IntelliJ
verified the analyzer plugin works the same as before

aaronlademann-wf

+1 to bdc3467

robbecker-wf · 2023-09-15T18:16:59Z

@Workiva/release-management-p

rmconsole-wf

+1 from RM

Sourcegraph and others added 7 commits August 30, 2023 22:10

"Update dev dependencies for analyzer 2"

18d6877

pub get and format

bdc3467

Upgrade main package to analyzer 5

a7a456f

Upgrade main package to analyzer 5

35243a2

Fix test file not getting run due to it missing the _test suffix

474bb3c

Update generated code

b366b4f

Update CI analyzer matrix, run on 2.19, stop running on 2.13.4

e82647a

rmconsole-wf added the Merge Requirements Unmet label Sep 5, 2023

rmconsole-wf added the Skynet 2.0 Failed label Sep 5, 2023

greglittlefield-wf added 5 commits September 5, 2023 13:35

Update generated files with new comment placements

c8e8e5a

Update gold files with new comment placements

780119a

Update Dockerfile Dart version

f2ffd7d

Upgrade analyzer and analyzer_plugin, first stab at fixing ServerPlug…

1dce39a

…in breakages

robbecker-wf changed the title ~~Upgrade to analyzer 5~~ FEDX-165 Upgrade to analyzer 5 Sep 6, 2023

greglittlefield-wf added 13 commits September 6, 2023 11:03

Address analyzer breakages

6a852c4

Address analyzer breakages in boilerplate_validator

946cf1f

This code doesn't currently run, so this implementation may not be correct.

Refactor test setup in response to analyzer_plugin changes

ea6bf32

Fix bad usages of ClassElement now that mixins elements don't use it

620db2f

Update plugin test setup to use shared analysis context

2a299d9

Before this change, each test would spin up a new AnalysisContextCollection, which was much too slow

Fix ast_util tests

9d2fd9b

Support tests with custom analysis_options.yaml

6d3583b

Sync NodeLocator(2) and tests from analyzer package

4779728

Fix boilerplate_util tests

511ca2a

Fix analysis warnings in test cases

4e961de

Fix references to types no longer being Identifiers

6070bd6

Fix warning

d798b10

Revert local pubspec.yaml changes

2d80fd4

greglittlefield-wf force-pushed the analyzer-5 branch from 7a1a718 to 25ab621 Compare September 7, 2023 23:59

Clean up and document StubChannel

edc08c6

greglittlefield-wf marked this pull request as ready for review September 8, 2023 18:45

Remove unused dependency

d49d36a

greglittlefield-wf force-pushed the analyzer-5 branch from b3b93cb to d49d36a Compare September 8, 2023 18:48

greglittlefield-wf requested a review from robbecker-wf September 8, 2023 18:48

greglittlefield-wf commented Sep 8, 2023

View reviewed changes

tools/analyzer_plugin/lib/src/diagnostic/exhaustive_deps.dart

Copy link

Contributor Author

greglittlefield-wf Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace-agnostic diff is helpful for this file.

robbecker-wf reviewed Sep 8, 2023

View reviewed changes

greglittlefield-wf added 3 commits September 8, 2023 13:18

Clean up unused_local_variable ignore

c10f1d3

Remove outdated comment

8adc8ed

Remove unused NavigationMixin to fix VS Code

b132fe3

For some reason, the call to getResolvedUnitResult within DartNavigationMixin results in an InconsistentAnalysisException only in VS Code and not in JetBrains IDES.

rmconsole-wf added the Skynet 2.0 Failed label Sep 8, 2023

Fix missed NavigationMixin

e52d994

rmconsole-wf removed the Skynet 2.0 Failed label Sep 8, 2023

greglittlefield-wf added 2 commits September 8, 2023 14:39

Don't analyze non-Dart files

9bcf9b0

Don't warn on keys based on enum values

2aaba91

robbecker-wf approved these changes Sep 8, 2023

View reviewed changes

robbecker-wf added the Hold Hold merges label Sep 8, 2023

aaronlademann-wf approved these changes Sep 13, 2023

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Sep 13, 2023

robbecker-wf removed the Hold Hold merges label Sep 15, 2023

rmconsole-wf added the RM Ready label Sep 15, 2023

rmconsole-wf approved these changes Sep 15, 2023

View reviewed changes

rmconsole3-wf merged commit 06957aa into master Sep 15, 2023

rmconsole3-wf deleted the analyzer-5 branch September 15, 2023 18:17

This was referenced Sep 15, 2023

RM-208146 RM-208145 Release over_react 4.10.1 #837

Closed

RM-208146 RM-208145 Release over_react 4.10.1 #839

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEDX-165 Upgrade to analyzer 5#835

FEDX-165 Upgrade to analyzer 5#835
rmconsole3-wf merged 45 commits intomasterfrom
analyzer-5

greglittlefield-wf commented Sep 5, 2023 •

edited

Loading

Uh oh!

aviary-wf commented Sep 5, 2023

Uh oh!

greglittlefield-wf Sep 8, 2023

Uh oh!

greglittlefield-wf Sep 8, 2023

Uh oh!

robbecker-wf Sep 8, 2023

Uh oh!

robbecker-wf Sep 8, 2023

Uh oh!

robbecker-wf Sep 8, 2023

Uh oh!

robbecker-wf commented Sep 8, 2023

Uh oh!

aaronlademann-wf left a comment

Uh oh!

robbecker-wf commented Sep 15, 2023

Uh oh!

rmconsole-wf left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

greglittlefield-wf commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Root over_react package

Analyzer plugin subpackage

Release Notes

Review

QA Checklist

Merge Checklist

Uh oh!

aviary-wf commented Sep 5, 2023

Security Insights

Action Items

Uh oh!

greglittlefield-wf Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

greglittlefield-wf Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

robbecker-wf Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

robbecker-wf Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

robbecker-wf Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

robbecker-wf commented Sep 8, 2023

Uh oh!

aaronlademann-wf left a comment

Choose a reason for hiding this comment

Uh oh!

robbecker-wf commented Sep 15, 2023

Uh oh!

rmconsole-wf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

greglittlefield-wf commented Sep 5, 2023 •

edited

Loading