Skip to content
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

Merged
merged 45 commits into from
Sep 15, 2023
Merged

FEDX-165 Upgrade to analyzer 5 #835

merged 45 commits into from
Sep 15, 2023

Conversation

greglittlefield-wf
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf commented Sep 5, 2023

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
Copy link

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.
@robbecker-wf robbecker-wf changed the title Upgrade to analyzer 5 FEDX-165 Upgrade to analyzer 5 Sep 6, 2023
@greglittlefield-wf greglittlefield-wf marked this pull request as ready for review September 8, 2023 18:45

abstract class OverReactAnalyzerPluginBase extends ServerPlugin
with
mixin OverReactAnalyzerPluginBase
Copy link
Contributor Author

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

Copy link
Contributor Author

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
Copy link
Member

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

Comment on lines +109 to +117
/// 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;
}

Copy link
Member

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(),
Copy link
Member

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.
@robbecker-wf
Copy link
Member

QA+1

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

@robbecker-wf robbecker-wf added the Hold Hold merges label Sep 8, 2023
Copy link
Contributor

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

+1 to bdc3467

@robbecker-wf
Copy link
Member

@Workiva/release-management-p

Copy link

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

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

+1 from RM

@rmconsole3-wf rmconsole3-wf merged commit 06957aa into master Sep 15, 2023
6 checks passed
@rmconsole3-wf rmconsole3-wf deleted the analyzer-5 branch September 15, 2023 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants