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

FED-1885: Prop requiredness codemod #290

Merged
merged 91 commits into from
Aug 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
91 commits
Select commit Hold shift + click to select a range
dfdeacf
Fix required hint util checking comments for subsequent nodes
greglittlefield-wf Jul 16, 2024
1ddf484
Add constant for late hint
greglittlefield-wf Jul 16, 2024
1660af4
First stab at prop requiredness codemod, using external data
greglittlefield-wf Jul 16, 2024
d1655e2
Update aggregated data format, consume updates
greglittlefield-wf Jul 16, 2024
c07c785
Factor in isPublic and skip rate, default to optional when no data
greglittlefield-wf Jul 16, 2024
514a86b
Improve reason strings
greglittlefield-wf Jul 16, 2024
d28974d
Fix no-data case updating non-props
greglittlefield-wf Jul 16, 2024
0f7d718
Add helpful todo for classes that were skipped
greglittlefield-wf Jul 18, 2024
c1b4df6
Handle missing data case
greglittlefield-wf Jul 18, 2024
e4f4cdd
Update args with recommender options
greglittlefield-wf Jul 22, 2024
4ef3d4e
Re-sync aggregated data format
greglittlefield-wf Jul 22, 2024
b95b461
Clarify message
greglittlefield-wf Jul 22, 2024
2e0247d
Format
greglittlefield-wf Jul 22, 2024
6bac8f9
Don't emit comments on individual props if the whole mixin was skipped
greglittlefield-wf Jul 22, 2024
f9b2c58
Fix inverted boolean
greglittlefield-wf Jul 22, 2024
584e80b
Honor required prop annotations under flag
greglittlefield-wf Jul 22, 2024
60eb48f
Remove requiredProp annotations
greglittlefield-wf Jul 22, 2024
33b45c2
Make TODOs consistent
greglittlefield-wf Jul 22, 2024
6a28031
Reorganize code
greglittlefield-wf Jul 22, 2024
f058c31
Add copyright comments
greglittlefield-wf Jul 22, 2024
6143eb9
Indicate copied files
greglittlefield-wf Jul 22, 2024
4cbb5a6
Support negatable args
greglittlefield-wf Jul 22, 2024
fcf1e5d
Add tests, fix bug in multi-arg option logic
greglittlefield-wf Jul 22, 2024
b144727
Add required props data collection scripts
greglittlefield-wf Jul 23, 2024
6f94adc
Format
greglittlefield-wf Jul 23, 2024
264f767
Add progress log back in
greglittlefield-wf Jul 23, 2024
f3ded7b
Log package specs
greglittlefield-wf Jul 23, 2024
b6f18ed
Add output option to aggregate script
greglittlefield-wf Jul 23, 2024
d4719a7
Allow hosted pub packages in collection script
greglittlefield-wf Jul 24, 2024
ff66c0c
Add help text to collect script
greglittlefield-wf Jul 24, 2024
4f674f2
Add output directory arg to collect script
greglittlefield-wf Jul 24, 2024
9cb548f
Fail collect if no packages are specified
greglittlefield-wf Jul 24, 2024
803be69
Add some tests for required prop data collection and aggregation
greglittlefield-wf Jul 24, 2024
44eecb0
Fix output file not including package name
greglittlefield-wf Jul 24, 2024
3178106
Add tests for visibility data, fix indirectlyPublic visibility
greglittlefield-wf Jul 24, 2024
5e82b7d
Add tests for props with usages in other packages
greglittlefield-wf Jul 30, 2024
043f4c5
Improve directory output
greglittlefield-wf Jul 30, 2024
7a69e89
Add test for non-factory usage filtering
greglittlefield-wf Jul 30, 2024
5821b84
Fix test setup to use test_consuming_package
greglittlefield-wf Jul 30, 2024
bdb3101
Aggregate data by default in collect.dart
greglittlefield-wf Jul 30, 2024
35bba9f
Include stdout on process failure
greglittlefield-wf Jul 30, 2024
ccec045
Add end-to-end test for required props data collection and codemod
greglittlefield-wf Jul 30, 2024
637fd19
Make collect and codemod subcommands of a single executable
greglittlefield-wf Jul 30, 2024
95f8014
Improve invocation line of help output
greglittlefield-wf Jul 30, 2024
d6be7b5
Format
greglittlefield-wf Jul 31, 2024
bc5f078
Add some usage instructions
greglittlefield-wf Jul 31, 2024
935a191
Run pub get in codemod
greglittlefield-wf Jul 31, 2024
197e07d
Rename flag
greglittlefield-wf Jul 31, 2024
4a930d6
Fix inverted boolean
greglittlefield-wf Jul 31, 2024
361feb1
Add test coverage for required annotation logic and flag
greglittlefield-wf Jul 31, 2024
a6709c4
Clean up logging, add verbose flag
greglittlefield-wf Jul 31, 2024
46dacc3
More logging tweaks
greglittlefield-wf Jul 31, 2024
794f29e
Add coloring/formatting to logs
greglittlefield-wf Jul 31, 2024
09dad6d
Log more with verbose:true
greglittlefield-wf Jul 31, 2024
055502a
Fix inverted boolean
greglittlefield-wf Jul 31, 2024
cd3c19c
Set up json_serializable, remove unused mockito whose builder was err…
greglittlefield-wf Jul 31, 2024
f72e4e2
Add CI check for generated changes
greglittlefield-wf Jul 31, 2024
52ff940
Merge remote-tracking branch 'origin/master' into prop-requiredness-c…
greglittlefield-wf Jul 31, 2024
e362ef7
Format
greglittlefield-wf Jul 31, 2024
b8726b9
Update test in response to formatting
greglittlefield-wf Jul 31, 2024
8ad3aad
Add missing dependency
greglittlefield-wf Jul 31, 2024
7c8a02a
Merge branch 'fixes-and-cleanup' into prop-requiredness-codemod-with-…
greglittlefield-wf Aug 1, 2024
1e55e90
Add tests for requiredness threshold options
greglittlefield-wf Aug 1, 2024
c85d6bd
Add tests for skip arguments
greglittlefield-wf Aug 1, 2024
f612d95
Add some doc comments
greglittlefield-wf Aug 1, 2024
bb959d1
Split out aggregation from its executable
greglittlefield-wf Aug 1, 2024
c898f84
Reorganize files
greglittlefield-wf Aug 1, 2024
bd6dbad
Reorganize/rename tests
greglittlefield-wf Aug 1, 2024
97556a0
Fix outdated name in pubspec
greglittlefield-wf Aug 1, 2024
d53328f
Don't write individual results to files by default
greglittlefield-wf Aug 2, 2024
d71085c
Clean up temporary folder when we're done with it
greglittlefield-wf Aug 2, 2024
55565be
Format
greglittlefield-wf Aug 2, 2024
db0f7e1
Fix deletion of non-empty directory failing
greglittlefield-wf Aug 2, 2024
c8f9d98
Merge remote-tracking branch 'origin/master' into prop-requiredness-c…
greglittlefield-wf Aug 2, 2024
134e15c
Add copyright comments
greglittlefield-wf Aug 5, 2024
6ec1504
Update command descriptions
greglittlefield-wf Aug 5, 2024
83a5dc3
Add and improve instructions
greglittlefield-wf Aug 5, 2024
1bb0891
Consolidate copies of get_all_props.dart
greglittlefield-wf Aug 5, 2024
f0f7d11
Remove unused vendored file
greglittlefield-wf Aug 5, 2024
ded4368
Improve test coverage of required annotation tests
greglittlefield-wf Aug 6, 2024
0785240
Align class name with file name and convention
greglittlefield-wf Aug 6, 2024
c223666
Add tests for existing hints, don't short-circuit on non-nullable hints
greglittlefield-wf Aug 6, 2024
407ea41
Add tests for existing hints, don't short-circuit on non-nullable hints
greglittlefield-wf Aug 6, 2024
cbc86da
Clean up .package-cache references, used same temp directory for Git
greglittlefield-wf Aug 6, 2024
900191c
Use dart_dev for formatting so that we can exclude files
greglittlefield-wf Aug 6, 2024
5bf1fc9
Format
greglittlefield-wf Aug 6, 2024
eda7140
Improve instructions, add note about mismatched versions
greglittlefield-wf Aug 6, 2024
50b6fc7
Indicate when using the latest version of a Pub package
greglittlefield-wf Aug 6, 2024
dc5cb89
Display package versions from pubspec.lock, improve logs
greglittlefield-wf Aug 6, 2024
db2dc7e
Update instructions about package mismatches
greglittlefield-wf Aug 6, 2024
6393501
Format
greglittlefield-wf Aug 6, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion .github/workflows/dart_ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,19 @@ jobs:
if: always() && steps.install.outcome == 'success'

