diff --git a/.travis.yml b/.travis.yml index 48da05b6b..1a937c0a9 100644 --- a/.travis.yml +++ b/.travis.yml @@ -10,6 +10,6 @@ before_install: - sleep 3 # give xvfb some time to start script: - pub run dart_dev analyze - - pub run dart_dev test + - pub run dart_dev test --integration - ./tool/generate_coverage.sh - bash <(curl -s https://codecov.io/bash) -f coverage/coverage.lcov diff --git a/lib/src/transformer/impl_generation.dart b/lib/src/transformer/impl_generation.dart index bebe4c113..7c0fa81ec 100644 --- a/lib/src/transformer/impl_generation.dart +++ b/lib/src/transformer/impl_generation.dart @@ -200,8 +200,11 @@ class ImplGenerator { ..writeln(); typedPropsFactoryImpl = - ' @override\n' - ' $propsName typedPropsFactory(Map backingMap) => new $propsImplName(backingMap);'; + '@override ' + // Don't type this so that it doesn't interfere with classes with generic parameter props type: + // class FooComponent extends UiComponent {...} + // TODO use long-term solution of component impl class instantiated via factory constructor + 'typedPropsFactory(Map backingMap) => new $propsImplName(backingMap) as dynamic;'; // ---------------------------------------------------------------------- // State implementation @@ -231,8 +234,11 @@ class ImplGenerator { ..writeln(); typedStateFactoryImpl = - ' @override\n' - ' $stateName typedStateFactory(Map backingMap) => new $stateImplName(backingMap);'; + '@override ' + // Don't type this so that it doesn't interfere with classes with generic parameter state type: + // class FooComponent extends UiStatefulComponent {...} + // TODO use long-term solution of component impl class instantiated via factory constructor + 'typedStateFactory(Map backingMap) => new $stateImplName(backingMap) as dynamic;'; } // ---------------------------------------------------------------------- @@ -253,8 +259,6 @@ class ImplGenerator { ..writeln(' @override') ..writeln(' final List \$defaultConsumedProps = ' 'const [$propsName.$staticConsumedPropsName];') - ..writeln(typedPropsFactoryImpl) - ..writeln(typedStateFactoryImpl) ..writeln('}'); if (declarations.component.node.withClause != null) { @@ -273,6 +277,28 @@ class ImplGenerator { ' extends Object with $componentClassImplMixinName' ); } + + var implementsTypedPropsStateFactory = declarations.component.node.members.any((member) => + member is MethodDeclaration && + !member.isStatic && + (member.name.name == 'typedPropsFactory' || member.name.name == 'typedStateFactory') + ); + + if (implementsTypedPropsStateFactory) { + // Can't be an error, because consumers may be implementing typedPropsFactory or typedStateFactory in their components. + logger.warning( + 'Components should not add their own implementions of typedPropsFactory or typedStateFactory.', + span: getSpan(sourceFile, declarations.component.node) + ); + } else { + // For some reason, strong mode is okay with these declarations being in the component, + // but not in the mixin. + // TODO use long-term solution of component impl class instantiated via factory constructor + transformedFile.insert( + sourceFile.location(declarations.component.node.leftBracket.end), + ' /* GENERATED IMPLEMENTATIONS */ $typedPropsFactoryImpl $typedStateFactoryImpl' + ); + } } if (implementations.isNotEmpty) { @@ -422,15 +448,21 @@ class ImplGenerator { annotations.Accessor accessorMeta = instantiateAnnotation(field, annotations.Accessor); annotations.Accessor requiredProp = getConstantAnnotation(field, 'requiredProp', annotations.requiredProp); annotations.Accessor nullableRequiredProp = getConstantAnnotation(field, 'nullableRequiredProp', annotations.nullableRequiredProp); + // ignore: deprecated_member_use annotations.Required requiredMeta = instantiateAnnotation(field, annotations.Required); String individualKeyNamespace = accessorMeta?.keyNamespace ?? keyNamespace; String individualKey = accessorMeta?.key ?? accessorName; - String keyConstantName = '${generatedPrefix}key__$accessorName'; + /// Necessary to work around issue where private static declarations in different classes + /// conflict with each other in strong mode: https://github.com/dart-lang/sdk/issues/29751 + /// TODO remove once that issue is resolved + String staticConstNamespace = typedMap.node.name.name; + + String keyConstantName = '${generatedPrefix}key__${accessorName}__$staticConstNamespace'; String keyValue = stringLiteral(individualKeyNamespace + individualKey); - String constantName = '${generatedPrefix}prop__$accessorName'; + String constantName = '${generatedPrefix}prop__${accessorName}__$staticConstNamespace'; String constantValue = 'const $constConstructorName($keyConstantName'; var annotationCount = 0; diff --git a/lib/transformer.dart b/lib/transformer.dart index 525931b88..24750671b 100644 --- a/lib/transformer.dart +++ b/lib/transformer.dart @@ -69,7 +69,7 @@ class WebSkinDartTransformer extends Transformer implements LazyTransformer { Future apply(Transform transform) async { var primaryInputContents = await transform.primaryInput.readAsString(); - SourceFile sourceFile = new SourceFile(primaryInputContents, url: idToPackageUri(transform.primaryInput.id)); + SourceFile sourceFile = new SourceFile.fromString(primaryInputContents, url: idToPackageUri(transform.primaryInput.id)); TransformedSourceFile transformedFile = new TransformedSourceFile(sourceFile); TransformLogger logger = new JetBrainsFriendlyLogger(transform.logger); diff --git a/test/integration/strong_mode_test.dart b/test/integration/strong_mode_test.dart new file mode 100644 index 000000000..7c1b2f1af --- /dev/null +++ b/test/integration/strong_mode_test.dart @@ -0,0 +1,89 @@ +@TestOn('vm') +library over_react.strong_mode; + +import 'dart:async'; +import 'dart:io'; + +import 'package:dart_dev/util.dart' show copyDirectory; +import 'package:test/test.dart'; + +const String strongModeProject = 'test_fixtures/strong_mode'; + +/// Runs the dart analzyer task for the given project. +Future analyzeProject(String projectPath) async { + // Won't work unless all files are listed, for some reason. + var files = [] + ..addAll(new Directory(projectPath).listSync() + .where((e) => FileSystemEntity.isFileSync(e.path) && e.path.endsWith('.dart')) + .map((e) => e.path) + ); + + var args = ['--strong'] + ..addAll(files); + + return await Process.run('dartanalyzer', args); +} + +// Returns the path to a new temporary copy of the [project] fixture +// testing purposes, necessary since formatter may make changes to ill-formatted files. +String copyProject(String project) { + final String testProject = '${project}_temp'; + + final Directory temp = new Directory(testProject); + copyDirectory(new Directory(project), temp); + addTearDown(() { + // Clean up the temporary test project created for this test case. + temp.deleteSync(recursive: true); + }); + + return testProject; +} + +String getPubspec() { + var lines = [ + '# Generated by strong_mode_test.dart', + 'name: test_fixture', + 'version: 0.0.0', + 'dependencies:', + ' over_react:', + ' path: ../..' + ]; + + return lines.join('\n'); +} + +main() { + group('OverReact strong mode compliance:', () { + test('generates strong mode compliant code', () async { + var testProject = copyProject(strongModeProject); + + new File('$testProject/pubspec.yaml').writeAsStringSync(getPubspec()); + + expect(await Process.run('pub', ['get'], workingDirectory: testProject), succeeded); + expect(await Process.run('pub', ['build', '--mode="debug"'], workingDirectory: testProject), succeeded); + + expect(await analyzeProject('$testProject/build/web'), succeeded); + }); + }); +} + +class _ProcessSucceededMatcher extends Matcher { + const _ProcessSucceededMatcher(); + + @override + Description describe(Description description) => + description.add('a ProcessResult that completed successfully'); + + @override + bool matches(covariant ProcessResult item, _) => + item.exitCode == 0; + + @override + Description describeMismatch(covariant ProcessResult item, Description mismatchDescription, _, __) => + mismatchDescription.add('has exit code ${item.exitCode} and output ${{ + 'stdout': item.stdout.toString(), + 'stderr': item.stderr.toString(), + }}'); +} + +const _ProcessSucceededMatcher succeeded = const _ProcessSucceededMatcher(); diff --git a/test/vm_tests/transformer/declaration_parsing_test.dart b/test/vm_tests/transformer/declaration_parsing_test.dart index cc26c6485..1024d7668 100644 --- a/test/vm_tests/transformer/declaration_parsing_test.dart +++ b/test/vm_tests/transformer/declaration_parsing_test.dart @@ -60,7 +60,7 @@ main() { void setUpAndParse(String source) { logger = new MockTransformLogger(); - sourceFile = new SourceFile(source); + sourceFile = new SourceFile.fromString(source); unit = parseCompilationUnit(source); declarations = new ParsedDeclarations(unit, sourceFile, logger); } diff --git a/test/vm_tests/transformer/impl_generation_test.dart b/test/vm_tests/transformer/impl_generation_test.dart index d259594df..e72d7b44b 100644 --- a/test/vm_tests/transformer/impl_generation_test.dart +++ b/test/vm_tests/transformer/impl_generation_test.dart @@ -39,7 +39,7 @@ main() { void setUpAndParse(String source) { logger = new MockTransformLogger(); - sourceFile = new SourceFile(source); + sourceFile = new SourceFile.fromString(source); transformedFile = new TransformedSourceFile(sourceFile); unit = parseCompilationUnit(source); @@ -336,9 +336,9 @@ main() { } '''); - expect(transformedFile.getTransformedText(), contains('String get foo => props[_\$key__foo];')); + expect(transformedFile.getTransformedText(), contains('String get foo => props[_\$key__foo__AbstractFooProps];')); expect(transformedFile.getTransformedText(), - contains('set foo(covariant String value) => props[_\$key__foo] = value;')); + contains('set foo(covariant String value) => props[_\$key__foo__AbstractFooProps] = value;')); }); group('accessors', () { @@ -640,6 +640,42 @@ main() { verifyTransformedSourceIsValid(); }); + group('a Component', () { + test('implements typedPropsFactory', () { + setUpAndGenerate(''' + @Factory() + UiFactory Foo; + + @Props() + class FooProps {} + + @Component() + class FooComponent { + typedPropsFactory(Map backingMap) => {}; + } + '''); + + verify(logger.warning('Components should not add their own implementions of typedPropsFactory or typedStateFactory.', span: any)); + }); + + test('implements typedStateFactory', () { + setUpAndGenerate(''' + @Factory() + UiFactory Foo; + + @Props() + class FooProps {} + + @Component() + class FooComponent { + typedStateFactory(Map backingMap) => {}; + } + '''); + + verify(logger.warning('Components should not add their own implementions of typedPropsFactory or typedStateFactory.', span: any)); + }); + }); + group('comma-separated variables are declared', () { const String expectedCommaSeparatedWarning = 'Note: accessors declared as comma-separated variables will not all be generated ' diff --git a/test_fixtures/strong_mode/web/abstract_component.dart b/test_fixtures/strong_mode/web/abstract_component.dart new file mode 100644 index 000000000..cee595512 --- /dev/null +++ b/test_fixtures/strong_mode/web/abstract_component.dart @@ -0,0 +1,10 @@ +import 'package:over_react/over_react.dart'; + +@AbstractProps() +abstract class AbstractStatelessProps extends UiProps {} + +@AbstractComponent() +abstract class AbstractStatelessComponent extends UiComponent { + @override + render() { } +} diff --git a/test_fixtures/strong_mode/web/generic_component.dart b/test_fixtures/strong_mode/web/generic_component.dart new file mode 100644 index 000000000..0a726351f --- /dev/null +++ b/test_fixtures/strong_mode/web/generic_component.dart @@ -0,0 +1,16 @@ +import 'package:over_react/over_react.dart'; + +@Factory() +UiFactory GenericStateful; + +@Props() +class GenericStatefulProps extends UiProps {} + +@State() +class GenericStatefulState extends UiState {} + +@Component() +class GenericStatefulComponent extends UiStatefulComponent { + @override + render() { } +} diff --git a/test_fixtures/strong_mode/web/stateful_component.dart b/test_fixtures/strong_mode/web/stateful_component.dart new file mode 100644 index 000000000..5ed4f0206 --- /dev/null +++ b/test_fixtures/strong_mode/web/stateful_component.dart @@ -0,0 +1,16 @@ +import 'package:over_react/over_react.dart'; + +@Factory() +UiFactory Stateful; + +@Props() +class StatefulProps extends UiProps {} + +@State() +class StatefulState extends UiState {} + +@Component() +class StatefulComponent extends UiStatefulComponent { + @override + render() { } +} diff --git a/test_fixtures/strong_mode/web/stateless_component.dart b/test_fixtures/strong_mode/web/stateless_component.dart new file mode 100644 index 000000000..67f698af0 --- /dev/null +++ b/test_fixtures/strong_mode/web/stateless_component.dart @@ -0,0 +1,13 @@ +import 'package:over_react/over_react.dart'; + +@Factory() +UiFactory Stateless; + +@Props() +class StatelessProps extends UiProps {} + +@Component() +class StatelessComponent extends UiComponent { + @override + render() { } +} diff --git a/tool/dev.dart b/tool/dev.dart index d299be00f..202d557e6 100644 --- a/tool/dev.dart +++ b/tool/dev.dart @@ -44,6 +44,9 @@ main(List args) async { 'test/over_react_component_declaration_test.dart', 'test/over_react_component_test.dart', 'test/over_react_util_test.dart', + ] + ..integrationTests = [ + 'test/integration/' ]; config.coverage