Skip to content

Conversation

@greglittlefield-wf
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf commented Jul 22, 2021

Motivation

The build for the analyzer plugin was failing in Dart 2.13 with errors that seemed like null safety was being incorrectly opted into.

Example: https://github.com/Workiva/over_react/runs/3306575416

I forget exactly why, but increasing analyzer_plugin (and thus increasing the resolvable upper bound of analyzer) seem to fix the issue.

Changes

  • Increase upper bound of analyzer_plugin (thus also allowing newer versions of analyzer)
  • Fix issues caused by analyzer breakages
    • (e.g., analyzer 0.40.0 made getDisplayString's withNullability arg required)
  • Address deprecated API usages
  • Copy over analyzer APIs imported from src directories that were removed/moved in newer analyzer versions
  • Update build to only run the format check in a single Dart version
  • Remove mockito dependency since its builder was timing out test runs in newer Dart SDKs for some reason

Note this gets CI passing, but for some reason, diagnostics aren't showing up in the Dart Analysis panel of IDEs 😕 . Other things like suggested edits (e.g., "Add ref") work as expected. The AnalysisErrorsParams notification being sent by the plugin seems to be getting constructed properly with the correct errors and gets sent, but for some reason the analysis server isn't piping it through... Update: upgrading to analyzer_plugin ^0.6.0 (and analyzer ^1.7.0) fixes the issue! We'll do that in a separate PR, since upgrading those dependencies in over_react and over_react_analyzer_plugin is going to take some effort.

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:
      • Analyzer plugin job runs tests, and passes in all SDKs
    • 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 Client Platform 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

@aviary2-wf
Copy link

aviary2-wf commented Jul 22, 2021

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.

@greglittlefield-wf greglittlefield-wf changed the title Fix analyzer plugin stable Fix analyzer plugin build in Dart stable Aug 24, 2021
run: |
dart tool/travis_link_plugin_deps.dart
cd ./tools/analyzer_plugin
./tool/travis.sh
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 broke this up so that we could more easily conditionally run formatting step

@greglittlefield-wf greglittlefield-wf marked this pull request as ready for review August 24, 2021 23:17
cd ./tools/analyzer_plugin
./tool/travis.sh
- name: Validate dependencies
run: pub run dependency_validator --no-fatal-pins -i analyzer,build_runner,build_web_compilers,built_value_generator
Copy link
Contributor

Choose a reason for hiding this comment

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

fyi if you want to upgrade to dependency_validator v2 or v3, then it should detect the executable from build_runner and the builders from the last two, and then you'd only need to ignore analyzer which you could do statically. You'd also have to drop --no-fatal-pins and ignore whichever packages are ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

(probably out-of-scope since this is a small PR, just thought I'd throw it out there for future reference)

@@ -0,0 +1,584 @@
// From analyzer 0.39.17 src/dart/ast/utilities.dart
Copy link
Contributor

Choose a reason for hiding this comment

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

Were these utils removed from public entrypoints?

Comment on lines -6 to -9
// This is necessary for `ConstantEvaluator`. If that API is removed, it can just
// be copied and pasted into this analyzer package (if still needed).
// ignore: deprecated_member_use
import 'package:analyzer/analyzer.dart' show ConstantEvaluator;
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, looks like this answers my previous question. Did they replace this with an alternative or a better option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope ☹️

@greglittlefield-wf greglittlefield-wf mentioned this pull request Sep 10, 2021
7 tasks
@greglittlefield-wf greglittlefield-wf force-pushed the fix-analyzer-plugin-stable branch from a8b7b06 to 1861f36 Compare October 29, 2021 18:48
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

@aaronlademann-wf
Copy link
Contributor

@Workiva/release-management-pp

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

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.

7 participants