From 147eba429c752a3b23bc2cdaaaf73e3d5c05ba01 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Tue, 26 Jan 2021 14:28:45 -0800 Subject: [PATCH 1/3] Fix three little issues in mustachio codegen: * Allow duplicate types in different `@Renderer` specifications. * Only build each render class and renderer method once; you cannot declare two classes or functions with the same name. * Do not attempt to add generators for type parameters; instead, use the bound if it is not `dynamic`. --- test/mustachio/builder_test.dart | 33 +++++++++++++----- tool/mustachio/codegen_runtime_renderer.dart | 35 +++++++++++++++----- 2 files changed, 50 insertions(+), 18 deletions(-) diff --git a/test/mustachio/builder_test.dart b/test/mustachio/builder_test.dart index 18f58a7a4c..0dcf6a8374 100644 --- a/test/mustachio/builder_test.dart +++ b/test/mustachio/builder_test.dart @@ -25,7 +25,7 @@ class Context { }; const _libraryFrontMatter = ''' -@Renderer(#renderFoo, Context(), visibleTypes: {Bar}) +@Renderer(#renderFoo, Context(), visibleTypes: {Bar, Baz}) library foo; import 'package:mustachio/annotations.dart'; '''; @@ -73,15 +73,19 @@ $sourceLibraryContent setUpAll(() async { writer = InMemoryAssetWriter(); await testMustachioBuilder(''' -abstract class FooBase { +abstract class FooBase2 { + T get generic; +} +abstract class FooBase extends FooBase2 { Bar get bar; } -class Foo extends FooBase { +abstract class Foo extends FooBase { String s1 = "s1"; bool b1 = false; List l1 = [1, 2, 3]; } class Bar {} +class Baz {} '''); renderersLibrary = await resolveGeneratedLibrary(writer); var rendererAsset = AssetId.parse('foo|lib/foo.renderers.dart'); @@ -92,11 +96,13 @@ class Bar {} // The render function for Foo expect( generatedContent, - contains('String _render_FooBase(\n' - ' FooBase context, List ast, Template template,')); + contains('String _render_FooBase(\n' + ' FooBase context, List ast, Template template,')); // The renderer class for Foo - expect(generatedContent, - contains('class _Renderer_FooBase extends RendererBase')); + expect( + generatedContent, + contains( + 'class _Renderer_FooBase extends RendererBase>')); }); test('for Object', () { @@ -121,6 +127,11 @@ class Bar {} expect(renderersLibrary.getType('_Renderer_Bar'), isNotNull); }); + test('for a generic, bounded type found in a getter', () { + expect(renderersLibrary.getTopLevelFunction('_render_Baz'), isNotNull); + expect(renderersLibrary.getType('_Renderer_Baz'), isNotNull); + }); + test('with a property map', () { expect( generatedContent, @@ -130,7 +141,7 @@ class Bar {} test('with a property map which references the superclass', () { expect(generatedContent, - contains('..._Renderer_FooBase.propertyMap(),')); + contains('..._Renderer_FooBase.propertyMap(),')); }); test('with a property map with a bool property', () { @@ -203,6 +214,7 @@ class Bar {} await testMustachioBuilder(''' class Foo {} class Bar {} +class Baz {} ''', libraryFrontMatter: ''' @Renderer(#renderFoo, Context()) @Renderer(#renderBar, Context()) @@ -229,6 +241,7 @@ class FooBase {} class Foo extends FooBase {} class BarBase {} class Bar extends BarBase {} +class Baz {} ''', libraryFrontMatter: ''' @Renderer(#renderFoo, Context()) @Renderer(#renderBar, Context()) @@ -275,6 +288,7 @@ import 'package:mustachio/annotations.dart'; await testMustachioBuilder(''' class Foo {} class Bar {} +class Baz {} '''); var renderersLibrary = await resolveGeneratedLibrary(writer); @@ -295,13 +309,14 @@ class Bar {} setUpAll(() async { writer = InMemoryAssetWriter(); await testMustachioBuilder(''' -class Foo { +abstract class Foo { static Static get static1 => Bar(); Private get _private1 => Bar(); void set setter1(Setter s); Method method1(Method m); } class Bar {} +class Baz {} class Static {} class Private {} class Setter {} diff --git a/tool/mustachio/codegen_runtime_renderer.dart b/tool/mustachio/codegen_runtime_renderer.dart index 8e930f07b4..5e4210aa4c 100644 --- a/tool/mustachio/codegen_runtime_renderer.dart +++ b/tool/mustachio/codegen_runtime_renderer.dart @@ -101,10 +101,13 @@ String _simpleResolveErrorMessage(List key, String type) => specs.forEach(_addTypesForRendererSpec); + var builtRenderers = {}; + while (_typesToProcess.isNotEmpty) { var info = _typesToProcess.removeFirst(); - if (info.isFullRenderer) { + if (info.isFullRenderer && !builtRenderers.contains(info._contextClass)) { + builtRenderers.add(info._contextClass); _buildRenderer(info); } } @@ -142,6 +145,18 @@ String _simpleResolveErrorMessage(List key, String type) => if (property.isPrivate || property.isStatic || property.isSetter) return; if (property.hasProtected || property.hasVisibleForTesting) return; var type = property.type.returnType; + if (type is TypeParameterType) { + if ((type as TypeParameterType).bound == null || + (type as TypeParameterType).bound.isDynamic) { + // Don't add functions for a generic type, for example + // `List.first` has type `E`, which we don't have a specific + // renderer for. + // TODO(srawlins): Find a solution for this. We can track all of the + // concrete types substituted for `E` for example. + return; + } + type = (type as TypeParameterType).bound; + } var isFullRenderer = _isVisibleToMustache(type.element); if (_typeSystem.isAssignableTo(type, _typeProvider.iterableDynamicType)) { @@ -170,9 +185,8 @@ String _simpleResolveErrorMessage(List key, String type) => /// Adds [type] to the [_typesToProcess] queue, if it is not already there. void _addTypeToProcess(ClassElement element, {@required isFullRenderer}) { - var typeToProcess = _typesToProcess - .singleWhere((rs) => rs._contextClass == element, orElse: () => null); - if (typeToProcess == null) { + var types = _typesToProcess.where((rs) => rs._contextClass == element); + if (types.isEmpty) { var rendererInfo = _RendererInfo(element, isFullRenderer: isFullRenderer, public: _rendererClassesArePublic); _typesToProcess.add(rendererInfo); @@ -181,11 +195,14 @@ String _simpleResolveErrorMessage(List key, String type) => _typeToRendererClassName[element] = rendererInfo._rendererClassName; } } else { - if (isFullRenderer && !typeToProcess.isFullRenderer) { - // This is the only case in which we update a type-to-render. - typeToProcess.isFullRenderer = true; - _typeToRenderFunctionName[element] = typeToProcess._renderFunctionName; - _typeToRendererClassName[element] = typeToProcess._rendererClassName; + for (var typeToProcess in types) { + if (isFullRenderer && !typeToProcess.isFullRenderer) { + // This is the only case in which we update a type-to-render. + typeToProcess.isFullRenderer = true; + _typeToRenderFunctionName[element] = + typeToProcess._renderFunctionName; + _typeToRendererClassName[element] = typeToProcess._rendererClassName; + } } } } From 3310ee926b1d19b5913862e61a2bd71743e544f2 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Tue, 26 Jan 2021 14:32:52 -0800 Subject: [PATCH 2/3] style --- tool/mustachio/codegen_runtime_renderer.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tool/mustachio/codegen_runtime_renderer.dart b/tool/mustachio/codegen_runtime_renderer.dart index 5e4210aa4c..e1c4871f87 100644 --- a/tool/mustachio/codegen_runtime_renderer.dart +++ b/tool/mustachio/codegen_runtime_renderer.dart @@ -146,8 +146,8 @@ String _simpleResolveErrorMessage(List key, String type) => if (property.hasProtected || property.hasVisibleForTesting) return; var type = property.type.returnType; if (type is TypeParameterType) { - if ((type as TypeParameterType).bound == null || - (type as TypeParameterType).bound.isDynamic) { + var bound = (type as TypeParameterType).bound; + if (bound == null || bound.isDynamic) { // Don't add functions for a generic type, for example // `List.first` has type `E`, which we don't have a specific // renderer for. @@ -155,7 +155,7 @@ String _simpleResolveErrorMessage(List key, String type) => // concrete types substituted for `E` for example. return; } - type = (type as TypeParameterType).bound; + type = bound; } var isFullRenderer = _isVisibleToMustache(type.element); From 9704ed455a0217e7bdfca9a8128b300529d0ea58 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Wed, 27 Jan 2021 08:23:11 -0800 Subject: [PATCH 3/3] Fix deduplication logic --- tool/mustachio/codegen_runtime_renderer.dart | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/tool/mustachio/codegen_runtime_renderer.dart b/tool/mustachio/codegen_runtime_renderer.dart index e1c4871f87..9e85bd56f1 100644 --- a/tool/mustachio/codegen_runtime_renderer.dart +++ b/tool/mustachio/codegen_runtime_renderer.dart @@ -106,9 +106,11 @@ String _simpleResolveErrorMessage(List key, String type) => while (_typesToProcess.isNotEmpty) { var info = _typesToProcess.removeFirst(); - if (info.isFullRenderer && !builtRenderers.contains(info._contextClass)) { + if (info.isFullRenderer) { + var buildOnlyPublicFunction = + builtRenderers.contains(info._contextClass); + _buildRenderer(info, buildOnlyPublicFunction: buildOnlyPublicFunction); builtRenderers.add(info._contextClass); - _buildRenderer(info); } } @@ -218,14 +220,19 @@ String _simpleResolveErrorMessage(List key, String type) => return _isVisibleToMustache(element.supertype.element); } - /// Builds both the render function and the renderer class for [renderer]. + /// Builds render functions and the renderer class for [renderer]. /// /// The function and the class are each written as Dart code to [_buffer]. /// /// If [renderer] also specifies a `publicApiFunctionName`, then a public API /// function (which renders a context object using a template file at a path, /// rather than an AST) is also written. - void _buildRenderer(_RendererInfo renderer) { + /// + /// If [buildOnlyPublicFunction] is true, then the private render function and + /// renderer classes are not built, having been built for a different + /// [_RendererInfo]. + void _buildRenderer(_RendererInfo renderer, + {@required bool buildOnlyPublicFunction}) { var typeName = renderer._typeName; var typeWithVariables = '$typeName${renderer._typeVariablesString}'; @@ -238,6 +245,8 @@ String ${renderer.publicApiFunctionName}${renderer._typeParametersString}( '''); } + if (buildOnlyPublicFunction) return; + // Write out the render function. _buffer.writeln(''' String ${renderer._renderFunctionName}${renderer._typeParametersString}(