diff --git a/lib/src/transformer/declaration_parsing.dart b/lib/src/transformer/declaration_parsing.dart index f1a3f12b3..066afdf95 100644 --- a/lib/src/transformer/declaration_parsing.dart +++ b/lib/src/transformer/declaration_parsing.dart @@ -19,6 +19,7 @@ import 'dart:mirrors'; import 'package:analyzer/analyzer.dart'; import 'package:barback/barback.dart' show TransformLogger; import 'package:over_react/src/component_declaration/annotations.dart' as annotations; +import 'package:over_react/src/transformer/util.dart' show getMetaField; import 'package:source_span/source_span.dart'; import 'package:transformer_utils/src/transformed_source_file.dart' show getSpan; import 'package:transformer_utils/transformer_utils.dart'; @@ -211,15 +212,7 @@ class ParsedDeclarations { }); } else { void validateMetaField(ClassDeclaration cd, String expectedType) { - bool isPropsOrStateMeta(ClassMember member) { - if (member is! FieldDeclaration) return false; - final FieldDeclaration fd = member; - if (!fd.isStatic) return false; - if (fd.fields.variables.length > 1) return false; - if (fd.fields.variables.single.name.name != 'meta') return false; - return true; - } - final FieldDeclaration metaField = cd.members.firstWhere(isPropsOrStateMeta, orElse: () => null); + final metaField = getMetaField(cd); if (metaField == null) return; if (metaField.fields.type?.toSource() != expectedType) { @@ -229,15 +222,17 @@ class ParsedDeclarations { ); } final isClassPrivate = cd.name.name.startsWith('_'); - final expectedInitializer = isClassPrivate - ? '_\$metaFor${cd.name.name.substring(1)}' - : '\$metaFor${cd.name.name}'; + var expectedInitializers = ['\$metaFor${cd.name.name}', '_\$metaFor${cd.name.name}']; + if (isClassPrivate) { + expectedInitializers.add('_\$metaFor${cd.name.name.substring(1)}'); + } final initializer = metaField.fields.variables.single.initializer?.toSource(); - if (initializer != expectedInitializer) { + if (!expectedInitializers.contains(initializer)) { error( - 'Static $expectedType field in accessor class must be initialized to ' - '`$expectedInitializer`', + 'Static $expectedType field in accessor class must be initialized to:' + // The second in the list of expected initializers is the one it will need to be once on Dart 2 + '`${expectedInitializers[1]}`', getSpan(sourceFile, metaField), ); } @@ -280,6 +275,36 @@ class ParsedDeclarations { } } + // validate that the factory is initialized correctly + final factory = declarationMap[key_factory].length <= 1 ? singleOrNull(declarationMap[key_factory]) : null; + if (factory != null && factory is TopLevelVariableDeclaration) { + final String factoryName = factory.variables.variables.first.name.name; + + if (factory.variables.variables.length != 1) { + error('Factory declarations must be a single variable.', + getSpan(sourceFile, factory.variables)); + } + + final variable = factory.variables.variables.first; + final isPrivate = factoryName.startsWith('_'); + var expectedInitializers = ['\$$factoryName', '_\$$factoryName']; + + if (isPrivate) { + expectedInitializers.add('_\$${factoryName.substring(1)}'); + } + + if (variable.initializer != null && + !expectedInitializers.contains(variable.initializer.toString())) { + error( + 'Factory variables are stubs for the generated factories, and should not have initializers ' + 'unless initialized with a valid variable name for Dart 2 builder compatibility. ' + // The second in the list of expected initializers is the one it will need to be once on Dart 2 + 'Should be:\n ${expectedInitializers[1]}', + getSpan(sourceFile, variable.initializer) + ); + } + } + if (hasErrors) { for (final key in declarationMap.keys) { declarationMap[key] = []; diff --git a/lib/src/transformer/impl_generation.dart b/lib/src/transformer/impl_generation.dart index 520410dd8..7f1a457ee 100644 --- a/lib/src/transformer/impl_generation.dart +++ b/lib/src/transformer/impl_generation.dart @@ -18,7 +18,7 @@ import 'package:analyzer/analyzer.dart'; import 'package:barback/barback.dart'; import 'package:over_react/src/component_declaration/annotations.dart' as annotations; import 'package:over_react/src/transformer/declaration_parsing.dart'; -import 'package:over_react/src/transformer/text_util.dart'; +import 'package:over_react/src/transformer/util.dart'; import 'package:source_span/source_span.dart'; import 'package:transformer_utils/src/text_util.dart' show stringLiteral; import 'package:transformer_utils/src/transformed_source_file.dart' show getSpan; @@ -105,26 +105,6 @@ class ImplGenerator { // Factory implementation // ---------------------------------------------------------------------- - if (declarations.factory.node.variables.variables.length != 1) { - logger.error('Factory declarations must a single variable.', - span: getSpan(sourceFile, declarations.factory.node.variables)); - } - - declarations.factory.node.variables.variables.forEach((variable) { - final isPrivate = factoryName.startsWith(privatePrefix); - final validInitializer = isPrivate - ? '$generatedPrefix${factoryName.substring(privatePrefix.length)}' - : '$publicGeneratedPrefix$factoryName'; - if (variable.initializer != null && variable.initializer.toString() != validInitializer) { - logger.error( - 'Factory variables are stubs for the generated factories, and should not have initializers ' - 'unless initialized with \$$factoryName for Dart 2 builder compatibility. ' - 'Should be:\n $validInitializer', - span: getSpan(sourceFile, variable.initializer) - ); - } - }); - transformedFile.replace( sourceFile.span( declarations.factory.node.variables.variables.first.name.end, @@ -682,27 +662,30 @@ class ImplGenerator { staticGettersCompanionImpl ); } - - final name = (companionNode ?? typedMap.node).name.name; - final isPrivate = name.startsWith(privatePrefix); - final publicName = isPrivate ? name.substring(privatePrefix.length) : name; - final metaClassName = '$generatedPrefix${name}Meta'; - final metaInstanceName = isPrivate - ? '${generatedPrefix}metaFor$publicName' - : '${publicGeneratedPrefix}metaFor$publicName'; - final metaStructName = type == AccessorType.props - ? 'PropsMeta' - : 'StateMeta'; + + final classDeclaration = (companionNode ?? typedMap.node); + final metaField = getMetaField(classDeclaration); final output = new StringBuffer(); - output.writeln('/// A class that allows us to reuse generated code from the accessors class.'); - output.writeln('/// This is only used by other generated code, and can be simplified if needed.'); - output.writeln('class $metaClassName {'); - output.writeln(staticVariablesImpl); - output.writeln('}'); - output.writeln('const $metaStructName $metaInstanceName = const $metaStructName('); - output.writeln(' fields: $metaClassName.$constantListName,'); - output.writeln(' keys: $metaClassName.$keyListName,'); - output.writeln(');'); + // if metaField is null, we are on Dart 1 code which has not transitioned + // to the transitional Dart 2 compatible boilerplate, and thus the $metaFor + // constant is not needed + if (metaField != null) { + final name = classDeclaration.name.name; + final metaClassName = '$generatedPrefix${name}Meta'; + final metaInstanceName = metaField.fields.variables.single.initializer.toSource(); + final metaStructName = type == AccessorType.props + ? 'PropsMeta' + : 'StateMeta'; + output.writeln('/// A class that allows us to reuse generated code from the accessors class.'); + output.writeln('/// This is only used by other generated code, and can be simplified if needed.'); + output.writeln('class $metaClassName {'); + output.writeln(staticVariablesImpl); + output.writeln('}'); + output.writeln('const $metaStructName $metaInstanceName = const $metaStructName('); + output.writeln(' fields: $metaClassName.$constantListName,'); + output.writeln(' keys: $metaClassName.$keyListName,'); + output.writeln(');'); + } return new AccessorOutput(output.toString()); } diff --git a/lib/src/transformer/text_util.dart b/lib/src/transformer/util.dart similarity index 64% rename from lib/src/transformer/text_util.dart rename to lib/src/transformer/util.dart index 8ac2459d9..512284c7f 100644 --- a/lib/src/transformer/text_util.dart +++ b/lib/src/transformer/util.dart @@ -12,7 +12,9 @@ // See the License for the specific language governing permissions and // limitations under the License. -library over_react.transformer.text_util; +library over_react.transformer.util; + +import 'package:analyzer/analyzer.dart'; String commentBanner(String bannerText, { int bannerWidth: 80, int textIndent: 2, bool topBorder: true, bool bottomBorder: true @@ -30,3 +32,17 @@ String commentBanner(String bannerText, { (bottomBorder ? bannerBorder : '') + '\n'; } + +/// Returns a [FieldDeclaration] for the meta field on a [ClassDeclaration] if +/// it exists, otherwise returns null. +FieldDeclaration getMetaField(ClassDeclaration cd) { + bool isPropsOrStateMeta(ClassMember member) { + if (member is! FieldDeclaration) return false; + final FieldDeclaration fd = member; + if (!fd.isStatic) return false; + if (fd.fields.variables.length > 1) return false; + if (fd.fields.variables.single.name.name != 'meta') return false; + return true; + } + return cd.members.firstWhere(isPropsOrStateMeta, orElse: () => null); +} diff --git a/test/vm_tests/transformer/declaration_parsing_test.dart b/test/vm_tests/transformer/declaration_parsing_test.dart index 4505bbef6..15ff3fef3 100644 --- a/test/vm_tests/transformer/declaration_parsing_test.dart +++ b/test/vm_tests/transformer/declaration_parsing_test.dart @@ -362,6 +362,77 @@ main() { }); }); + const String restOfComponent = ''' + @Props() + class FooProps {} + + @Component() + class FooComponent {} + '''; + + group('does not log a hard error when', () { + group('the factory is', () { + group('public and initialized with', () { + test('correct \$ prefixed variable name', () { + setUpAndParse(''' + @Factory() + UiFactory Foo = \$Foo; + + $restOfComponent + '''); + + verifyNever(logger.error(any, span: any)); + }); + + test('correct _\$ prefixed variable name', () { + setUpAndParse(''' + @Factory() + UiFactory Foo = _\$Foo; + + $restOfComponent + '''); + + verifyNever(logger.error(any, span: any)); + }); + }); + + group('private and initialized with', () { + test('correct \$ prefixed variable name', () { + setUpAndParse(''' + @Factory() + UiFactory _Foo = \$_Foo; + + $restOfComponent + '''); + + verifyNever(logger.error(any, span: any)); + }); + + test('correct _\$ prefixed variable name', () { + setUpAndParse(''' + @Factory() + UiFactory _Foo = _\$_Foo; + + $restOfComponent + '''); + + verifyNever(logger.error(any, span: any)); + }); + + test('_\$ prefixed variable name with private prefix stripped', () { + setUpAndParse(''' + @Factory() + UiFactory _Foo = _\$Foo; + + $restOfComponent + '''); + + verifyNever(logger.error(any, span: any)); + }); + }); + }); + }); + group('and logs a hard error when', () { const String factorySrc = '\n@Factory()\nUiFactory Foo;\n'; const String propsSrc = '\n@Props()\nclass FooProps {}\n'; @@ -497,6 +568,47 @@ main() { }); }); + group('a factory is', () { + test('declared using multiple variables', () { + setUpAndParse(''' + @Factory() + UiFactory Foo, Bar; + + $restOfComponent + '''); + + verify(logger.error('Factory declarations must be a single variable.', span: any)); + }); + + test('public and declared with an invalid initializer', () { + setUpAndParse(''' + @Factory() + UiFactory Foo = null; + + $restOfComponent + '''); + + verify(logger.error( + 'Factory variables are stubs for the generated factories, and should not have initializers' + ' unless initialized with a valid variable name for Dart 2 builder compatibility. Should be:\n' + ' _\$Foo', span: any)); + }); + + test('private and declared with an invalid initializer', () { + setUpAndParse(''' + @Factory() + UiFactory _Foo = null; + + $restOfComponent + '''); + + verify(logger.error( + 'Factory variables are stubs for the generated factories, and should not have initializers' + ' unless initialized with a valid variable name for Dart 2 builder compatibility. Should be:\n' + ' _\$_Foo', span: any)); + }); + }); + group('a static meta field', () { group('for a props class', () { test('has the wrong type', () { @@ -514,7 +626,18 @@ main() { static const PropsMeta meta = \$metaForBarProps; } '''); - verify(logger.error('Static PropsMeta field in accessor class must be initialized to `\$metaForFooProps`', span: any)); + verify(logger.error('Static PropsMeta field in accessor class must be initialized to:' + '`_\$metaForFooProps`', span: any)); + }); + + test('is private and initialized incorrectly', () { + setUpAndParse(factorySrc + privatePropsSrc + componentSrc + ''' + class _FooProps { + static const PropsMeta meta = \$metaForBarProps; + } + '''); + verify(logger.error('Static PropsMeta field in accessor class must be initialized to:' + '`_\$metaFor_FooProps`', span: any)); }); }); @@ -534,7 +657,18 @@ main() { static const StateMeta meta = \$metaForBarState; } '''); - verify(logger.error('Static StateMeta field in accessor class must be initialized to `\$metaForFooState`', span: any)); + verify(logger.error('Static StateMeta field in accessor class must be initialized to:' + '`_\$metaForFooState`', span: any)); + }); + + test('is private and initialized incorrectly', () { + setUpAndParse(factorySrc + propsSrc + privateStateSrc + componentSrc + ''' + class _FooState { + static const StateMeta meta = \$metaForBarState; + } + '''); + verify(logger.error('Static StateMeta field in accessor class must be initialized to:' + '`_\$metaFor_FooState`', span: any)); }); }); @@ -556,7 +690,19 @@ main() { static const PropsMeta meta = \$metaForAbstractBarProps; } '''); - verify(logger.error('Static PropsMeta field in accessor class must be initialized to `\$metaForAbstractFooProps`', span: any)); + verify(logger.error('Static PropsMeta field in accessor class must be initialized to:' + '`_\$metaForAbstractFooProps`', span: any)); + }); + + test('is private and initialized incorrectly', () { + setUpAndParse(''' + @AbstractProps() abstract class _\$AbstractFooProps {} + abstract class _AbstractFooProps { + static const PropsMeta meta = \$metaForAbstractBarProps; + } + '''); + verify(logger.error('Static PropsMeta field in accessor class must be initialized to:' + '`_\$metaFor_AbstractFooProps`', span: any)); }); }); @@ -578,7 +724,19 @@ main() { static const StateMeta meta = \$metaForAbstractBarState; } '''); - verify(logger.error('Static StateMeta field in accessor class must be initialized to `\$metaForAbstractFooState`', span: any)); + verify(logger.error('Static StateMeta field in accessor class must be initialized to:' + '`_\$metaForAbstractFooState`', span: any)); + }); + + test('is private and initialized incorrectly', () { + setUpAndParse(''' + @AbstractState() abstract class _\$AbstractFooState {} + abstract class _AbstractFooState { + static const StateMeta meta = \$metaForAbstractBarState; + } + '''); + verify(logger.error('Static StateMeta field in accessor class must be initialized to:' + '`_\$metaFor_AbstractFooState`', span: any)); }); }); @@ -598,7 +756,18 @@ main() { static const PropsMeta meta = \$metaForBarPropsMixin; } '''); - verify(logger.error('Static PropsMeta field in accessor class must be initialized to `\$metaForFooPropsMixin`', span: any)); + verify(logger.error('Static PropsMeta field in accessor class must be initialized to:' + '`_\$metaForFooPropsMixin`', span: any)); + }); + + test('is private and initialized incorrectly', () { + setUpAndParse(''' + @PropsMixin() abstract class _FooPropsMixin { + static const PropsMeta meta = \$metaForBarPropsMixin; + } + '''); + verify(logger.error('Static PropsMeta field in accessor class must be initialized to:' + '`_\$metaFor_FooPropsMixin`', span: any)); }); }); @@ -618,7 +787,18 @@ main() { static const StateMeta meta = \$metaForBarStateMixin; } '''); - verify(logger.error('Static StateMeta field in accessor class must be initialized to `\$metaForFooStateMixin`', span: any)); + verify(logger.error('Static StateMeta field in accessor class must be initialized to:' + '`_\$metaForFooStateMixin`', span: any)); + }); + + test('is private and initialized incorrectly', () { + setUpAndParse(''' + @StateMixin() abstract class _FooStateMixin { + static const StateMeta meta = \$metaForBarStateMixin; + } + '''); + verify(logger.error('Static StateMeta field in accessor class must be initialized to:' + '`_\$metaFor_FooStateMixin`', span: any)); }); }); }); diff --git a/test/vm_tests/transformer/impl_generation_test.dart b/test/vm_tests/transformer/impl_generation_test.dart index 0d14479df..3b66a5f19 100644 --- a/test/vm_tests/transformer/impl_generation_test.dart +++ b/test/vm_tests/transformer/impl_generation_test.dart @@ -765,51 +765,7 @@ main() { }); group('logs an error when', () { - group('a factory is', () { - const String restOfComponent = ''' - @Props() - class FooProps {} - - @Component() - class FooComponent {} - '''; - - test('declared using multiple variables', () { - setUpAndGenerate(''' - @Factory() - UiFactory Foo, Bar; - $restOfComponent - '''); - - verify(logger.error('Factory declarations must a single variable.', span: any)); - }); - - test('declared with an initializer', () { - setUpAndGenerate(''' - @Factory() - UiFactory Foo = null; - - $restOfComponent - '''); - - verify(logger.error('Factory variables are stubs for the generated factories, and should not have initializers' - ' unless initialized with \$Foo for Dart 2 builder compatibility. Should be:\n' - ' \$Foo', span: any)); - }); - - test('declared with an \$ prefixed initializer matching the factory name', () { - setUpAndGenerate(''' - @Factory() - UiFactory Foo = \$Foo; - - $restOfComponent - '''); - - verifyNever(logger.error('Factory variables are stubs for the generated factories, and should not have initializers' - ' unless initialized with \$Foo for Dart 2 builder compatibility.', span: any)); - }); - }); group('a component class', () { test('subtypes itself', () { diff --git a/test/vm_tests/transformer/text_util_test.dart b/test/vm_tests/transformer/util_test.dart similarity index 98% rename from test/vm_tests/transformer/text_util_test.dart rename to test/vm_tests/transformer/util_test.dart index f8e0583f6..14167209f 100644 --- a/test/vm_tests/transformer/text_util_test.dart +++ b/test/vm_tests/transformer/util_test.dart @@ -15,7 +15,7 @@ @TestOn('vm') library text_util_test; -import 'package:over_react/src/transformer/text_util.dart'; +import 'package:over_react/src/transformer/util.dart'; import 'package:test/test.dart'; main() {