Merge pull request #81 from Workiva/UIP-1638_strong_transformer/dev

UIP-1638: Make transformer output strong mode compliant

Closes #14

.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

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<T extends FooProps> extends UiComponent<T> {...}
+          // 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<T extends FooProps, T extends FooState> extends UiStatefulComponent<T> {...}
+          // 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<ConsumedProps> \$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;

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);
 

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<ProcessResult> analyzeProject(String projectPath) async {
+  // Won't work unless all files are listed, for some reason.
+  var files = <String>[]
+    ..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 = <String>[
+    '# 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();

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);
       }

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<FooProps> 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<FooProps> 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 '

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<T extends AbstractStatelessProps> extends UiComponent<T> {
+  @override
+  render() { }
+}

test_fixtures/strong_mode/web/generic_component.dart

@@ -0,0 +1,16 @@
+import 'package:over_react/over_react.dart';
+
+@Factory()
+UiFactory<GenericStatefulProps> GenericStateful;
+
+@Props()
+class GenericStatefulProps extends UiProps {}
+
+@State()
+class GenericStatefulState extends UiState {}
+
+@Component()
+class GenericStatefulComponent<T extends GenericStatefulProps, S extends GenericStatefulState> extends UiStatefulComponent<T, S> {
+  @override
+  render() { }
+}

test_fixtures/strong_mode/web/stateful_component.dart

@@ -0,0 +1,16 @@
+import 'package:over_react/over_react.dart';
+
+@Factory()
+UiFactory<StatefulProps> Stateful;
+
+@Props()
+class StatefulProps extends UiProps {}
+
+@State()
+class StatefulState extends UiState {}
+
+@Component()
+class StatefulComponent extends UiStatefulComponent<StatefulProps, StatefulState> {
+  @override
+  render() { }
+}

test_fixtures/strong_mode/web/stateless_component.dart

@@ -0,0 +1,13 @@
+import 'package:over_react/over_react.dart';
+
+@Factory()
+UiFactory<StatelessProps> Stateless;
+
+@Props()
+class StatelessProps extends UiProps {}
+
+@Component()
+class StatelessComponent extends UiComponent<StatelessProps> {
+  @override
+  render() { }
+}

tool/dev.dart

@@ -44,6 +44,9 @@ main(List<String> 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