FED-1885: Prop requiredness codemod #290

greglittlefield-wf · 2024-07-31T23:21:12Z

Motivation

over_react consumers migrating to null safety will need to update each prop to be either required or optional.

Determining the correct requiredness for each prop is tedious to do manually, so we want to help automate this process, similar to how Dart's null safety migrator tool helps determine correct nullability for other code.

Changes

Add null_safety_required_props executable with the following commands (more info below):
- collect - Collects requiredness data for all OverReact props based on usages in the specified packages and all their transitive dependencies.
- codemod - Adds null safety migrator hints to OverReact props using prop requiredness data from 'collect' command.
Add tests
- End-to-end tests for codemod command using collect command data: required_props_collect_and_codemod_test.dart
- Test for data collection in various scenarios collect command: required_props_collect_test.dart
- Unit tests for new argument removal utilities
Misc changes
- dfdeacf Fix required hint util checking comments for subsequent nodes
- Split out getAllPropsClassesOrMixins method from getAllProps
- Set up json_serializable codegen, with optimized *.sg.dart build.yaml configuration and CI check for outdated generated files

`collect` command

High-level overview diagram

Help output, containing instructions

Collects requiredness data for all OverReact props based on usages in the specified packages and all their transitive dependencies.

Usage: null_safety_required_props collect [<options>] <package_spec> [<package_spec>...]
-h, --help                         Print this usage information.
    --raw-data-output-directory    An optional directory to output raw usage data file to.
-o, --output=<path>                The file to write aggregated results to.
                                   (defaults to "prop_requiredness.json")
    --verbose                      Enable verbose output.

Run "null_safety_required_props help" to see global options.

Supported package spec formats:
- Hosted pub package with optional version (uses latest if omitted):
    - `[email protected]:over_react`
    - `[email protected]:over_react#5.2.0`
- Git URL with optional revision:
    - `[email protected]:Workiva/over_react.git`
    - `https://github.com/Workiva/over_react.git`
    - `[email protected]:Workiva/over_react.git#5.2.0`
- Local file path:
    - `/path/to/over_react`
    - `file:///path/to/over_react`

Instructions
============

1. First, identify the least-common consumer(s) of OverReact components exposed by your package.

   (If all your package's components are private, you can skip the rest of this step,
   step and just use your package).

   For example, say we're dealing with package A, which is directly consumed by
   packages B, E, and F, and so on:

       A---B---C---D
       |\     /
       | E----
       \
        F---G---H

   The least-common consumers would be C (covers both B and E) and F, so we'd run:

      null_safety_required_props collect pub@…:C pub@…:F

   Note: if F were to re-export members of A, which could potentially get used
   in G, we'd do G instead of F.

      null_safety_required_props collect pub@…:C pub@…:G

   Alternatively, we could just run on D and H from the start, but if those
   packages include more transitive dependencies, then the analysis step of the
   collection process will take a bit longer.

2. If step 1 yielded more than one package, make sure all of them can resolve to
   the latest version of your package.

   If they can't, then data may be missing for recently-added props, or could be
   incorrect if props in your package were moved to different files.

   If you're not sure, try either:
   - Cloning those packages, ensuring they resolve to the latest locally,
     and providing them as local path package specs.
   - Running the command and verifying the package versions in the command output
     line up.

3. Run the 'null_safety_required_props collect' command with the packages from step 1, using
   one of the package specifier formats listed above.

4. Use the `codemod` command within the package you want to update
   (see that command's --help for instructions):

      cd my_package
      null_safety_required_props codemod --help

`codemod` command

Help output, containing instructions

Adds null safety migrator hints to OverReact props using prop requiredness data from 'collect' command.

Usage: null_safety_required_props codemod [<options>]
-h, --help                               Print this usage information.
    --prop-requiredness-data             The file containing prop requiredness data, collected via the 'over_react_codemod:collect' command.
                                         (defaults to "prop_requiredness.json")
    --[no-]trust-required-annotations    Whether to migrate @requiredProp and `@nullableRequiredProp` props to late required, regardless of usage data.
                                         Note that @requiredProp has no effect on function components, so these annotations may be incorrect.
                                         (defaults to on)
    --private-requiredness-threshold     The minimum rate (0.0-1.0) a private prop must be set to be considered required.
                                         (defaults to "0.95")
    --private-max-allowed-skip-rate      The maximum allowed rate (0.0-1.0) of dynamic usages of private mixins, for which data collection was skipped.
                                         If above this, all props in a mixin will be made optional (with a TODO comment).
                                         (defaults to "0.2")
    --public-requiredness-threshold      The minimum rate (0.0-1.0) a public prop must be set to be considered required.
                                         (defaults to "1")
    --public-max-allowed-skip-rate       The maximum allowed rate (0.0-1.0) of dynamic usages of public mixins, for which data collection was skipped.
                                         If above this, all props in a mixin will be made optional (with a TODO comment).
                                         (defaults to "0.05")

Codemod options
-v, --verbose                            Outputs all logging to stdout/stderr.
    --yes-to-all                         Forces all patches accepted without prompting the user. Useful for scripts.
    --fail-on-changes                    Returns a non-zero exit code if there are changes to be made. Will not make any changes (i.e. this is a dry-run).
    --stderr-assume-tty                  Forces ansi color highlighting of stderr. Useful for debugging.

Run "null_safety_required_props help" to see global options.

Instructions
============

1. First, run the 'collect' command to collect data on usages of props declared
   in your package (see that command's --help for instructions).

    null_safety_required_props collect --help

2. Run this command within the package you want to update:

    null_safety_required_props codemod

3. Inspect the TODO comments left over from the codemod. If you want to adjust
   any thresholds or re-collect data, discard changes before re-running the codemod.

4. Commit the changes made by the codemod.

5. Proceed with using the Dart null safety migrator tool to migrate your code.

6. Review TODO comments, adjusting requiredness if desired. You can use a
   find-replace with the following regex to remove them:

       ^ *// TODO\(orcm.required_props\):.+(?:\n *//  .+)*

Release Notes

Review

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

Please review:

QA Instructions

To run the executable locally, either:

Use dpx to run from this branch:

dpx github:Workiva/over_react_codemod#ref=prop-requiredness-codemod-with-collection:null_safety_required_props

Clone locally, and within over_react_codemod run dart pub global --source path .. Then, you can run null_safety_required_props or dart pub global run over_react_codemod:null_safety_required_props

QA steps:

CI passes
Verify help output for each subcommand is well-formatted
Verify all supported package spec formats (from help comment) work for collect command
Run collect and codemod commands for a package that has at least some public components and verify output looks good

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

test/executables/required_props_collect_and_codemod_test.dart

lib/src/dart3_suggestors/required_props/codemod/required_props_suggestor.dart

Specifically, there wasn't a test case for --no-trust-required-annotations making props optional based on usage data.

greglittlefield-wf · 2024-08-06T19:28:22Z

@aaronlademann-wf @sydneyjodon-wk Alright, all the items that came up during our meeting yesterday should be addressed now

sydneyjodon-wk

QA +1 I tested it out and it worked great!! This is awesome!

CI passes
Verify help output for each subcommand is well-formatted
Verify all supported package spec formats (from help comment) work for collect command
Run collect and codemod commands for a package that has at least some public components and verify output looks good
- Ran on graph_ui: https://github.com/Workiva/graph_ui/compare/test-prop-requiredness-codemod

sydneyjodon-wk · 2024-08-08T23:14:10Z

lib/src/dart3_suggestors/required_props/bin/collect.dart

+   For example, say we're dealing with package A, which is directly consumed by
+   packages B, E, and F, and so on:
+
+       ${r'A---B---C---D'}
+       ${r'|\     /'}
+       ${r'| E----'}
+       ${r'\'}
+       ${r' F---G---H'}
+
+   The least-common consumers would be C (covers both B and E) and F, so we'd run:
+
+      $invocationPrefix pub@…:C pub@…:F
+
+   Note: if F were to re-export members of A, which could potentially get used
+   in G, we'd do G instead of F.


Sorry I still don't get this. I understand the last Alternatively,... would just be doing wdesk or something, but I don't really understand what on the graph makes it okay to do C and F

Sorry about that; I was trying to capture that there may be branches of downstream consumers that aren't related. For instance, in this example, there's no package that transitively consumes both C and F.

Was that the part you weren't following, or maybe something else? Happy to hop on a call and talk through it too

And I'm totally down to adjust this example however needed to make things clearer

Yeah that makes sense, I guess I just don't get why it can be F and C and not always D and H

Oh it can be D and H, the idea is just that F and C is preferred (mainly because we get to skip analyzing code for D, H, G, and any other transitive dependencies not pulled in by F or C)

...And also increases the risk of mismatched dependencies talked about in the next step:

If step 1 yielded more than one package, make sure all of them can resolve to
the latest version of your package.

If they can't, then data may be missing for recently-added props, or could be
incorrect if props in your package were moved to different files.

oh wait I get it - I was misunderstanding the transitive dependency thing and that D and H aren't directly consuming A - cool cool that makes sense! Thank you!

Sweet, np! If you have any suggestions on how to make the wording or diagram more clear, I'm all ears!

aaronlademann-wf

+1 #nothinbutnits

This is impressive stuff @greglittlefield-wf!!!

When we don't have use for this specific codemod anymore - we should make sure to keep some of the code you wrote here because I could see a lot of it being reusable for other mods in the future!

aaronlademann-wf · 2024-08-09T20:35:44Z

lib/src/dart3_suggestors/required_props/codemod/required_props_suggestor.dart

+          final commentContents =
+              "$_todoWithPrefix: No data for prop; either it's never set,"
+              " all places it was set were on dynamic usages,"
+              " or requiredness data was collected on a version before this prop was added.";


Wow - talk about being thorough...

aaronlademann-wf · 2024-08-09T20:36:58Z

lib/src/dart3_suggestors/required_props/codemod/required_props_suggestor.dart

+extension on Token {
+  /// The offset of the next token or comment
+  /// (since comments can occur before the next token)
+  /// following this token, or null if nothing follows it.
+  int? get nextTokenOrCommentOffset {
+    final next = this.next;
+    if (next == null) return null;
+    final nextTokenOrComment = next.precedingComments ?? next;
+    return nextTokenOrComment.offset;
+  }
+}


I feel like we have this - or something like this - in like 5 or 6 codemods lol

aaronlademann-wf · 2024-08-09T20:45:02Z

lib/src/dart3_suggestors/required_props/collect/aggregate.dart

+      final isSamePackage = usage.usagePackage == mixin.mixinPackage;
+
+      final categorizedStats = usageStatsByMixinId.putIfAbsent(
+          mixin.mixinId, CategorizedPropsMixinUsageStats.new);


TIL you can use new for a class constructor tearoff!??!?

Yup, as of language version 2.15!! Soo nice

aaronlademann-wf · 2024-08-09T20:46:32Z

lib/src/dart3_suggestors/required_props/collect/aggregate.dart

+    ),
+  );
+
+  logger.finer('Aggregating final results...');


aaronlademann-wf · 2024-08-09T20:49:20Z

lib/src/dart3_suggestors/required_props/collect/collect.dart

+
+    logProgress();
+
+    //logger.finest('Processing ${unitResult.uri}');


aaronlademann-wf · 2024-08-09T20:50:31Z

lib/src/dart3_suggestors/required_props/collect/collect.dart

+              .where((element) => element.isGetter)
+              // Note that this is public relative to the library, not necessarily the package.
+              .where((element) => element.isPublic)


#supernit

Suggested change

.where((element) => element.isGetter)

// Note that this is public relative to the library, not necessarily the package.

.where((element) => element.isPublic)

.where((element) => element.isGetter && element.isPublic)

Yeah a lot of times I like splitting these out separately for organizational purposes

aaronlademann-wf · 2024-08-09T20:51:33Z

lib/src/dart3_suggestors/required_props/collect/collect.dart

+      final assignedPropNames = assignedProps.map((p) => p.name.name).toSet();
+      final unaccountedForPropNames = {...assignedPropNames};
+
+      // TODO maybe store mixin metadata separately?


aaronlademann-wf · 2024-08-09T20:55:26Z

pubspec.yaml

@@ -8,30 +8,35 @@ description: >


 environment:
-  sdk: '>=2.12.0 <3.0.0'
+  sdk: '>=2.15.0 <3.0.0'


Just curious, why not 2.19.6?

Yeah in this case I probably could have raised it higher, but didn't wanna mess with any potential breakages caused by newer language versions (particularly 2.18).

If I did raise it, I'd probably 2.19.0 to not unnecessarily restrict it, unless the package really needed the patches

greglittlefield-wf · 2024-08-10T00:02:18Z

@Workiva/release-management-p

rmconsole-wf

+1 from RM

greglittlefield-wf added 30 commits July 16, 2024 10:59

Fix required hint util checking comments for subsequent nodes

dfdeacf

Add constant for late hint

1ddf484

First stab at prop requiredness codemod, using external data

1660af4

Update aggregated data format, consume updates

d1655e2

Factor in isPublic and skip rate, default to optional when no data

c07c785

Improve reason strings

514a86b

Fix no-data case updating non-props

d28974d

Add helpful todo for classes that were skipped

0f7d718

Handle missing data case

c1b4df6

Update args with recommender options

e4f4cdd

Re-sync aggregated data format

4ef3d4e

Clarify message

b95b461

Format

2e0247d

Don't emit comments on individual props if the whole mixin was skipped

6bac8f9

Fix inverted boolean

f9b2c58

Honor required prop annotations under flag

584e80b

Remove requiredProp annotations

60eb48f

Make TODOs consistent

33b45c2

Reorganize code

6a28031

Add copyright comments

f058c31

Indicate copied files

6143eb9

Support negatable args

4cbb5a6

Add tests, fix bug in multi-arg option logic

fcf1e5d

Add required props data collection scripts

b144727

Format

6f94adc

Add progress log back in

264f767

Log package specs

f3ded7b

Add output option to aggregate script

b6f18ed

Allow hosted pub packages in collection script

d4719a7

Add help text to collect script

ff66c0c

greglittlefield-wf marked this pull request as ready for review August 5, 2024 19:35

greglittlefield-wf requested review from aaronlademann-wf and sydneyjodon-wk August 5, 2024 19:46

greglittlefield-wf commented Aug 5, 2024

View reviewed changes

test/executables/required_props_collect_and_codemod_test.dart Outdated Show resolved Hide resolved

greglittlefield-wf commented Aug 5, 2024

View reviewed changes

lib/src/dart3_suggestors/required_props/codemod/required_props_suggestor.dart Outdated Show resolved Hide resolved

lib/src/dart3_suggestors/required_props/codemod/required_props_suggestor.dart Outdated Show resolved Hide resolved

greglittlefield-wf added 4 commits August 6, 2024 09:51

Improve test coverage of required annotation tests

ded4368

Specifically, there wasn't a test case for --no-trust-required-annotations making props optional based on usage data.

Align class name with file name and convention

0785240

Add tests for existing hints, don't short-circuit on non-nullable hints

c223666

Add tests for existing hints, don't short-circuit on non-nullable hints

407ea41

greglittlefield-wf force-pushed the prop-requiredness-codemod-with-collection branch from 6182a4a to 407ea41 Compare August 6, 2024 17:03

greglittlefield-wf added 8 commits August 6, 2024 10:10

Clean up .package-cache references, used same temp directory for Git

cbc86da

Use dart_dev for formatting so that we can exclude files

900191c

Format

5bf1fc9

Improve instructions, add note about mismatched versions

eda7140

Indicate when using the latest version of a Pub package

50b6fc7

Display package versions from pubspec.lock, improve logs

dc5cb89

Update instructions about package mismatches

db2dc7e

Format

6393501

greglittlefield-wf mentioned this pull request Aug 8, 2024

FED-3009 Update migration guide with required props codemod instructions Workiva/over_react#935

Merged

7 tasks

sydneyjodon-wk approved these changes Aug 8, 2024

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Aug 8, 2024

aaronlademann-wf approved these changes Aug 9, 2024

View reviewed changes

rmconsole-wf added the RM Ready label Aug 10, 2024

rmconsole-wf approved these changes Aug 10, 2024

View reviewed changes

rmconsole2-wf merged commit d76eaa2 into master Aug 10, 2024
6 checks passed

rmconsole2-wf deleted the prop-requiredness-codemod-with-collection branch August 10, 2024 00:02

rmconsole-readonly-wk mentioned this pull request Aug 12, 2024

RM-260254 Release over_react_codemod 2.33.0 #292

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FED-1885: Prop requiredness codemod #290

FED-1885: Prop requiredness codemod #290

greglittlefield-wf commented Jul 31, 2024 •

edited

Loading

greglittlefield-wf commented Aug 6, 2024

sydneyjodon-wk left a comment

sydneyjodon-wk Aug 8, 2024

greglittlefield-wf Aug 8, 2024

sydneyjodon-wk Aug 8, 2024 •

edited

Loading

greglittlefield-wf Aug 8, 2024

greglittlefield-wf Aug 8, 2024

sydneyjodon-wk Aug 9, 2024

greglittlefield-wf Aug 9, 2024

aaronlademann-wf left a comment

aaronlademann-wf Aug 9, 2024

aaronlademann-wf Aug 9, 2024

aaronlademann-wf Aug 9, 2024

greglittlefield-wf Aug 10, 2024

aaronlademann-wf Aug 9, 2024

aaronlademann-wf Aug 9, 2024

aaronlademann-wf Aug 9, 2024

greglittlefield-wf Aug 10, 2024

aaronlademann-wf Aug 9, 2024

aaronlademann-wf Aug 9, 2024

greglittlefield-wf Aug 9, 2024

greglittlefield-wf commented Aug 10, 2024

rmconsole-wf left a comment


		logProgress();

		//logger.finest('Processing ${unitResult.uri}');

FED-1885: Prop requiredness codemod #290

FED-1885: Prop requiredness codemod #290

Conversation

greglittlefield-wf commented Jul 31, 2024 • edited Loading

Motivation

Changes

collect command

codemod command

Release Notes

Review

QA Instructions

Merge Checklist

greglittlefield-wf commented Aug 6, 2024

sydneyjodon-wk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sydneyjodon-wk Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlademann-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglittlefield-wf commented Aug 10, 2024

rmconsole-wf left a comment

Choose a reason for hiding this comment

greglittlefield-wf commented Jul 31, 2024 •

edited

Loading

`collect` command

`codemod` command

sydneyjodon-wk Aug 8, 2024 •

edited

Loading