Remove facade classes, find unexported terms in its own visitor.
Reused `AngularSubsetVisitor`, which seems like a perfect place for
this. A bit unnerving with the number of edge cases I have to account
for in this solution, but I think I covered everything.
Added a somewhat ominous CHANGELOG comment for that reason.
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 658e73d..05dbb5e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,6 @@
## Unpublished
+- Refactored how `exports` are handled in a fairly major way.
- Typecheck the results and input of pipe expressions and the existence of a
matching pipe. Optional arguments are not yet typechecked.
- Add typechecking support for `[attr.foo.if]`, and ensure that a corresponding
diff --git a/angular_analyzer_plugin/lib/errors.dart b/angular_analyzer_plugin/lib/errors.dart
index 50e964e..1626f97 100644
--- a/angular_analyzer_plugin/lib/errors.dart
+++ b/angular_analyzer_plugin/lib/errors.dart
@@ -56,6 +56,7 @@
AngularWarningCode.MATCHED_LET_BINDING_HAS_WRONG_TYPE,
AngularWarningCode.EXPORTS_MUST_BE_PLAIN_IDENTIFIERS,
AngularWarningCode.DUPLICATE_EXPORT,
+ AngularWarningCode.IDENTIFIER_NOT_EXPORTED,
AngularWarningCode.COMPONENTS_CANT_EXPORT_THEMSELVES,
AngularWarningCode.PIPE_SINGLE_NAME_REQUIRED,
AngularWarningCode.TYPE_IS_NOT_A_PIPE,
@@ -450,6 +451,14 @@
static const DUPLICATE_EXPORT = const AngularWarningCode(
'DUPLICATE_EXPORT', 'Duplicate export of identifier {0}');
+ /// An error code indicating that an identifier was used in a template, but
+ /// not exported in the component.
+ static const IDENTIFIER_NOT_EXPORTED = const AngularWarningCode(
+ 'IDENTIFIER_NOT_EXPORTED',
+ 'Identifier {0} was not exported by the component and therefore cannot be'
+ ' used in its template. Add it to the exports property on the component'
+ ' definition to use it here.');
+
/// An error code indicating component Foo exports Foo, which is unnecessary
static const COMPONENTS_CANT_EXPORT_THEMSELVES = const AngularWarningCode(
'COMPONENTS_CANT_EXPORT_THEMSELVES',
diff --git a/angular_analyzer_plugin/lib/src/facade/exports_compilation_unit_element.dart b/angular_analyzer_plugin/lib/src/facade/exports_compilation_unit_element.dart
deleted file mode 100644
index 752e6ff..0000000
--- a/angular_analyzer_plugin/lib/src/facade/exports_compilation_unit_element.dart
+++ /dev/null
@@ -1,72 +0,0 @@
-import 'package:analyzer/dart/element/element.dart';
-import 'package:analyzer/src/dart/element/wrapped.dart';
-import 'package:angular_analyzer_plugin/src/model.dart';
-
-/// A facade for a [CompilationUnitElement] which consists only of a component's
-/// exports, and the component itself.
-class ExportsLimitedCompilationUnitFacade
- extends WrappedCompilationUnitElement {
- final Component _component;
- LibraryElement libraryFacade;
-
- ExportsLimitedCompilationUnitFacade(
- CompilationUnitElement wrappedUnit, this._component,
- {this.libraryFacade})
- : super(wrappedUnit);
-
- @override
- List<PropertyAccessorElement> get accessors =>
- new List<PropertyAccessorElement>.from(_component.exports
- .where(_fromThisUnit)
- .map((export) => export.element)
- .where((element) => element is PropertyAccessorElement));
-
- @override
- LibraryElement get enclosingElement => libraryFacade;
-
- @override
- List<ClassElement> get enums => new List<ClassElement>.from(_component.exports
- .where(_fromThisUnit)
- .map((export) => export.element)
- .where((element) => element is ClassElement && element.isEnum));
-
- @override
- List<FunctionElement> get functions =>
- new List<FunctionElement>.from(_component.exports
- .where(_fromThisUnit)
- .map((export) => export.element)
- .where((element) => element is FunctionElement));
-
- @override
- List<FunctionTypeAliasElement> get functionTypeAliases => [];
-
- @override
- bool get hasJS => false;
-
- @override
- bool get isJS => false;
-
- @override
- LibraryElement get library => libraryFacade;
-
- @override
- List<TopLevelVariableElement> get topLevelVariables => [];
-
- @override
- List<ClassElement> get types => new List<ClassElement>.from(_component.exports
- .where(_fromThisUnit)
- .map((export) => export.element)
- .where((element) => element is ClassElement))
- ..add(_component.classElement);
-
- @override
- ClassElement getEnum(String name) =>
- enums.firstWhere((_enum) => _enum.name == name, orElse: () => null);
-
- @override
- ClassElement getType(String className) =>
- types.firstWhere((type) => type.name == name, orElse: () => null);
-
- // CompilationUnitFacade's are not used for imports, which have prefixes
- bool _fromThisUnit(ExportedIdentifier export) => export.prefix == '';
-}
diff --git a/angular_analyzer_plugin/lib/src/facade/exports_import_element.dart b/angular_analyzer_plugin/lib/src/facade/exports_import_element.dart
deleted file mode 100644
index b75876f..0000000
--- a/angular_analyzer_plugin/lib/src/facade/exports_import_element.dart
+++ /dev/null
@@ -1,33 +0,0 @@
-import 'package:analyzer/dart/element/element.dart';
-import 'package:analyzer/src/dart/element/wrapped.dart';
-import 'package:angular_analyzer_plugin/src/facade/exports_library_element.dart';
-import 'package:angular_analyzer_plugin/src/model.dart';
-
-/// A facade for a [ImportElement] which consists only of a component's
-/// exports
-class ExportsImportElementFacade extends WrappedImportElement {
- final Component _component;
- LibraryElement libraryFacade;
-
- ExportsImportElementFacade(ImportElement wrappedImport, this._component,
- {this.libraryFacade})
- : super(wrappedImport);
-
- @override
- LibraryElement get enclosingElement => libraryFacade;
-
- @override
- bool get hasJS => false;
-
- @override
- LibraryElement get importedLibrary => wrappedImport.importedLibrary == null
- ? null
- : new ExportsLibraryFacade(wrappedImport.importedLibrary, _component,
- prefix: prefix?.name);
-
- @override
- bool get isJS => false;
-
- @override
- LibraryElement get library => libraryFacade;
-}
diff --git a/angular_analyzer_plugin/lib/src/facade/exports_library_element.dart b/angular_analyzer_plugin/lib/src/facade/exports_library_element.dart
deleted file mode 100644
index b682329..0000000
--- a/angular_analyzer_plugin/lib/src/facade/exports_library_element.dart
+++ /dev/null
@@ -1,108 +0,0 @@
-import 'package:analyzer/dart/element/element.dart';
-import 'package:analyzer/src/dart/element/wrapped.dart';
-import 'package:analyzer/src/generated/resolver.dart';
-import 'package:angular_analyzer_plugin/src/facade/exports_compilation_unit_element.dart';
-import 'package:angular_analyzer_plugin/src/facade/exports_import_element.dart';
-import 'package:angular_analyzer_plugin/src/model.dart';
-
-/// A facade for a [Library] which consists only of a components' exports, and
-/// the component itself.
-class ExportsLibraryFacade extends WrappedLibraryElement {
- final ExportsLimitedCompilationUnitFacade _definingUnit;
- final Component _owningComponent;
-
- factory ExportsLibraryFacade(
- LibraryElement wrappedLib, Component owningComponent,
- {String prefix}) {
- if (prefix != null) {
- return new _PrefixedExportsLibraryFacade(
- wrappedLib, owningComponent, prefix);
- }
- return new ExportsLibraryFacade._(wrappedLib, owningComponent);
- }
-
- ExportsLibraryFacade._(LibraryElement wrappedLib, this._owningComponent)
- : _definingUnit = new ExportsLimitedCompilationUnitFacade(
- wrappedLib.definingCompilationUnit, _owningComponent),
- super(wrappedLib) {
- _definingUnit.libraryFacade = this;
- }
-
- @override
- CompilationUnitElement get definingCompilationUnit => _definingUnit;
-
- @override
- bool get hasJS => false;
-
- @override
- List<ImportElement> get imports => wrappedLib.imports
- .map((import) => new ExportsImportElementFacade(import, _owningComponent,
- libraryFacade: this))
- .toList();
-
- @override
- bool get isDartAsync => false;
-
- @override
- bool get isDartCore => false;
-
- @override
- bool get isInSdk => false;
-
- @override
- bool get isJS => false;
-
- @override
- LibraryElement get library => this;
-
- @override
- List<LibraryElement> get libraryCycle => wrappedLib.libraryCycle
- .map((lib) => new ExportsLibraryFacade(lib, _owningComponent))
- .toList();
-
- @override
- List<CompilationUnitElement> get parts => wrappedLib.parts
- .map((part) => new ExportsLimitedCompilationUnitFacade(
- part, _owningComponent,
- libraryFacade: this))
- .toList();
-
- @override
- List<PrefixElement> get prefixes => wrappedLib.prefixes
- .where((prefix) => _owningComponent.exports
- .any((export) => export.prefix == prefix.name))
- .toList();
-
- @override
- List<CompilationUnitElement> get units => wrappedLib.units
- .map((unit) => new ExportsLimitedCompilationUnitFacade(
- unit, _owningComponent,
- libraryFacade: this))
- .toList();
-
- @override
- List<ImportElement> getImportsWithPrefix(PrefixElement prefix) => wrappedLib
- .getImportsWithPrefix(prefix)
- .map((lib) => new ExportsImportElementFacade(lib, _owningComponent,
- libraryFacade: this))
- .toList();
-
- @override
- ClassElement getType(String className) => wrappedLib.getType(className);
-}
-
-class _PrefixedExportsLibraryFacade extends ExportsLibraryFacade {
- final String _prefix;
- _PrefixedExportsLibraryFacade(
- LibraryElement wrappedLib, Component owningComponent, this._prefix)
- : super._(wrappedLib, owningComponent);
-
- @override
- Namespace get exportNamespace {
- final map = <String, Element>{};
- _owningComponent.exports
- .where((export) => export.prefix == _prefix)
- .forEach((export) => map[export.identifier] = export.element);
- return new Namespace(map);
- }
-}
diff --git a/angular_analyzer_plugin/lib/src/resolver.dart b/angular_analyzer_plugin/lib/src/resolver.dart
index 3d05268..86df668 100644
--- a/angular_analyzer_plugin/lib/src/resolver.dart
+++ b/angular_analyzer_plugin/lib/src/resolver.dart
@@ -15,7 +15,6 @@
import 'package:analyzer/src/generated/source.dart';
import 'package:angular_analyzer_plugin/ast.dart';
import 'package:angular_analyzer_plugin/errors.dart';
-import 'package:angular_analyzer_plugin/src/facade/exports_library_element.dart';
import 'package:angular_analyzer_plugin/src/model.dart';
import 'package:angular_analyzer_plugin/src/options.dart';
import 'package:angular_analyzer_plugin/src/selector.dart';
@@ -199,15 +198,19 @@
}
}
-/// Find nodes which are not supported in angular, such as compound assignment
-/// and function expressions etc.
+/// Find nodes which are not supported in angular (such as compound assignment
+/// and function expressions etc.), as well as terms used in the template that
+/// weren't exported by the component.
class AngularSubsetVisitor extends RecursiveAstVisitor<Object> {
final bool acceptAssignment;
+ final Component owningComponent;
final ErrorReporter errorReporter;
AngularSubsetVisitor(
- {@required this.errorReporter, @required this.acceptAssignment});
+ {@required this.errorReporter,
+ @required this.owningComponent,
+ @required this.acceptAssignment});
@override
void visitAsExpression(AsExpression exp) {
@@ -217,7 +220,9 @@
_reportDisallowedExpression(exp, "As expression", visitChildren: false);
}
- super.visitAsExpression(exp);
+ // Don't visit the TypeName or it may suggest exporting it, which is not
+ // possible.
+ exp.expression.accept(this);
}
@override
@@ -251,18 +256,112 @@
void visitFunctionExpression(FunctionExpression exp) =>
_reportDisallowedExpression(exp, "Anonymous functions");
- @override
- void visitInstanceCreationExpression(InstanceCreationExpression exp) =>
- _reportDisallowedExpression(exp, "Usage of new");
+ /// Only allow access to:
+ /// * current class members
+ /// * inherited class members
+ /// * methods
+ /// * angular references (ie `<h1 #ref id="foo"></h1> {{h1.id}}`)
+ /// * exported members
+ ///
+ /// Flag the rest and give the hint that they should be exported.
+ void visitIdentifier(Identifier id) {
+ final element = id.staticElement;
+ if (id is PrefixedIdentifier && id.prefix.staticElement is! PrefixElement) {
+ // Static methods, enums, etc. Check the LHS.
+ visitIdentifier(id.prefix);
+ return;
+ }
+ if (id.parent is PropertyAccess &&
+ identical(id, (id.parent as PropertyAccess).propertyName)) {
+ // Accessors are always allowed.
+ return;
+ }
+ if (element is PrefixElement) {
+ // Prefixes can't be exported, and analyzer reports a warning for dangling
+ // prefixes.
+ return;
+ }
+ if (element is MethodElement) {
+ // All methods are OK, as in `x.y()`. It's only `x` that may be hidden.
+ return;
+ }
+ if (element is ClassElement && element == owningComponent.classElement) {
+ // Static method calls on the current class are allowed
+ return;
+ }
+ if (element is DynamicElementImpl) {
+ // Usually indicates a resolution error, so don't double report it.
+ return;
+ }
+ if (element == null) {
+ // Also usually indicates an error, don't double report.
+ return;
+ }
+ if (element is LocalVariableElement) {
+ // `$event` variables, `ngFor` variables, these are OK.
+ return;
+ }
+ if (element is ParameterElement) {
+ // Named parameters: not allowed, but flagged in [visitNamedExpression].
+ return;
+ }
+ if (element is AngularElement) {
+ // Variables local to the template
+ return;
+ }
+ if ((element is PropertyInducingElement ||
+ element is PropertyAccessorElement) &&
+ (owningComponent.classElement.lookUpGetter(id.name, null) != null ||
+ owningComponent.classElement.lookUpSetter(id.name, null) != null)) {
+ // Part of the component interface.
+ return;
+ }
+
+ if (id is PrefixedIdentifier) {
+ if (owningComponent.exports.any((export) =>
+ export.prefix == id.prefix.name && id.name == export.identifier)) {
+ // Correct reference to exported prefix identifier
+ return;
+ }
+ } else {
+ if (owningComponent.exports.any(
+ (export) => export.prefix == null && id.name == export.identifier)) {
+ // Correct reference to exported simple identifier
+ return;
+ }
+ }
+
+ errorReporter.reportErrorForNode(
+ AngularWarningCode.IDENTIFIER_NOT_EXPORTED, id, [id]);
+ }
@override
- void visitIsExpression(IsExpression exp) =>
- _reportDisallowedExpression(exp, "Is expression");
+ void visitInstanceCreationExpression(InstanceCreationExpression exp) {
+ _reportDisallowedExpression(exp, "Usage of new", visitChildren: false);
+ // Don't visit the TypeName or it may suggest exporting it, which is not
+ // possible.
+
+ exp.argumentList.accept(this);
+ }
+
+ @override
+ void visitIsExpression(IsExpression exp) {
+ _reportDisallowedExpression(exp, "Is expression", visitChildren: false);
+ // Don't visit the TypeName or it may suggest exporting it, which is not
+ // possible.
+
+ exp.expression.accept(this);
+ }
@override
void visitListLiteral(ListLiteral list) {
if (list.typeArguments != null) {
- _reportDisallowedExpression(list, "Typed list literals");
+ _reportDisallowedExpression(list, "Typed list literals",
+ visitChildren: false);
+ // Don't visit the TypeName or it may suggest exporting it, which is not
+ // possible.e.
+
+ list.elements.accept(this);
} else {
super.visitListLiteral(list);
}
@@ -271,7 +370,12 @@
@override
void visitMapLiteral(MapLiteral map) {
if (map.typeArguments != null) {
- _reportDisallowedExpression(map, "Typed map literals");
+ _reportDisallowedExpression(map, "Typed map literals",
+ visitChildren: false);
+ // Don't visit the TypeName or it may suggest exporting it, which is not
+ // possible.e.
+
+ map.entries.accept(this);
} else {
super.visitMapLiteral(map);
}
@@ -287,6 +391,9 @@
}
@override
+ void visitPrefixedIdentifier(PrefixedIdentifier id) => visitIdentifier(id);
+
+ @override
void visitPrefixExpression(PrefixExpression exp) {
if (exp.operator.type != TokenType.MINUS) {
_reportDisallowedExpression(exp, exp.operator.lexeme);
@@ -294,6 +401,9 @@
}
@override
+ void visitSimpleIdentifier(SimpleIdentifier id) => visitIdentifier(id);
+
+ @override
void visitSymbolLiteral(SymbolLiteral exp) =>
_reportDisallowedExpression(exp, "Symbol literal");
@@ -1538,8 +1648,7 @@
/// Resolve the given [AstNode] ([expression] or [statement]) and report errors.
void _resolveDartAstNode(AstNode astNode, bool acceptAssignment) {
final classElement = view.classElement;
- final library =
- new ExportsLibraryFacade(classElement.library, view.component);
+ final library = classElement.library;
{
final visitor = new TypeResolverVisitor(
library, view.source, typeProvider, errorListener);
@@ -1568,7 +1677,9 @@
astNode.accept(verifier);
// Check for concepts illegal to templates (for instance function literals).
final angularSubsetChecker = new AngularSubsetVisitor(
- errorReporter: errorReporter, acceptAssignment: acceptAssignment);
+ errorReporter: errorReporter,
+ acceptAssignment: acceptAssignment,
+ owningComponent: view.component);
astNode.accept(angularSubsetChecker);
}
diff --git a/angular_analyzer_plugin/test/resolver_test.dart b/angular_analyzer_plugin/test/resolver_test.dart
index 8122080..c6fa8ba 100644
--- a/angular_analyzer_plugin/test/resolver_test.dart
+++ b/angular_analyzer_plugin/test/resolver_test.dart
@@ -3569,9 +3569,18 @@
class TestPanel {
static void componentStatic() {
}
+ local();
+ int get getter => null;
+ set setter(int x) => null;
+ int field = null;
}
''');
final code = r'''
+methods/getters/setters on current class ok:
+{{local()}}
+{{getter}}
+{{field}}
+<div (click)="setter = null"></div>
static on current class ok:
{{TestPanel.componentStatic()}}
exports ok:
@@ -3586,7 +3595,7 @@
''';
_addHtmlSource(code);
await _resolveSingleTemplate(dartSource);
- expect(ranges, hasLength(18));
+ expect(ranges, hasLength(23));
_assertElement('TestPanel').dart.at('TestPanel {');
_assertElement('componentStatic').dart.method.at('componentStatic() {');
_assertElement('myAccessor').dart.getter.at('myAccessor = 1');
@@ -3726,20 +3735,19 @@
''';
_addHtmlSource(code);
await _resolveSingleTemplate(dartSource);
- expect(ranges, hasLength(0));
errorListener.assertErrorsWithCodes([
- StaticWarningCode.UNDEFINED_IDENTIFIER,
- StaticWarningCode.UNDEFINED_IDENTIFIER,
- StaticWarningCode.UNDEFINED_IDENTIFIER,
- StaticTypeWarningCode.UNDEFINED_METHOD,
- StaticWarningCode.UNDEFINED_IDENTIFIER,
- StaticWarningCode.UNDEFINED_IDENTIFIER,
- StaticWarningCode.UNDEFINED_IDENTIFIER,
- StaticWarningCode.UNDEFINED_IDENTIFIER,
- StaticWarningCode.UNDEFINED_IDENTIFIER,
- StaticWarningCode.UNDEFINED_IDENTIFIER,
- StaticWarningCode.UNDEFINED_IDENTIFIER,
- StaticWarningCode.UNDEFINED_IDENTIFIER,
+ AngularWarningCode.IDENTIFIER_NOT_EXPORTED,
+ AngularWarningCode.IDENTIFIER_NOT_EXPORTED,
+ AngularWarningCode.IDENTIFIER_NOT_EXPORTED,
+ AngularWarningCode.IDENTIFIER_NOT_EXPORTED,
+ AngularWarningCode.IDENTIFIER_NOT_EXPORTED,
+ AngularWarningCode.IDENTIFIER_NOT_EXPORTED,
+ AngularWarningCode.IDENTIFIER_NOT_EXPORTED,
+ AngularWarningCode.IDENTIFIER_NOT_EXPORTED,
+ AngularWarningCode.IDENTIFIER_NOT_EXPORTED,
+ AngularWarningCode.IDENTIFIER_NOT_EXPORTED,
+ AngularWarningCode.IDENTIFIER_NOT_EXPORTED,
+ AngularWarningCode.IDENTIFIER_NOT_EXPORTED,
]);
}
