Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import 'package:analyzer/dart/ast/ast.dart';
import 'package:over_react_codemod/src/vendor/over_react_analyzer_plugin/get_all_props.dart';
import 'package:pub_semver/pub_semver.dart';

import '../required_props/codemod/recommender.dart';
import 'utils/class_component_required_fields.dart';

/// Suggestor to assist with preparations for null-safety by adding
Expand Down Expand Up @@ -76,7 +77,10 @@ import 'utils/class_component_required_fields.dart';
/// ```
class ClassComponentRequiredDefaultPropsMigrator
extends ClassComponentRequiredFieldsMigrator<PropAssignment> {
ClassComponentRequiredDefaultPropsMigrator([Version? sdkVersion])
final PropRequirednessRecommender? _propRequirednessRecommender;

ClassComponentRequiredDefaultPropsMigrator(
[Version? sdkVersion, this._propRequirednessRecommender])
: super('defaultProps', 'getDefaultProps', sdkVersion);

@override
Expand Down Expand Up @@ -105,6 +109,7 @@ class ClassComponentRequiredDefaultPropsMigrator
.map((assignment) => PropAssignment(assignment))
.where((prop) => prop.node.writeElement?.displayName != null);

patchFieldDeclarations(getAllProps, cascadedDefaultProps, node);
patchFieldDeclarations(
getAllProps, cascadedDefaultProps, node, _propRequirednessRecommender);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import 'package:analyzer/dart/ast/visitor.dart';
import 'package:pub_semver/pub_semver.dart';

import '../../../util/class_suggestor.dart';
import '../../required_props/codemod/recommender.dart';
import '../analyzer_plugin_utils.dart';

/// A class shared by the suggestors that manage defaultProps/initialState.
Expand Down Expand Up @@ -52,7 +53,8 @@ abstract class ClassComponentRequiredFieldsMigrator<
void patchFieldDeclarations(
Iterable<FieldElement> Function(InterfaceElement) getAll,
Iterable<Assignment> cascadedDefaultPropsOrInitialState,
CascadeExpression node) {
CascadeExpression node,
[PropRequirednessRecommender? _propRequirednessRecommender]) {
for (final field in cascadedDefaultPropsOrInitialState) {
final isDefaultedToNull =
field.node.rightHandSide.staticType!.isDartCoreNull;
Expand All @@ -66,6 +68,17 @@ abstract class ClassComponentRequiredFieldsMigrator<
// The field declaration is likely in another file which our logic currently doesn't handle.
// In this case, don't add an entry to `fieldData`.
if (fieldDeclaration == null) continue;
final element = fieldDeclaration.declaredElement;

// Don't set as required if the prop is publicly exported.
if (_propRequirednessRecommender != null && element is FieldElement) {
final isPublic = _propRequirednessRecommender
.getRecommendation(element)
?.reason
?.isPublic ??
false;
if (isPublic) continue;
}

fieldData.add(DefaultedOrInitializedDeclaration(
fieldDeclaration, fieldEl, isDefaultedToNull));
Expand Down
11 changes: 11 additions & 0 deletions lib/src/dart3_suggestors/required_props/bin/codemod.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ 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 '../../null_safety_prep/class_component_required_default_props.dart';
import '../codemod/recommender.dart';
import '../collect/aggregated_data.sg.dart';

Expand Down Expand Up @@ -148,6 +149,16 @@ class CodemodCommand extends Command {
parsedArgs.argValueAsNumber(_Options.publicMaxAllowedSkipRate),
);

exitCode = await runInteractiveCodemodSequence(
dartPaths,
[
ClassComponentRequiredDefaultPropsMigrator(null, recommender),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this not work when added to the sequence below, like so?

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

Not a huge deal, but it would be nice if consumers who wanted to apply all patches via the prompt (like if they didn't think to pass --yes-to-all) didn't have to do so twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It combines the patches when I put them together 😢

'mixin TestPrivatePropsMixin on UiProps {\n'
              '  String/*?*/ notDefaultedOptional;\n'
              '  /*late*/ String notDefaultedAlwaysSet;\n'
              '  /*late/*?*/*/ String/*?*/ defaultedN/*?*/ullable;\n'
              '  /*late*/ num/*!*/ defaultedNonNullable;\n'
              '}\n'

https://github.com/Workiva/over_react_codemod/actions/runs/11372701838/job/31637614201?pr=299

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm okay, thanks for trying!

There's probably a bug in dart_codemod, then, possibly due to reusing FileContext/AnalysisContextCollection between suggestors.

],
defaultYes: true,
args: codemodArgs,
additionalHelpOutput: argParser.usage,
);

exitCode = await runInteractiveCodemodSequence(
dartPaths,
[
Expand Down
12 changes: 9 additions & 3 deletions lib/src/dart3_suggestors/required_props/codemod/recommender.dart
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class PropRequirednessRecommender {
isPublic ? publicRequirednessThreshold : privateRequirednessThreshold;

if (totalRequirednessRate < requirednessThreshold) {
final reason = RequirednessThresholdOptionalReason();
final reason = RequirednessThresholdOptionalReason(isPublic: isPublic);
return PropRecommendation.optional(reason);
} else {
return const PropRecommendation.required();
Expand Down Expand Up @@ -146,11 +146,14 @@ class PropRecommendation {
const PropRecommendation.optional(this.reason) : isRequired = false;
}

abstract class OptionalReason {}
abstract class OptionalReason {
abstract final bool isPublic;
}

class SkipRateOptionalReason extends OptionalReason {
final num skipRate;
final num maxAllowedSkipRate;
@override
final bool isPublic;

SkipRateOptionalReason({
Expand All @@ -161,5 +164,8 @@ class SkipRateOptionalReason extends OptionalReason {
}

class RequirednessThresholdOptionalReason extends OptionalReason {
RequirednessThresholdOptionalReason();
@override
final bool isPublic;

RequirednessThresholdOptionalReason({required this.isPublic});
}
2 changes: 0 additions & 2 deletions lib/src/executables/null_safety_migrator_companion.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import 'dart:io';

import 'package:args/args.dart';
import 'package:codemod/codemod.dart';
import 'package:over_react_codemod/src/dart3_suggestors/null_safety_prep/class_component_required_default_props.dart';
import 'package:over_react_codemod/src/dart3_suggestors/null_safety_prep/class_component_required_initial_state.dart';
import 'package:over_react_codemod/src/util.dart';

Expand All @@ -43,7 +42,6 @@ void main(List<String> args) async {
dartPaths,
aggregate([
CallbackRefHintSuggestor(),
ClassComponentRequiredDefaultPropsMigrator(),
ClassComponentRequiredInitialStateMigrator(),
]),
defaultYes: true,
Expand Down
18 changes: 18 additions & 0 deletions test/executables/required_props_collect_and_codemod_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,24 @@ mixin TestPrivateProps on UiProps {
String/*?*/ set20percent;
$noDataTodoComment
String/*?*/ set0percent;
}''')),
d.file('test_class_component_defaults.dart', contains('''
mixin TestPrivatePropsMixin on UiProps {
String/*?*/ notDefaultedOptional;
/*late*/ String notDefaultedAlwaysSet;
/*late*/ String/*?*/ defaultedNullable;
/*late*/ num/*!*/ defaultedNonNullable;
}

mixin SomeOtherPropsMixin on UiProps {
/*late*/ num/*!*/ anotherDefaultedNonNullable;
}''')),
d.file('test_class_component_defaults.dart', contains('''
mixin TestPublic2PropsMixin on UiProps {
String/*?*/ notDefaultedOptional;
/*late*/ String notDefaultedAlwaysSet;
String/*?*/ defaultedNullable;
num/*?*/ defaultedNonNullable;
}''')),
d.file('test_private_dynamic.dart', contains('''
// TODO(orcm.required_props): This codemod couldn't reliably determine requiredness for these props
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export 'src/test_class_component_defaults.dart' show TestPublic2, TestPublic2PropsMixin;
export 'src/test_public.dart';
export 'src/test_public_dynamic.dart';
export 'src/test_public_multiple_components.dart';
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import 'package:over_react/over_react.dart';

part 'test_class_component_defaults.over_react.g.dart';

mixin TestPrivatePropsMixin on UiProps {
String notDefaultedOptional;
String notDefaultedAlwaysSet;
String defaultedNullable;
num defaultedNonNullable;
}

mixin SomeOtherPropsMixin on UiProps {
num anotherDefaultedNonNullable;
}

class TestPrivateProps = UiProps
with TestPrivatePropsMixin, SomeOtherPropsMixin;

UiFactory<TestPrivateProps> TestPrivate =
castUiFactory(_$TestPrivate); // ignore: undefined_identifier

class TestPrivateComponent extends UiComponent2<TestPrivateProps> {
@override
get defaultProps => (newProps()
..defaultedNullable = null
..defaultedNonNullable = 2.1
..anotherDefaultedNonNullable = 1.1
);

@override
render() {}
}

mixin TestPublic2PropsMixin on UiProps {
String notDefaultedOptional;
String notDefaultedAlwaysSet;
String defaultedNullable;
num defaultedNonNullable;
}

class TestPublic2Props = UiProps
with TestPublic2PropsMixin, SomeOtherPropsMixin;

UiFactory<TestPublic2Props> TestPublic2 =
castUiFactory(_$TestPublic2); // ignore: undefined_identifier

class TestPublic2Component extends UiComponent2<TestPublic2Props> {
@override
get defaultProps => (newProps()
..defaultedNullable = null
..defaultedNonNullable = 2.1
..anotherDefaultedNonNullable = 1.1
);

@override
render() {}
}

usages() {
(TestPrivate()..notDefaultedAlwaysSet = 'abc')();
(TestPrivate()
..notDefaultedOptional = 'abc'
..notDefaultedAlwaysSet = 'abc'
..defaultedNullable = 'abc'
..defaultedNonNullable = 1
..anotherDefaultedNonNullable = 2
)();
(TestPublic2()..notDefaultedAlwaysSet = 'abc')();
(TestPublic2()
..notDefaultedAlwaysSet = 'abc'
..notDefaultedOptional = 'abc'
..defaultedNullable = 'abc'
..defaultedNonNullable = 1
..anotherDefaultedNonNullable = 2
)();
}
Loading