- name: Validate formatting
run: dart format --set-exit-if-changed .
run: dart run dart_dev format --check
if: always() && steps.install.outcome == 'success' && matrix.sdk == '2.18.7'

- name: Analyze project source
run: dart analyze
if: always() && steps.install.outcome == 'success'

- name: Ensure checked-in generated files are up to date
run: |
dart run build_runner build --delete-conflicting-outputs
git diff --exit-code
if: always() && steps.install.outcome == 'success' && matrix.sdk == '2.19.6'

test:
runs-on: ubuntu-latest
strategy:
Expand Down
2 changes: 1 addition & 1 deletion analysis_options.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ analyzer:
missing_required_param: error
must_call_super: error
exclude:
- test/test_fixtures
- test/test_fixtures/**
15 changes: 15 additions & 0 deletions bin/null_safety_required_props.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright 2024 Workiva Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

export 'package:over_react_codemod/src/executables/null_safety_required_props.dart';
11 changes: 11 additions & 0 deletions build.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
targets:
$default:
builders:
json_serializable:
generate_for:
include:
- '**.sg.dart'
source_gen|combining_builder:
options:
ignore_for_file:
- implicit_dynamic_parameter
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import 'package:over_react_codemod/src/util.dart';
import 'package:over_react_codemod/src/util/component_usage.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:over_react_codemod/src/util/get_all_props.dart';
import 'package:over_react_codemod/src/vendor/over_react_analyzer_plugin/get_all_props.dart';
import 'package:pub_semver/pub_semver.dart';

import 'utils/class_component_required_fields.dart';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

import 'package:over_react_codemod/src/util/component_usage.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:over_react_codemod/src/util/get_all_props.dart';
import 'package:over_react_codemod/src/util/get_all_state.dart';
import 'package:pub_semver/pub_semver.dart';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ abstract class ClassComponentRequiredFieldsMigrator<
final Set<DefaultedOrInitializedDeclaration> fieldData = {};

void patchFieldDeclarations(
List<FieldElement> Function(InterfaceElement) getAll,
Iterable<FieldElement> Function(InterfaceElement) getAll,
Iterable<Assignment> cascadedDefaultPropsOrInitialState,
CascadeExpression node) {
for (final field in cascadedDefaultPropsOrInitialState) {
Expand All @@ -77,7 +77,7 @@ abstract class ClassComponentRequiredFieldsMigrator<
}

VariableDeclaration? _getFieldDeclaration(
List<FieldElement> Function(InterfaceElement) getAll,
Iterable<FieldElement> Function(InterfaceElement) getAll,
{required InterfaceElement propsOrStateElement,
required String fieldName}) {
// For component1 boilerplate its possible that `fieldEl` won't be found using `lookUpVariable` below
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,12 @@ bool nonNullableHintAlreadyExists(TypeAnnotation type) {

const nonNullableHint = '/*!*/';

