-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FEDX-165 Upgrade to analyzer 5 #835
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
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.
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
7a1a718
to
25ab621
Compare
b3b93cb
to
d49d36a
Compare
|
||
abstract class OverReactAnalyzerPluginBase extends ServerPlugin | ||
with | ||
mixin OverReactAnalyzerPluginBase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this into a mixin so that we could extend from a stubbed version of ServerPlugin
in tests
There was a problem hiding this comment.
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.
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dep changes all look right
/// 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; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
(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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
QA+1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to bdc3467
@Workiva/release-management-p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from RM
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
Root over_react package
class Foo = Bar with Baz
) into a single APIAnalyzer plugin subpackage
Update to analyzer 5 and update related dependencies (notably including analyzer_plugin)
Address analyzer breakages, notably:
Fix some built-in analysis warnings in test cases
Address analyzer_plugin breakages.
Mainly, they moved away from having ServerPlugin subclasses instantiate their own
AnalysisDriver
s, in favor of the base class managing its ownAnalysisContextCollection
(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 theAnalysisContextCollection
provided by the base classWire 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
AnalysisContextCollection
s 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
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: