From 4b1b7cc11707fd577d55207eb68afed60593c862 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 17 Aug 2018 10:52:53 -0700 Subject: [PATCH] fix(Compiler): Refuse invalid function (null) in `FactoryProvider`. Turns a (mysterious) runtime error into a compile-time one. Shouldn't impact users. Closes #1500. Closes #1581. PiperOrigin-RevId: 209173141 --- _tests/build.debug.yaml | 1 + _tests/build.yaml | 1 + .../1500_factory_provider_null_test.dart | 21 +++++++++++++++++++ angular/CHANGELOG.md | 4 ++++ angular_compiler/CHANGELOG.md | 2 ++ .../lib/src/analyzer/di/injector.dart | 10 +++++++++ .../lib/src/analyzer/di/providers.dart | 16 ++++++++++++-- 7 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 _tests/test/regression/1500_factory_provider_null_test.dart diff --git a/_tests/build.debug.yaml b/_tests/build.debug.yaml index d822f1e573..6b106401dc 100644 --- a/_tests/build.debug.yaml +++ b/_tests/build.debug.yaml @@ -16,6 +16,7 @@ targets: - test/core/di/provider_test.dart* - test/regression/906_incorrect_injectable_warning_test.dart* - test/regression/519_missing_query_selector_test.dart* + - test/regression/1500_factory_provider_null_test.dart* - test/regression/1538_deferred_template_test.dart* angular: diff --git a/_tests/build.yaml b/_tests/build.yaml index 4988ea87ce..b9bc883fcc 100644 --- a/_tests/build.yaml +++ b/_tests/build.yaml @@ -12,6 +12,7 @@ targets: - test/core/di/provider_test.dart* - test/regression/906_incorrect_injectable_warning_test.dart* - test/regression/519_missing_query_selector_test.dart* + - test/regression/1500_factory_provider_null_test.dart* - test/regression/1538_deferred_template_test.dart* angular: options: diff --git a/_tests/test/regression/1500_factory_provider_null_test.dart b/_tests/test/regression/1500_factory_provider_null_test.dart new file mode 100644 index 0000000000..b84f6cb5e2 --- /dev/null +++ b/_tests/test/regression/1500_factory_provider_null_test.dart @@ -0,0 +1,21 @@ +@TestOn('vm') +import 'package:_tests/compiler.dart'; +import 'package:test/test.dart'; + +// See https://github.com/dart-lang/angular/issues/1500. +void main() { + test('should disallow a value of "null" for FactoryProvider', () async { + await compilesExpecting(""" + import '$ngImport'; + + class Service {} + + @GenerateInjector([ + FactoryProvider(Service, null), + ]) + final InjectorFactory injectorFactory = null; // OK for compiler tests. + """, errors: [ + contains('an explicit value of `null` was passed in where a function is'), + ]); + }); +} diff --git a/angular/CHANGELOG.md b/angular/CHANGELOG.md index 9932448c91..79551261c8 100644 --- a/angular/CHANGELOG.md +++ b/angular/CHANGELOG.md @@ -63,8 +63,12 @@ `*ngFor="let item of items;"` - note the trailing `;`), throws a proper unexpected token error instead of a confusing type error during recovery. +* [#1500][]: Configuring a provider with `FactoryProvider(Foo, null)` is now + a compile-time error, instead of a misleading runtime error. + [#434]: https://github.com/dart-lang/angular/issues/434 [#880]: https://github.com/dart-lang/angular/issues/880 +[#1500]: https://github.com/dart-lang/angular/issues/1500 [#1502]: https://github.com/dart-lang/angular/issues/1502 [#1538]: https://github.com/dart-lang/angular/issues/1538 [#1539]: https://github.com/dart-lang/angular/issues/1539 diff --git a/angular_compiler/CHANGELOG.md b/angular_compiler/CHANGELOG.md index b4d839b998..41926abd73 100644 --- a/angular_compiler/CHANGELOG.md +++ b/angular_compiler/CHANGELOG.md @@ -1,5 +1,7 @@ * Catches an (invalid) `null` token of a provider and throws a better error. +* Catches an (invalid) `null` value of the function for `FactoryProvider`. + ## 0.4.0 ### New Features diff --git a/angular_compiler/lib/src/analyzer/di/injector.dart b/angular_compiler/lib/src/analyzer/di/injector.dart index 2667f1df77..eafb314265 100644 --- a/angular_compiler/lib/src/analyzer/di/injector.dart +++ b/angular_compiler/lib/src/analyzer/di/injector.dart @@ -84,6 +84,14 @@ class InjectorReader { ); } + @alwaysThrows + void _throwFactoryProvider(DartObject context) { + BuildError.throwForElement( + field, + 'Invalid provider ($context): an explicit value of `null` was passed in ' + 'where a function is expected.'); + } + /// Providers that are part of the provided list of the annotation. Iterable get providers { if (_providers == null) { @@ -96,6 +104,8 @@ class InjectorReader { _providers = moduleReader.deduplicateProviders(module.flatten()); } on NullTokenException catch (e) { _throwParseError(e.constant); + } on NullFactoryException catch (e) { + _throwFactoryProvider(e.constant); } } return _providers; diff --git a/angular_compiler/lib/src/analyzer/di/providers.dart b/angular_compiler/lib/src/analyzer/di/providers.dart index 346a01caaa..da2a119a7f 100644 --- a/angular_compiler/lib/src/analyzer/di/providers.dart +++ b/angular_compiler/lib/src/analyzer/di/providers.dart @@ -36,7 +36,6 @@ class ProviderReader { } if (isType(o)) { // Represents "Foo", which is supported short-hand for "Provider(Foo)". - // TODO(matanl): Validate that Foo has @Injectable() when flag is set. return _parseType(o); } if (!isProvider(o)) { @@ -50,7 +49,7 @@ class ProviderReader { final reader = ConstantReader(o); final value = reader.read('token'); if (value.isNull) { - throw new NullTokenException(o); + throw NullTokenException(o); } final token = _tokenReader.parseTokenObject(value.objectValue); final useClass = reader.read('useClass'); @@ -81,6 +80,11 @@ class ProviderReader { } // Base case: const Provider(Foo) with no fields set. if (token is TypeTokenElement) { + // Ensure this isn't a FactoryProvider with a null function: + // https://github.com/dart-lang/angular/issues/1500 + if (!$Provider.isExactlyType(o.type)) { + throw NullFactoryException(o); + } return _parseUseClass(token, o, reader.read('token').typeValue.element); } throw UnsupportedError('Could not parse provider: $o.'); @@ -338,3 +342,11 @@ class NullTokenException implements Exception { const NullTokenException(this.constant); } + +/// Thrown when a value of `null` is read for `FactoryProvider`. +class NullFactoryException implements Exception { + /// Constant whose `.token` property was resolved to `null`. + final DartObject constant; + + const NullFactoryException(this.constant); +}