const lateHint = '/*late*/';

/// Whether the late hint already exists before [type]
bool requiredHintAlreadyExists(TypeAnnotation type) {
// Since the `/*late*/` comment is possibly adjacent to the prop declaration's doc comments,
// we have to recursively traverse the `precedingComments` in order to determine if the `/*late*/`
// comment actually exists.
return allComments(type.beginToken).any((t) => t.value() == '/*late*/');
return allCommentsForNode(type).any((t) => t.value() == lateHint);
}
112 changes: 112 additions & 0 deletions lib/src/dart3_suggestors/required_props/bin/aggregate.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
// Copyright 2024 Workiva Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import 'dart:async';
import 'dart:io';

import 'package:args/args.dart';
import 'package:collection/collection.dart';
import 'package:io/io.dart';
import 'package:logging/logging.dart';
import '../collect/aggregate.dart';
import '../collect/logging.dart';

/// Aggregates individual data files, like what the collect command does,
/// but as a standalone command.
///
/// This is leftover from before the collect command also aggregated data,
/// and is not publicly exposed, but is left in place just in case for
/// debugging purposes and potential future use.
///
/// Also outputs some additional statistics.
Future<void> main(List<String> args) async {
final argParser = ArgParser()
..addFlag('help', help: 'Print this usage information', negatable: false)
..addOption(
'output',
abbr: 'o',
help: 'The file to write output to.',
valueHelp: 'path',
defaultsTo: defaultAggregatedOutputFile,
);
final parsedArgs = argParser.parse(args);
if (parsedArgs['help'] as bool) {
print(argParser.usage);
exit(ExitCode.success.code);
}
final outputFile = parsedArgs['output']! as String;
final filesToAggregate = parsedArgs.rest;
if (filesToAggregate.isEmpty) {
print('Must specify files to aggregate.\n${argParser.usage}');
exit(ExitCode.usage.code);
}

initLogging();
final logger = Logger('prop_requiredness_aggregate');

logger.info('Loading results from files specified in arguments...');
final allResults = loadResultFiles(filesToAggregate);

{
// Gather some stats on how often different builder types show up.
final allUsages = allResults.expand((r) => r.usages);

final countsByBuilderType =
allUsages.countBy((u) => u.usageBuilderType.name);
File('counts_by_builder_type.json')
.writeAsStringSync(jsonEncodeIndented(countsByBuilderType));

final countsByBuilderTypeByMixin = allUsages
.multiGroupListsBy((u) => u.mixinData.map((e) => e.mixinId))
.map((mixinId, usages) =>
MapEntry(mixinId, usages.countBy((u) => u.usageBuilderType.name)));
File('counts_by_builder_type_by_mixin.json')
.writeAsStringSync(jsonEncodeIndented(countsByBuilderTypeByMixin));
}

logger.info('Aggregating data...');
final aggregated = aggregateData(allResults);
logger.info('Done.');

// logger.fine('Props mixins with the same name:');
// final mixinIdsByName = aggregated.mixinMetadata.mixinNamesById.keysByValues();
// mixinIdsByName.forEach((name, mixinIds) {
// if (mixinIds.length > 1) logger.fine('$name: ${mixinIds.map((id) => '\n - $id').join('')}');
// });

File(outputFile).writeAsStringSync(jsonEncodeIndented(aggregated));
logger.info('Wrote JSON results to $outputFile');
}

