From 0229a99ca95c5ea930d9f16630f93366de3f5f9c Mon Sep 17 00:00:00 2001 From: Scott Hyndman Date: Thu, 16 Jan 2020 11:01:25 -0600 Subject: [PATCH] Fix @observable generic types when using import aliases There were a number of bugs with the previous implementation of the `LibraryScopedNameFinder`. This resolves them, as well as ensures that a single code path is followed whether or not the analyzed source code contains named imports, reducing the potential for future bugs. The following bugs have been corrected when using named imports: * Missing type arguments on classes * Missing type arguments on function typedefs * Missing prefixes from imported typedefs Fixes #367 MR: import_alias_generics --- mobx_codegen/lib/src/type_names.dart | 88 ++++++----- mobx_codegen/pubspec.yaml | 7 +- .../test/data/valid_generic_store_input.dart | 3 + .../test/data/valid_generic_store_output.dart | 17 +++ .../data/valid_import_prefixed_input.dart | 29 +++- .../data/valid_import_prefixed_output.dart | 140 ++++++++++++++++-- 6 files changed, 222 insertions(+), 62 deletions(-) diff --git a/mobx_codegen/lib/src/type_names.dart b/mobx_codegen/lib/src/type_names.dart index 717939693..659e4a37a 100644 --- a/mobx_codegen/lib/src/type_names.dart +++ b/mobx_codegen/lib/src/type_names.dart @@ -1,7 +1,5 @@ import 'package:analyzer/dart/element/element.dart'; import 'package:analyzer/dart/element/type.dart'; -// ignore: implementation_imports -import 'package:analyzer/src/dart/resolver/scope.dart'; import 'package:mobx_codegen/src/template/comma_list.dart'; /// Determines the names of types within the context of a library, determining @@ -29,17 +27,20 @@ class LibraryScopedNameFinder { return _namesByElement; } - // Reverse each import's export namespace, so that we can easily map - // Elements to names. _namesByElement = {}; - final namespaceBuilder = NamespaceBuilder(); - for (final import in library.imports) { - // Build a namespace so that combinators (show/hide) can be computed - final namespace = - namespaceBuilder.createImportNamespaceForDirective(import); - // Record each element by name - for (final entry in namespace.definedNames.entries) { + // Add all of this library's type-defining elements to the name map + final libraryElements = + library.topLevelElements.whereType(); + for (final element in libraryElements) { + _namesByElement[element] = element.name; + } + + // Reverse each import's export namespace so we can map elements to their + // library-local names. Note that the definedNames include a prefix if there + // is one. + for (final import in library.imports) { + for (final entry in import.namespace.definedNames.entries) { _namesByElement[entry.value] = entry.key; } } @@ -48,7 +49,7 @@ class LibraryScopedNameFinder { } String findVariableTypeName(VariableElement variable) => - _getTypeName(variable.type); + _getDartTypeName(variable.type); String findGetterTypeName(PropertyAccessorElement getter) { assert(getter.isGetter); @@ -56,46 +57,41 @@ class LibraryScopedNameFinder { } String findParameterTypeName(ParameterElement parameter) => - _getTypeName(parameter.type); + _getDartTypeName(parameter.type); String findReturnTypeName(FunctionTypedElement executable) => - _getTypeName(executable.returnType); + _getDartTypeName(executable.returnType); List findReturnTypeArgumentTypeNames(ExecutableElement executable) { final returnType = executable.returnType; return returnType is ParameterizedType - ? returnType.typeArguments.map(_getTypeName).toList() + ? returnType.typeArguments.map(_getDartTypeName).toList() : []; } String findTypeParameterBoundsTypeName(TypeParameterElement typeParameter) { assert(typeParameter.bound != null); - return _getTypeName(typeParameter.bound); + return _getDartTypeName(typeParameter.bound); } /// Calculates a type name, including its type arguments /// /// The returned string will include import prefixes on all applicable types. - String _getTypeName(DartType type) { - final typeElement = type.element; + String _getDartTypeName(DartType type) { + var typeElement = type.element; if (type is FunctionType) { - // A type with a typedef should be written as is - if (typeElement is GenericFunctionTypeElement && - typeElement.enclosingElement is GenericTypeAliasElement) { - return typeElement.enclosingElement.displayName; + // If we're dealing with a typedef, we let it undergo the standard name + // lookup. Otherwise, we special case the function naming. + if (typeElement?.enclosingElement is GenericTypeAliasElement) { + typeElement = typeElement.enclosingElement; + } else { + return _getFunctionTypeName(type); } - - return _findFunctionTypeName(type); } else if ( - // If the library has no named imports, we don't need to worry about - // prefixes - library.prefixes.isEmpty || - // Some types don't have associated elements, like void - typeElement == null || + // Some types don't have associated elements, like void + typeElement == null || // This is a bare type param, like "T" - type is TypeParameterType || - // If the type lives in this library, no prefix necessary - typeElement.library == library) { + type is TypeParameterType) { // TODO(shyndman): This ignored deprecation can be removed when we // increase the analyzer dependency's lower bound to 0.39.2, and // migrate to using `DartType.getDisplayString`. @@ -103,19 +99,18 @@ class LibraryScopedNameFinder { return type.displayName; } - assert(namesByElement.containsKey(typeElement)); - return namesByElement[typeElement]; + return _getNamedElementTypeName(typeElement, type); } - String _findFunctionTypeName(FunctionType type) { - final returnTypeName = _getTypeName(type.returnType); + String _getFunctionTypeName(FunctionType type) { + final returnTypeName = _getDartTypeName(type.returnType); final normalParameterTypeNames = - CommaList(type.normalParameterTypes.map(_getTypeName).toList()); + CommaList(type.normalParameterTypes.map(_getDartTypeName).toList()); final optionalParameterTypeNames = SurroundedCommaList( - '[', ']', type.optionalParameterTypes.map(_getTypeName).toList()); + '[', ']', type.optionalParameterTypes.map(_getDartTypeName).toList()); final namedParameterPairs = type.namedParameterTypes.entries - .map((entry) => '${_getTypeName(entry.value)} ${entry.key}') + .map((entry) => '${_getDartTypeName(entry.value)} ${entry.key}') .toList(); final namedParameterTypeNames = SurroundedCommaList('{', '}', namedParameterPairs); @@ -128,4 +123,19 @@ class LibraryScopedNameFinder { return '$returnTypeName Function($parameterTypeNames)'; } + + String _getNamedElementTypeName(Element typeElement, DartType type) { + // Determine the name of the type, without type arguments. + assert(namesByElement.containsKey(typeElement), + '$typeElement not found in name map'); + + // If the type is parameterized, we recursively name its type arguments + if (type is ParameterizedType && type.typeArguments.isNotEmpty) { + final typeArgNames = SurroundedCommaList( + '<', '>', type.typeArguments.map(_getDartTypeName).toList()); + return '${namesByElement[typeElement]}$typeArgNames'; + } + + return namesByElement[typeElement]; + } } diff --git a/mobx_codegen/pubspec.yaml b/mobx_codegen/pubspec.yaml index c3750f21f..322d54320 100644 --- a/mobx_codegen/pubspec.yaml +++ b/mobx_codegen/pubspec.yaml @@ -5,7 +5,7 @@ version: 0.4.0+1 homepage: https://github.com/mobxjs/mobx.dart environment: - sdk: '>=2.2.0 <3.0.0' + sdk: '>=2.2.2 <3.0.0' dependencies: analyzer: '>=0.38.5 <0.40.0' @@ -16,8 +16,6 @@ dependencies: source_gen: ^0.9.4 dev_dependencies: -# flutter: -# sdk: flutter build_runner: ^1.7.2 build_test: ^0.10.9 logging: ^0.11.3 @@ -25,6 +23,3 @@ dev_dependencies: test: ^1.9.4 test_coverage: git: https://github.com/shyndman/test-coverage - -#flutter: -# uses-material-design: true diff --git a/mobx_codegen/test/data/valid_generic_store_input.dart b/mobx_codegen/test/data/valid_generic_store_input.dart index fa926f165..b2a712502 100644 --- a/mobx_codegen/test/data/valid_generic_store_input.dart +++ b/mobx_codegen/test/data/valid_generic_store_input.dart @@ -9,4 +9,7 @@ class Item = _Item with _$Item; abstract class _Item with Store { @observable T value; + + @observable + List values; } diff --git a/mobx_codegen/test/data/valid_generic_store_output.dart b/mobx_codegen/test/data/valid_generic_store_output.dart index 4204467d7..0cb71d76b 100644 --- a/mobx_codegen/test/data/valid_generic_store_output.dart +++ b/mobx_codegen/test/data/valid_generic_store_output.dart @@ -15,4 +15,21 @@ mixin _$Item on _Item, Store { _$valueAtom.reportChanged(); }, _$valueAtom, name: '${_$valueAtom.name}_set'); } + + final _$valuesAtom = Atom(name: '_Item.values'); + + @override + List get values { + _$valuesAtom.context.enforceReadPolicy(_$valuesAtom); + _$valuesAtom.reportObserved(); + return super.values; + } + + @override + set values(List value) { + _$valuesAtom.context.conditionallyRunInAction(() { + super.values = value; + _$valuesAtom.reportChanged(); + }, _$valuesAtom, name: '${_$valuesAtom.name}_set'); + } } diff --git a/mobx_codegen/test/data/valid_import_prefixed_input.dart b/mobx_codegen/test/data/valid_import_prefixed_input.dart index 851ea0912..12825dd36 100644 --- a/mobx_codegen/test/data/valid_import_prefixed_input.dart +++ b/mobx_codegen/test/data/valid_import_prefixed_input.dart @@ -7,23 +7,42 @@ import 'package:mobx/mobx.dart'; part 'generator_sample.g.dart'; -class User = UserBase with _$User; +class User = UserBase with _$User; -abstract class UserBase with Store { +abstract class UserBase with Store { UserBase(); + @observable + List names; + + @observable + List files; + + @observable + List processes; + @observable io.File biography; + // This should output the type's constraint, prefixed @observable - User friend; + User friendWithImplicitTypeArgument; @observable - void Function(io.File, {io.File another}) callback; + User friendWithExplicitTypeArgument; + + @observable + void Function(io.File, {T another}) callback; @observable io.File Function(String, [int, io.File]) callback2; + @observable + ValueCallback localTypedefCallback; + + @observable + io.BadCertificateCallback prefixedTypedefCallback; + @computed io.File get biographyNotes => io.File('${biography.path}.notes'); @@ -41,3 +60,5 @@ abstract class UserBase with Store { yield directory; } } + +typedef ValueCallback = void Function(T); diff --git a/mobx_codegen/test/data/valid_import_prefixed_output.dart b/mobx_codegen/test/data/valid_import_prefixed_output.dart index 4394a1cd5..636ca1da1 100644 --- a/mobx_codegen/test/data/valid_import_prefixed_output.dart +++ b/mobx_codegen/test/data/valid_import_prefixed_output.dart @@ -1,4 +1,4 @@ -mixin _$User on UserBase, Store { +mixin _$User on UserBase, Store { Computed _$biographyNotesComputed; @override @@ -6,6 +6,57 @@ mixin _$User on UserBase, Store { Computed(() => super.biographyNotes)) .value; + final _$namesAtom = Atom(name: 'UserBase.names'); + + @override + List get names { + _$namesAtom.context.enforceReadPolicy(_$namesAtom); + _$namesAtom.reportObserved(); + return super.names; + } + + @override + set names(List value) { + _$namesAtom.context.conditionallyRunInAction(() { + super.names = value; + _$namesAtom.reportChanged(); + }, _$namesAtom, name: '${_$namesAtom.name}_set'); + } + + final _$filesAtom = Atom(name: 'UserBase.files'); + + @override + List get files { + _$filesAtom.context.enforceReadPolicy(_$filesAtom); + _$filesAtom.reportObserved(); + return super.files; + } + + @override + set files(List value) { + _$filesAtom.context.conditionallyRunInAction(() { + super.files = value; + _$filesAtom.reportChanged(); + }, _$filesAtom, name: '${_$filesAtom.name}_set'); + } + + final _$processesAtom = Atom(name: 'UserBase.processes'); + + @override + List get processes { + _$processesAtom.context.enforceReadPolicy(_$processesAtom); + _$processesAtom.reportObserved(); + return super.processes; + } + + @override + set processes(List value) { + _$processesAtom.context.conditionallyRunInAction(() { + super.processes = value; + _$processesAtom.reportChanged(); + }, _$processesAtom, name: '${_$processesAtom.name}_set'); + } + final _$biographyAtom = Atom(name: 'UserBase.biography'); @override @@ -23,34 +74,57 @@ mixin _$User on UserBase, Store { }, _$biographyAtom, name: '${_$biographyAtom.name}_set'); } - final _$friendAtom = Atom(name: 'UserBase.friend'); + final _$friendWithImplicitTypeArgumentAtom = + Atom(name: 'UserBase.friendWithImplicitTypeArgument'); @override - User get friend { - _$friendAtom.context.enforceReadPolicy(_$friendAtom); - _$friendAtom.reportObserved(); - return super.friend; + User get friendWithImplicitTypeArgument { + _$friendWithImplicitTypeArgumentAtom.context + .enforceReadPolicy(_$friendWithImplicitTypeArgumentAtom); + _$friendWithImplicitTypeArgumentAtom.reportObserved(); + return super.friendWithImplicitTypeArgument; } @override - set friend(User value) { - _$friendAtom.context.conditionallyRunInAction(() { - super.friend = value; - _$friendAtom.reportChanged(); - }, _$friendAtom, name: '${_$friendAtom.name}_set'); + set friendWithImplicitTypeArgument(User value) { + _$friendWithImplicitTypeArgumentAtom.context.conditionallyRunInAction(() { + super.friendWithImplicitTypeArgument = value; + _$friendWithImplicitTypeArgumentAtom.reportChanged(); + }, _$friendWithImplicitTypeArgumentAtom, + name: '${_$friendWithImplicitTypeArgumentAtom.name}_set'); + } + + final _$friendWithExplicitTypeArgumentAtom = + Atom(name: 'UserBase.friendWithExplicitTypeArgument'); + + @override + User get friendWithExplicitTypeArgument { + _$friendWithExplicitTypeArgumentAtom.context + .enforceReadPolicy(_$friendWithExplicitTypeArgumentAtom); + _$friendWithExplicitTypeArgumentAtom.reportObserved(); + return super.friendWithExplicitTypeArgument; + } + + @override + set friendWithExplicitTypeArgument(User value) { + _$friendWithExplicitTypeArgumentAtom.context.conditionallyRunInAction(() { + super.friendWithExplicitTypeArgument = value; + _$friendWithExplicitTypeArgumentAtom.reportChanged(); + }, _$friendWithExplicitTypeArgumentAtom, + name: '${_$friendWithExplicitTypeArgumentAtom.name}_set'); } final _$callbackAtom = Atom(name: 'UserBase.callback'); @override - void Function(io.File, {io.File another}) get callback { + void Function(io.File, {T another}) get callback { _$callbackAtom.context.enforceReadPolicy(_$callbackAtom); _$callbackAtom.reportObserved(); return super.callback; } @override - set callback(void Function(io.File, {io.File another}) value) { + set callback(void Function(io.File, {T another}) value) { _$callbackAtom.context.conditionallyRunInAction(() { super.callback = value; _$callbackAtom.reportChanged(); @@ -74,6 +148,46 @@ mixin _$User on UserBase, Store { }, _$callback2Atom, name: '${_$callback2Atom.name}_set'); } + final _$localTypedefCallbackAtom = + Atom(name: 'UserBase.localTypedefCallback'); + + @override + ValueCallback get localTypedefCallback { + _$localTypedefCallbackAtom.context + .enforceReadPolicy(_$localTypedefCallbackAtom); + _$localTypedefCallbackAtom.reportObserved(); + return super.localTypedefCallback; + } + + @override + set localTypedefCallback(ValueCallback value) { + _$localTypedefCallbackAtom.context.conditionallyRunInAction(() { + super.localTypedefCallback = value; + _$localTypedefCallbackAtom.reportChanged(); + }, _$localTypedefCallbackAtom, + name: '${_$localTypedefCallbackAtom.name}_set'); + } + + final _$prefixedTypedefCallbackAtom = + Atom(name: 'UserBase.prefixedTypedefCallback'); + + @override + io.BadCertificateCallback get prefixedTypedefCallback { + _$prefixedTypedefCallbackAtom.context + .enforceReadPolicy(_$prefixedTypedefCallbackAtom); + _$prefixedTypedefCallbackAtom.reportObserved(); + return super.prefixedTypedefCallback; + } + + @override + set prefixedTypedefCallback(io.BadCertificateCallback value) { + _$prefixedTypedefCallbackAtom.context.conditionallyRunInAction(() { + super.prefixedTypedefCallback = value; + _$prefixedTypedefCallbackAtom.reportChanged(); + }, _$prefixedTypedefCallbackAtom, + name: '${_$prefixedTypedefCallbackAtom.name}_set'); + } + @override ObservableFuture futureBiography() { final _$future = super.futureBiography();