From fa55d0654c7722208ea30ec590457de5e50e17a8 Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Wed, 10 Apr 2024 13:34:35 -0600 Subject: [PATCH 01/11] Add suggestor to migrate conditional prop function calls to null safety --- .../fn_prop_null_aware_call_suggestor.dart | 178 ++++++++++++++++++ lib/src/executables/null_safety_prep.dart | 2 + ...n_prop_null_aware_call_suggestor_test.dart | 141 ++++++++++++++ 3 files changed, 321 insertions(+) create mode 100644 lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart create mode 100644 test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart diff --git a/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart new file mode 100644 index 00000000..fa809a91 --- /dev/null +++ b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart @@ -0,0 +1,178 @@ +// 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 'package:analyzer/dart/analysis/results.dart'; +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/token.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/dart/element/type.dart'; +import 'package:collection/collection.dart'; +import 'package:over_react_codemod/src/util/class_suggestor.dart'; + +/// Suggestor that replaces conditional calls to functions declared in props +/// with inline null-aware property access. +/// +/// This is helpful for null-safety migrations because the conditional +/// function calls will otherwise get migrated with `!` modifiers. +/// +/// **Before:** +/// +/// ```dart +/// if (props.someCallback != null) { +/// props.someCallback(someValue); +/// } +/// +/// // Will be migrated to: +/// if (props.someCallback != null) { +/// props.someCallback!(someValue); +/// } +/// ``` +/// +/// **After:** +/// +/// ```dart +/// // This will require no changes during a null-safety migration. +/// props.someCallback?.call(someValue); +/// ``` +class FnPropNullAwareCallSuggestor extends RecursiveAstVisitor + with ClassSuggestor { + ResolvedUnitResult? _result; + + @override + visitExpressionStatement(ExpressionStatement node) { + super.visitExpressionStatement(node); + + if (node.expression is! BinaryExpression) return; + + final relevantExprStatement = + _getPropFunctionExpressionBeingCalledConditionally( + node.expression as BinaryExpression); + final inlineBinaryExpr = + // This cast is safe due to the type checks within `_getPropFunctionExpressionBeingCalledConditionally`. + relevantExprStatement?.expression as BinaryExpression?; + if (inlineBinaryExpr == null) return; + final relevantFnExpr = + // This cast is safe due to the type checks within `_getPropFunctionExpressionBeingCalledConditionally`. + inlineBinaryExpr.rightOperand as FunctionExpressionInvocation; + // This cast is safe due to the type checks within `_getPropFunctionExpressionBeingCalledConditionally`. + final fn = relevantFnExpr.function as PropertyAccess; + + yieldPatch( + '${fn.target}.${fn.propertyName}?.call${relevantFnExpr.argumentList};', + node.offset, + node.end); + } + + @override + visitIfStatement(IfStatement node) { + super.visitIfStatement(node); + + if (node.condition is! BinaryExpression) return; + + final relevantFnExprStatement = + _getPropFunctionExpressionBeingCalledConditionally( + node.condition as BinaryExpression); + final relevantFnExpr = + // This cast is safe due to the type checks within `_getPropFunctionExpressionBeingCalledConditionally`. + relevantFnExprStatement?.expression as FunctionExpressionInvocation?; + if (relevantFnExpr == null) return; + // This cast is safe due to the type checks within `_getPropFunctionExpressionBeingCalledConditionally`. + final fn = relevantFnExpr.function as PropertyAccess?; + if (fn == null) return; + + yieldPatch( + '${fn.target}.${fn.propertyName}?.call${relevantFnExpr.argumentList};', + node.offset, + node.end); + } + + /// Returns the function expression (e.g. `props.onClick(event)`) being called + /// after the null condition is checked. + ExpressionStatement? _getPropFunctionExpressionBeingCalledConditionally( + BinaryExpression condition) { + // if (props.fn != null) { ... } + if (condition.parent is IfStatement) { + var propFunctionBeingNullChecked = + _getPropFunctionBeingNullChecked(condition); + + return ((condition.parent! as IfStatement).thenStatement as Block) + .statements + .singleWhereOrNull((element) { + if (element is! ExpressionStatement) return false; + final expression = element.expression; + if (expression is! FunctionExpressionInvocation) return false; + final fn = expression.function; + if (fn is! PropertyAccess) return false; + final target = fn.target; + if (target is! SimpleIdentifier) return false; + if (target.name != 'props') return false; + final matches = fn.propertyName.staticElement?.declaration == + propFunctionBeingNullChecked?.staticElement?.declaration; + return matches; + }) as ExpressionStatement?; + // props.fn != null && ... + } else if (condition.parent is ExpressionStatement && + condition.leftOperand is BinaryExpression) { + final propFunctionBeingNullChecked = _getPropFunctionBeingNullChecked( + condition.leftOperand as BinaryExpression); + if (propFunctionBeingNullChecked == null) return null; + + if (condition.rightOperand is! FunctionExpressionInvocation) return null; + final fn = + (condition.rightOperand as FunctionExpressionInvocation).function; + if (fn is! PropertyAccess) return null; + final target = fn.target; + if (target is! SimpleIdentifier) return null; + if (target.name != 'props') return null; + final matches = fn.propertyName.staticElement?.declaration == + propFunctionBeingNullChecked.staticElement?.declaration; + if (!matches) return null; + return condition.parent as ExpressionStatement?; + } + + return null; + } + + /// Returns the identifier for the function that is being + /// null checked before being called. + SimpleIdentifier? _getPropFunctionBeingNullChecked( + BinaryExpression condition) { + if (condition.leftOperand is! PrefixedIdentifier) { + return null; + } + final leftOperand = condition.leftOperand as PrefixedIdentifier; + final prefix = leftOperand.prefix; + if (prefix.name != 'props') { + return null; + } + if (leftOperand.identifier.staticType is! FunctionType) { + return null; + } + if (condition.operator.stringValue != '!=' && + condition.operator.next?.keyword != Keyword.NULL) { + return null; + } + return leftOperand.identifier; + } + + @override + Future generatePatches() async { + _result = await context.getResolvedUnit(); + if (_result == null) { + throw Exception( + 'Could not get resolved result for "${context.relativePath}"'); + } + _result!.unit.accept(this); + } +} diff --git a/lib/src/executables/null_safety_prep.dart b/lib/src/executables/null_safety_prep.dart index f9eb42ee..185d7628 100644 --- a/lib/src/executables/null_safety_prep.dart +++ b/lib/src/executables/null_safety_prep.dart @@ -17,6 +17,7 @@ import 'dart:io'; import 'package:args/args.dart'; import 'package:codemod/codemod.dart'; import 'package:over_react_codemod/src/dart3_suggestors/null_safety_prep/dom_callback_null_args.dart'; +import 'package:over_react_codemod/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart'; import 'package:over_react_codemod/src/dart3_suggestors/null_safety_prep/use_ref_init_migration.dart'; import 'package:over_react_codemod/src/util.dart'; @@ -38,6 +39,7 @@ void main(List args) async { dartPaths, aggregate([ UseRefInitMigration(), + FnPropNullAwareCallSuggestor(), DomCallbackNullArgs(), CallbackRefHintSuggestor(), ]), diff --git a/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart new file mode 100644 index 00000000..07358d8c --- /dev/null +++ b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart @@ -0,0 +1,141 @@ +// 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 'package:over_react_codemod/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart'; +import 'package:test/test.dart'; + +import '../../resolved_file_context.dart'; +import '../../util.dart'; +import '../../util/component_usage_migrator_test.dart'; + +void main() { + final resolvedContext = SharedAnalysisContext.overReact; + + // Warm up analysis in a setUpAll so that if getting the resolved AST times out + // (which is more common for the WSD context), it fails here instead of failing the first test. + setUpAll(resolvedContext.warmUpAnalysis); + + group('FnPropNullAwareCallSuggestor', () { + late SuggestorTester testSuggestor; + + setUp(() { + testSuggestor = getSuggestorTester( + FnPropNullAwareCallSuggestor(), + resolvedContext: resolvedContext, + ); + }); + + group('handles block if conditions', () { + test('with a single condition', () async { + await testSuggestor( + expectedPatchCount: 1, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + if (props.onClick != null) { + props.onClick(e); + } + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + expectedOutput: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + props.onClick?.call(e); + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + ''')); + }); + + test('unless there are multiple conditions', () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + if (props.onClick != null && props.onMouseEnter != null) { + props.onClick(e); + } + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + ); + }); + }); + + group('handles inline if conditions', () { + test('with a single condition', () async { + await testSuggestor( + expectedPatchCount: 1, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + props.onClick != null && props.onClick(e); + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + expectedOutput: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + props.onClick?.call(e); + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + ''')); + }); + + test('unless there are multiple conditions', () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + 1 > 0 && props.onClick != null && props.onClick(e); + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + ); + }); + }); + }); +} From f5297154cd20db5c495a8cebc442ee0116c2a240 Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Wed, 10 Apr 2024 13:42:14 -0600 Subject: [PATCH 02/11] Improve clarification --- .../fn_prop_null_aware_call_suggestor.dart | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart index fa809a91..8c3e0a98 100644 --- a/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart +++ b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart @@ -101,8 +101,12 @@ class FnPropNullAwareCallSuggestor extends RecursiveAstVisitor /// after the null condition is checked. ExpressionStatement? _getPropFunctionExpressionBeingCalledConditionally( BinaryExpression condition) { - // if (props.fn != null) { ... } if (condition.parent is IfStatement) { + // + // Handles conditions of the form: + // if (props.fn != null) { ... } + // + var propFunctionBeingNullChecked = _getPropFunctionBeingNullChecked(condition); @@ -121,9 +125,12 @@ class FnPropNullAwareCallSuggestor extends RecursiveAstVisitor propFunctionBeingNullChecked?.staticElement?.declaration; return matches; }) as ExpressionStatement?; - // props.fn != null && ... } else if (condition.parent is ExpressionStatement && condition.leftOperand is BinaryExpression) { + // + // Handles conditions of the form: + // props.fn != null && ... + // final propFunctionBeingNullChecked = _getPropFunctionBeingNullChecked( condition.leftOperand as BinaryExpression); if (propFunctionBeingNullChecked == null) return null; From 8078dbce42f71be90624d109c43e4efab5965641 Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Wed, 10 Apr 2024 13:46:43 -0600 Subject: [PATCH 03/11] Add more test coverage --- ...n_prop_null_aware_call_suggestor_test.dart | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart index 07358d8c..5807a392 100644 --- a/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart +++ b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart @@ -68,6 +68,28 @@ void main() { ''')); }); + test( + 'unless the single condition is not a null check of the function being called', + () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + if (1 > 0) { + props.onClick(e); + } + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + ); + }); + test('unless there are multiple conditions', () async { await testSuggestor( expectedPatchCount: 0, @@ -119,6 +141,26 @@ void main() { ''')); }); + test( + 'unless the single condition is not a null check of the function being called', + () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + 1 > 0 && props.onClick(e); + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + ); + }); + test('unless there are multiple conditions', () async { await testSuggestor( expectedPatchCount: 0, From 046dc7d47617b6ddcd4e1483b03c00674c6ab7af Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Wed, 10 Apr 2024 13:48:39 -0600 Subject: [PATCH 04/11] Moar test coverage --- ...n_prop_null_aware_call_suggestor_test.dart | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart index 5807a392..45f9d3dc 100644 --- a/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart +++ b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart @@ -90,6 +90,28 @@ void main() { ); }); + test( + 'unless the single condition involves the function being called, but is not a null check', + () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + if (props.onClick is Function) { + props.onClick(e); + } + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + ); + }); + test('unless there are multiple conditions', () async { await testSuggestor( expectedPatchCount: 0, @@ -161,6 +183,26 @@ void main() { ); }); + test( + 'unless the single condition involves the function being called, but is not a null check', + () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + props.onClick is Function && props.onClick(e); + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + ); + }); + test('unless there are multiple conditions', () async { await testSuggestor( expectedPatchCount: 0, From 5bbfd5bae91ae372e735d7206d5357fc433a41d7 Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Thu, 11 Apr 2024 12:35:12 -0600 Subject: [PATCH 05/11] Handle else condition --- .../fn_prop_null_aware_call_suggestor.dart | 15 +++- ...n_prop_null_aware_call_suggestor_test.dart | 70 +++++++++++++++---- 2 files changed, 70 insertions(+), 15 deletions(-) diff --git a/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart index 8c3e0a98..56a4f8aa 100644 --- a/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart +++ b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart @@ -107,10 +107,19 @@ class FnPropNullAwareCallSuggestor extends RecursiveAstVisitor // if (props.fn != null) { ... } // - var propFunctionBeingNullChecked = + final propFunctionBeingNullChecked = _getPropFunctionBeingNullChecked(condition); - - return ((condition.parent! as IfStatement).thenStatement as Block) + final ifStatement = condition.parent! as IfStatement; + if (ifStatement.elseStatement != null) return null; + if (ifStatement.parent + ?.thisOrAncestorOfType() + ?.elseStatement != + null) { + // There is an else-if statement present + return null; + } + + return (ifStatement.thenStatement as Block) .statements .singleWhereOrNull((element) { if (element is! ExpressionStatement) return false; diff --git a/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart index 45f9d3dc..9b9a5c89 100644 --- a/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart +++ b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart @@ -90,12 +90,58 @@ void main() { ); }); + test('unless there is an else condition', () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final bar = useState(0); + final handleClick = useCallback((e) { + if (props.onClick != null) { + props.onClick(e); + } else { + bar.set(1); + } + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(bar.value); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + ); + }); + + test('unless there is an else if condition', () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final bar = useState(0); + final handleClick = useCallback((e) { + if (props.onMouseEnter != null) { + bar.set(1); + } else if (props.onClick != null) { + props.onClick(e); + } + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(bar.value); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + ); + }); + test( 'unless the single condition involves the function being called, but is not a null check', - () async { - await testSuggestor( - expectedPatchCount: 0, - input: withOverReactImport(''' + () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' final Foo = uiFunction( (props) { final handleClick = useCallback((e) { @@ -109,8 +155,8 @@ void main() { UiFactoryConfig(displayName: 'Foo'), ); '''), - ); - }); + ); + }); test('unless there are multiple conditions', () async { await testSuggestor( @@ -185,10 +231,10 @@ void main() { test( 'unless the single condition involves the function being called, but is not a null check', - () async { - await testSuggestor( - expectedPatchCount: 0, - input: withOverReactImport(''' + () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' final Foo = uiFunction( (props) { final handleClick = useCallback((e) { @@ -200,8 +246,8 @@ void main() { UiFactoryConfig(displayName: 'Foo'), ); '''), - ); - }); + ); + }); test('unless there are multiple conditions', () async { await testSuggestor( From 2010ba228b8e54a2bea165db30894ac89c0fd0c2 Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Thu, 11 Apr 2024 12:49:33 -0600 Subject: [PATCH 06/11] Handle inline if then expressions --- .../fn_prop_null_aware_call_suggestor.dart | 43 +++++---- ...n_prop_null_aware_call_suggestor_test.dart | 89 +++++++++++++++++++ 2 files changed, 116 insertions(+), 16 deletions(-) diff --git a/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart index 56a4f8aa..684a523d 100644 --- a/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart +++ b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart @@ -118,22 +118,15 @@ class FnPropNullAwareCallSuggestor extends RecursiveAstVisitor // There is an else-if statement present return null; } - - return (ifStatement.thenStatement as Block) - .statements - .singleWhereOrNull((element) { - if (element is! ExpressionStatement) return false; - final expression = element.expression; - if (expression is! FunctionExpressionInvocation) return false; - final fn = expression.function; - if (fn is! PropertyAccess) return false; - final target = fn.target; - if (target is! SimpleIdentifier) return false; - if (target.name != 'props') return false; - final matches = fn.propertyName.staticElement?.declaration == - propFunctionBeingNullChecked?.staticElement?.declaration; - return matches; - }) as ExpressionStatement?; + final thenStatement = ifStatement.thenStatement; + if (thenStatement is Block) { + return _getMatchingConditionalPropFunctionCallStatement( + thenStatement.statements, propFunctionBeingNullChecked); + } else if (thenStatement is ExpressionStatement) { + return _getMatchingConditionalPropFunctionCallStatement( + [thenStatement], propFunctionBeingNullChecked); + } + return null; } else if (condition.parent is ExpressionStatement && condition.leftOperand is BinaryExpression) { // @@ -160,6 +153,24 @@ class FnPropNullAwareCallSuggestor extends RecursiveAstVisitor return null; } + ExpressionStatement? _getMatchingConditionalPropFunctionCallStatement( + List thenStatements, + SimpleIdentifier? propFunctionBeingNullChecked) { + return thenStatements.singleWhereOrNull((element) { + if (element is! ExpressionStatement) return false; + final expression = element.expression; + if (expression is! FunctionExpressionInvocation) return false; + final fn = expression.function; + if (fn is! PropertyAccess) return false; + final target = fn.target; + if (target is! SimpleIdentifier) return false; + if (target.name != 'props') return false; + final matches = fn.propertyName.staticElement?.declaration == + propFunctionBeingNullChecked?.staticElement?.declaration; + return matches; + }) as ExpressionStatement?; + } + /// Returns the identifier for the function that is being /// null checked before being called. SimpleIdentifier? _getPropFunctionBeingNullChecked( diff --git a/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart index 9b9a5c89..a938d4bf 100644 --- a/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart +++ b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart @@ -180,6 +180,95 @@ void main() { }); group('handles inline if conditions', () { + test('with a single condition', () async { + await testSuggestor( + expectedPatchCount: 1, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + if (props.onClick != null) props.onClick(e); + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + expectedOutput: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + props.onClick?.call(e); + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + ''')); + }); + + test( + 'unless the single condition is not a null check of the function being called', + () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + if (1 > 0) props.onClick(e); + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + ); + }); + + test( + 'unless the single condition involves the function being called, but is not a null check', + () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + if (props.onClick is Function) props.onClick(e); + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + ); + }); + + test('unless there are multiple conditions', () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + if (props.onClick != null && props.onMouseEnter != null) props.onClick(e); + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + ); + }); + }); + + group('handles inline binary expressions', () { test('with a single condition', () async { await testSuggestor( expectedPatchCount: 1, From f4463d61e1aa9e0e4f4dfbd58c70551d0da7e722 Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Thu, 11 Apr 2024 12:51:51 -0600 Subject: [PATCH 07/11] Make cast safer --- .../null_safety_prep/fn_prop_null_aware_call_suggestor.dart | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart index 684a523d..f2c76ed2 100644 --- a/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart +++ b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart @@ -109,6 +109,8 @@ class FnPropNullAwareCallSuggestor extends RecursiveAstVisitor final propFunctionBeingNullChecked = _getPropFunctionBeingNullChecked(condition); + final parent = condition.parent; + if (parent is! IfStatement) return null; final ifStatement = condition.parent! as IfStatement; if (ifStatement.elseStatement != null) return null; if (ifStatement.parent From 4a2de28744448499bdc914de8ee51077b9e9507b Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Thu, 11 Apr 2024 12:55:18 -0600 Subject: [PATCH 08/11] Remove handling of inline binary expressions There are no use cases of this --- .../fn_prop_null_aware_call_suggestor.dart | 69 +++++--------- ...n_prop_null_aware_call_suggestor_test.dart | 89 ------------------- 2 files changed, 20 insertions(+), 138 deletions(-) diff --git a/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart index f2c76ed2..258eceb9 100644 --- a/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart +++ b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart @@ -101,57 +101,28 @@ class FnPropNullAwareCallSuggestor extends RecursiveAstVisitor /// after the null condition is checked. ExpressionStatement? _getPropFunctionExpressionBeingCalledConditionally( BinaryExpression condition) { - if (condition.parent is IfStatement) { - // - // Handles conditions of the form: - // if (props.fn != null) { ... } - // - - final propFunctionBeingNullChecked = - _getPropFunctionBeingNullChecked(condition); - final parent = condition.parent; - if (parent is! IfStatement) return null; - final ifStatement = condition.parent! as IfStatement; - if (ifStatement.elseStatement != null) return null; - if (ifStatement.parent - ?.thisOrAncestorOfType() - ?.elseStatement != - null) { - // There is an else-if statement present - return null; - } - final thenStatement = ifStatement.thenStatement; - if (thenStatement is Block) { - return _getMatchingConditionalPropFunctionCallStatement( - thenStatement.statements, propFunctionBeingNullChecked); - } else if (thenStatement is ExpressionStatement) { - return _getMatchingConditionalPropFunctionCallStatement( - [thenStatement], propFunctionBeingNullChecked); - } + final parent = condition.parent; + if (parent is! IfStatement) return null; + + final propFunctionBeingNullChecked = + _getPropFunctionBeingNullChecked(condition); + final ifStatement = parent; + if (ifStatement.elseStatement != null) return null; + if (ifStatement.parent + ?.thisOrAncestorOfType() + ?.elseStatement != + null) { + // There is an else-if statement present return null; - } else if (condition.parent is ExpressionStatement && - condition.leftOperand is BinaryExpression) { - // - // Handles conditions of the form: - // props.fn != null && ... - // - final propFunctionBeingNullChecked = _getPropFunctionBeingNullChecked( - condition.leftOperand as BinaryExpression); - if (propFunctionBeingNullChecked == null) return null; - - if (condition.rightOperand is! FunctionExpressionInvocation) return null; - final fn = - (condition.rightOperand as FunctionExpressionInvocation).function; - if (fn is! PropertyAccess) return null; - final target = fn.target; - if (target is! SimpleIdentifier) return null; - if (target.name != 'props') return null; - final matches = fn.propertyName.staticElement?.declaration == - propFunctionBeingNullChecked.staticElement?.declaration; - if (!matches) return null; - return condition.parent as ExpressionStatement?; } - + final thenStatement = ifStatement.thenStatement; + if (thenStatement is Block) { + return _getMatchingConditionalPropFunctionCallStatement( + thenStatement.statements, propFunctionBeingNullChecked); + } else if (thenStatement is ExpressionStatement) { + return _getMatchingConditionalPropFunctionCallStatement( + [thenStatement], propFunctionBeingNullChecked); + } return null; } diff --git a/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart index a938d4bf..997e06b3 100644 --- a/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart +++ b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart @@ -267,94 +267,5 @@ void main() { ); }); }); - - group('handles inline binary expressions', () { - test('with a single condition', () async { - await testSuggestor( - expectedPatchCount: 1, - input: withOverReactImport(''' - final Foo = uiFunction( - (props) { - final handleClick = useCallback((e) { - props.onClick != null && props.onClick(e); - }, [props.onClick]); - - return (Dom.button()..onClick = handleClick)(); - }, - UiFactoryConfig(displayName: 'Foo'), - ); - '''), - expectedOutput: withOverReactImport(''' - final Foo = uiFunction( - (props) { - final handleClick = useCallback((e) { - props.onClick?.call(e); - }, [props.onClick]); - - return (Dom.button()..onClick = handleClick)(); - }, - UiFactoryConfig(displayName: 'Foo'), - ); - ''')); - }); - - test( - 'unless the single condition is not a null check of the function being called', - () async { - await testSuggestor( - expectedPatchCount: 0, - input: withOverReactImport(''' - final Foo = uiFunction( - (props) { - final handleClick = useCallback((e) { - 1 > 0 && props.onClick(e); - }, [props.onClick]); - - return (Dom.button()..onClick = handleClick)(); - }, - UiFactoryConfig(displayName: 'Foo'), - ); - '''), - ); - }); - - test( - 'unless the single condition involves the function being called, but is not a null check', - () async { - await testSuggestor( - expectedPatchCount: 0, - input: withOverReactImport(''' - final Foo = uiFunction( - (props) { - final handleClick = useCallback((e) { - props.onClick is Function && props.onClick(e); - }, [props.onClick]); - - return (Dom.button()..onClick = handleClick)(); - }, - UiFactoryConfig(displayName: 'Foo'), - ); - '''), - ); - }); - - test('unless there are multiple conditions', () async { - await testSuggestor( - expectedPatchCount: 0, - input: withOverReactImport(''' - final Foo = uiFunction( - (props) { - final handleClick = useCallback((e) { - 1 > 0 && props.onClick != null && props.onClick(e); - }, [props.onClick]); - - return (Dom.button()..onClick = handleClick)(); - }, - UiFactoryConfig(displayName: 'Foo'), - ); - '''), - ); - }); - }); }); } From 1c7270053b3e17d0c4330a2952fe7cb9646944ce Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Thu, 11 Apr 2024 13:03:44 -0600 Subject: [PATCH 09/11] =?UTF-8?q?Don=E2=80=99t=20handle=20then=20statement?= =?UTF-8?q?s=20containing=20multiple=20statements?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../fn_prop_null_aware_call_suggestor.dart | 42 +++++++++---------- ...n_prop_null_aware_call_suggestor_test.dart | 22 ++++++++++ 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart index 258eceb9..4ac259f9 100644 --- a/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart +++ b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart @@ -17,7 +17,6 @@ import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/token.dart'; import 'package:analyzer/dart/ast/visitor.dart'; import 'package:analyzer/dart/element/type.dart'; -import 'package:collection/collection.dart'; import 'package:over_react_codemod/src/util/class_suggestor.dart'; /// Suggestor that replaces conditional calls to functions declared in props @@ -116,32 +115,33 @@ class FnPropNullAwareCallSuggestor extends RecursiveAstVisitor return null; } final thenStatement = ifStatement.thenStatement; - if (thenStatement is Block) { - return _getMatchingConditionalPropFunctionCallStatement( - thenStatement.statements, propFunctionBeingNullChecked); + if (thenStatement is Block && thenStatement.statements.length == 1) { + if (_isMatchingConditionalPropFunctionCallStatement( + thenStatement.statements.single, propFunctionBeingNullChecked)) { + return thenStatement.statements.single as ExpressionStatement?; + } } else if (thenStatement is ExpressionStatement) { - return _getMatchingConditionalPropFunctionCallStatement( - [thenStatement], propFunctionBeingNullChecked); + if (_isMatchingConditionalPropFunctionCallStatement( + thenStatement, propFunctionBeingNullChecked)) { + return thenStatement; + } } return null; } - ExpressionStatement? _getMatchingConditionalPropFunctionCallStatement( - List thenStatements, + bool _isMatchingConditionalPropFunctionCallStatement( + Statement statementWithinThenStatement, SimpleIdentifier? propFunctionBeingNullChecked) { - return thenStatements.singleWhereOrNull((element) { - if (element is! ExpressionStatement) return false; - final expression = element.expression; - if (expression is! FunctionExpressionInvocation) return false; - final fn = expression.function; - if (fn is! PropertyAccess) return false; - final target = fn.target; - if (target is! SimpleIdentifier) return false; - if (target.name != 'props') return false; - final matches = fn.propertyName.staticElement?.declaration == - propFunctionBeingNullChecked?.staticElement?.declaration; - return matches; - }) as ExpressionStatement?; + if (statementWithinThenStatement is! ExpressionStatement) return false; + final expression = statementWithinThenStatement.expression; + if (expression is! FunctionExpressionInvocation) return false; + final fn = expression.function; + if (fn is! PropertyAccess) return false; + final target = fn.target; + if (target is! SimpleIdentifier) return false; + if (target.name != 'props') return false; + return fn.propertyName.staticElement?.declaration == + propFunctionBeingNullChecked?.staticElement?.declaration; } /// Returns the identifier for the function that is being diff --git a/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart index 997e06b3..663b3de9 100644 --- a/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart +++ b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart @@ -177,6 +177,28 @@ void main() { '''), ); }); + + test('unless there are multiple statements within the then statement', + () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + if (props.onClick != null) { + props.onMouseEnter?.call(e); + props.onClick(e); + } + }, [props.onClick, props.onMouseEnter]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + ); + }); }); group('handles inline if conditions', () { From 4f3ac724c87802fc43cdbf5220e875a5cc0023dc Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Thu, 11 Apr 2024 13:08:15 -0600 Subject: [PATCH 10/11] Moar test cases --- ...n_prop_null_aware_call_suggestor_test.dart | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart index 663b3de9..d7f54777 100644 --- a/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart +++ b/test/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor_test.dart @@ -158,6 +158,28 @@ void main() { ); }); + test('unless the single condition does not involve props at all', + () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final bar = false; + final handleClick = useCallback((e) { + if (bar) { + props.onClick(e); + } + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + ); + }); + test('unless there are multiple conditions', () async { await testSuggestor( expectedPatchCount: 0, @@ -178,6 +200,50 @@ void main() { ); }); + test('unless the relevant prop fn is returned within the then statement', + () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final handleClick = useCallback((e) { + if (props.onClick != null) { + return props.onClick(e); + } + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + ); + }); + + test( + 'unless the relevant prop fn is not called within the then statement', + () async { + await testSuggestor( + expectedPatchCount: 0, + input: withOverReactImport(''' + final Foo = uiFunction( + (props) { + final bar = useState(0); + final handleClick = useCallback((e) { + if (props.onClick != null) { + bar.set(1); + } + }, [props.onClick]); + + return (Dom.button()..onClick = handleClick)(); + }, + UiFactoryConfig(displayName: 'Foo'), + ); + '''), + ); + }); + test('unless there are multiple statements within the then statement', () async { await testSuggestor( From 5a3d61cb33b505d1457cfb36483169ce2ff2b491 Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Thu, 18 Apr 2024 08:07:48 -0700 Subject: [PATCH 11/11] Address CR Feedback --- .../fn_prop_null_aware_call_suggestor.dart | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart index 4ac259f9..9bbc011e 100644 --- a/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart +++ b/lib/src/dart3_suggestors/null_safety_prep/fn_prop_null_aware_call_suggestor.dart @@ -17,6 +17,7 @@ import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/token.dart'; import 'package:analyzer/dart/ast/visitor.dart'; import 'package:analyzer/dart/element/type.dart'; +import 'package:over_react_codemod/src/util.dart'; import 'package:over_react_codemod/src/util/class_suggestor.dart'; /// Suggestor that replaces conditional calls to functions declared in props @@ -107,11 +108,9 @@ class FnPropNullAwareCallSuggestor extends RecursiveAstVisitor _getPropFunctionBeingNullChecked(condition); final ifStatement = parent; if (ifStatement.elseStatement != null) return null; - if (ifStatement.parent - ?.thisOrAncestorOfType() - ?.elseStatement != - null) { - // There is an else-if statement present + if (ifStatement.parent?.tryCast()?.elseStatement == + ifStatement) { + // ifStatement is an else-if return null; } final thenStatement = ifStatement.thenStatement;