extension<E> on Iterable<E> {
Map<T, int> countBy<T>(T Function(E) getBucket) {
final counts = <T, int>{};
for (final element in this) {
final bucket = getBucket(element);
counts[bucket] = (counts[bucket] ?? 0) + 1;
}
return counts;
}

/// Like [groupListsBy] but allows elements to be added to multiple groups.
Map<T, List<E>> multiGroupListsBy<T>(Iterable<T> Function(E) keysOf) {
final groups = <T, List<E>>{};
for (final element in this) {
for (final key in keysOf(element)) {
groups.putIfAbsent(key, () => []).add(element);
}
}
return groups;
}
}
169 changes: 169 additions & 0 deletions lib/src/dart3_suggestors/required_props/bin/codemod.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
// Copyright 2024 Workiva Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import 'dart:convert';
import 'dart:io';

import 'package:args/args.dart';
import 'package:args/command_runner.dart';
import 'package:codemod/codemod.dart';
import 'package:over_react_codemod/src/dart3_suggestors/required_props/codemod/required_props_suggestor.dart';
import 'package:over_react_codemod/src/util.dart';
import 'package:over_react_codemod/src/util/args.dart';
import 'package:over_react_codemod/src/util/command_runner.dart';
import 'package:over_react_codemod/src/util/package_util.dart';

import '../codemod/recommender.dart';
import '../collect/aggregated_data.sg.dart';

abstract class _Options {
static const propRequirednessData = 'prop-requiredness-data';
static const privateRequirednessThreshold = 'private-requiredness-threshold';
static const privateMaxAllowedSkipRate = 'private-max-allowed-skip-rate';
static const publicRequirednessThreshold = 'public-requiredness-threshold';
static const publicMaxAllowedSkipRate = 'public-max-allowed-skip-rate';

static const all = {
propRequirednessData,
privateRequirednessThreshold,
privateMaxAllowedSkipRate,
publicRequirednessThreshold,
publicMaxAllowedSkipRate
};
}

abstract class _Flags {
static const trustRequiredAnnotations = 'trust-required-annotations';
static const all = {
trustRequiredAnnotations,
};
}

class CodemodCommand extends Command {
@override
String get description =>
"Adds null safety migrator hints to OverReact props using prop requiredness data from 'collect' command.";

@override
String get name => 'codemod';

@override
String get invocation => '$invocationPrefix [<options>]';

@override
String get usageFooter => '''
\nInstructions
============

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

$parentInvocationPrefix collect --help

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

$invocationPrefix

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:

${r'^ *// TODO\(orcm.required_props\):.+(?:\n *// .+)*'}
''';

CodemodCommand() {
argParser
..addOption(_Options.propRequirednessData,
help:
"The file containing prop requiredness data, collected via the 'over_react_codemod:collect' command.",
defaultsTo: 'prop_requiredness.json')
..addFlag(_Flags.trustRequiredAnnotations,
defaultsTo: true,
help:
'Whether to migrate @requiredProp and `@nullableRequiredProp` props to late required, regardless of usage data.'
'\nNote that @requiredProp has no effect on function components, so these annotations may be incorrect.')
..addOption(_Options.privateRequirednessThreshold,
defaultsTo: (0.95).toString(),
help:
'The minimum rate (0.0-1.0) a private prop must be set to be considered required.')
..addOption(_Options.privateMaxAllowedSkipRate,
defaultsTo: (0.2).toString(),
help:
'The maximum allowed rate (0.0-1.0) of dynamic usages of private mixins, for which data collection was skipped.'
'\nIf above this, all props in a mixin will be made optional (with a TODO comment).')
..addOption(_Options.publicRequirednessThreshold,
defaultsTo: (1).toString(),
help:
'The minimum rate (0.0-1.0) a public prop must be set to be considered required.')
..addOption(_Options.publicMaxAllowedSkipRate,
defaultsTo: (0.05).toString(),
help:
'The maximum allowed rate (0.0-1.0) of dynamic usages of public mixins, for which data collection was skipped.'
'\nIf above this, all props in a mixin will be made optional (with a TODO comment).');

argParser.addSeparator('Codemod options');
addCodemodArgs(argParser);
}

@override
Future<void> run() async {
final parsedArgs = this.argResults!;
final propRequirednessDataFile =
parsedArgs[_Options.propRequirednessData]! as String;
final codemodArgs = removeFlagArgs(
removeOptionArgs(parsedArgs.arguments, _Options.all), _Flags.all);

final packageRoot = findPackageRootFor('.');
await runPubGetIfNeeded(packageRoot);
final dartPaths = allDartPathsExceptHiddenAndGenerated();

final results = PropRequirednessResults.fromJson(
jsonDecode(File(propRequirednessDataFile).readAsStringSync()));
final recommender = PropRequirednessRecommender(
results,
privateRequirednessThreshold:
parsedArgs.argValueAsNumber(_Options.privateRequirednessThreshold),
privateMaxAllowedSkipRate:
parsedArgs.argValueAsNumber(_Options.privateMaxAllowedSkipRate),
publicRequirednessThreshold:
parsedArgs.argValueAsNumber(_Options.publicRequirednessThreshold),
publicMaxAllowedSkipRate:
parsedArgs.argValueAsNumber(_Options.publicMaxAllowedSkipRate),
);

exitCode = await runInteractiveCodemodSequence(
dartPaths,
[
RequiredPropsSuggestor(
recommender,
trustRequiredAnnotations:
parsedArgs[_Flags.trustRequiredAnnotations] as bool,
),
],
defaultYes: true,
args: codemodArgs,
additionalHelpOutput: argParser.usage,
);
}
}

extension on ArgResults {
num argValueAsNumber(String name) => num.parse(this[name]);
}
Loading
Loading