First pass at pipe support, check return type of pipes. (#621)
* First pass at pipe support, check return+input type of pipes.
Some issues with pipe parsing offsets, and deserialization fixed.
Otherwise, we stick the pipe name & args into the AST properties, and
our already existing override of ResolverVisitor can find the synthetic
nodes for pipes and set the propagated type accordingly.
Haven't yet handled optional arguments -- I likely will run into issues where the
argument expressions need to be hit by each of the visitors. So I may be
better off doing a synthetic function call node that's treated
specially, rather than the previous cast technique which was more for
making an AST node that would result in the right analysis. That's no
longer necessary if we're hijacking analysis, and we want the children
of the pipe (when the pipe has optional arguments) to be typechecked safely which
means its probably best to make them first-class citizens.
diff --git a/CHANGELOG.md b/CHANGELOG.md
index d47eae1..76e277f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,8 @@
## Unpublished
+- Typecheck the results and input of pipe expressions and the existence of a
+ matching pipe. Optional arguments are not yet typechecked.
+
## 0.0.17+3
- Fixed an issue where a cast error from certain top-level getters would crash
diff --git a/README.md b/README.md
index beeccd3..8b35d1f 100644
--- a/README.md
+++ b/README.md
@@ -119,7 +119,7 @@
`<video directiveWithExportAs #moviePlayer="exportAsValue">` | :white_check_mark: existence of exportAs value checked within bound directives | :white_check_mark: | :white_check_mark: | :x:
`<video ref-movieplayer ...></video><button (click)="movieplayer.play()">` | :white_check_mark: |:white_check_mark: | :white_check_mark: | :x:
`<p *myUnless="myExpression">...</p>` | :white_check_mark: desugared to `<template [myUnless]="myExpression"><p>...` and checked from there | :white_check_mark: | :white_check_mark: some bugs ,mostly works | :x:
-`<p>Card No.: {{cardNumber \| myCardNumberFormatter}}</p>` | :x: Pipes are not typechecked yet | :x: | :x: | :x:
+`<p>Card No.: {{cardNumber \| myCardNumberFormatter}}</p>` | :first_quarter_moon: Pipe name, input, and return type checked, optional argument types not yet checked| :x: | :x: | :x:
`<my-component @deferred>` | :x: | :x: | :x: | :x:
Built-in directives | Validation | Auto-Complete | Navigation | Refactoring
diff --git a/angular_analyzer_plugin/lib/errors.dart b/angular_analyzer_plugin/lib/errors.dart
index af29608..5493cf4 100644
--- a/angular_analyzer_plugin/lib/errors.dart
+++ b/angular_analyzer_plugin/lib/errors.dart
@@ -62,6 +62,7 @@
AngularWarningCode.PIPE_REQUIRES_TRANSFORM_METHOD,
AngularWarningCode.PIPE_TRANSFORM_NO_NAMED_ARGS,
AngularWarningCode.PIPE_TRANSFORM_REQ_ONE_ARG,
+ AngularWarningCode.PIPE_NOT_FOUND,
AngularWarningCode.UNSAFE_BINDING,
AngularWarningCode.EVENT_REDUCTION_NOT_ALLOWED,
AngularWarningCode.FUNCTIONAL_DIRECTIVES_CANT_BE_EXPORTED,
@@ -475,6 +476,13 @@
'PIPE_TRANSFORM_REQ_ONE_ARG',
"'transform' method requires at least one argument");
+ /// An error indicating that pipe syntax was used in an angular template, but
+ /// the name of the pipe doesn't match one defined in the component
+ static const PIPE_NOT_FOUND = const AngularWarningCode(
+ 'PIPE_NOT_FOUND',
+ "Pipe by name of {0} not found. Did you reference it in your @Component"
+ " configuration?");
+
/// An error indicating that a security exception will be thrown by this input
/// binding
static const UNSAFE_BINDING = const AngularWarningCode(
diff --git a/angular_analyzer_plugin/lib/src/angular_driver.dart b/angular_analyzer_plugin/lib/src/angular_driver.dart
index b11de2b..2f3a3a6 100644
--- a/angular_analyzer_plugin/lib/src/angular_driver.dart
+++ b/angular_analyzer_plugin/lib/src/angular_driver.dart
@@ -632,6 +632,9 @@
pipes.add(new Pipe(dirSum.pipeName, dirSum.pipeNameOffset, classElem,
isPure: dirSum.isPure));
}
+
+ final pipeExtractor = new PipeExtractor(null, unit.source, null);
+ pipes.forEach(pipeExtractor.loadTransformInformation);
return pipes;
}
@@ -722,6 +725,7 @@
}
}
List<SummarizedDirectiveUseBuilder> dirUseSums;
+ List<SummarizedPipesUseBuilder> pipeUseSums;
final ngContents = <SummarizedNgContentBuilder>[];
String templateUrl;
int templateUrlOffset;
@@ -746,6 +750,12 @@
.toList(),
(constValue, _) => null);
+ pipeUseSums = directive.view.pipeReferences
+ .map((pipe) => new SummarizedPipesUseBuilder()
+ ..name = pipe.identifier
+ ..prefix = pipe.prefix)
+ .toList();
+
constDirectivesSourceRange = directive.view.directivesStrategy.resolve(
(references) => null, (constValue, sourceRange) => sourceRange);
@@ -774,6 +784,7 @@
..exports = exports
..usesArrayOfDirectiveReferencesStrategy = dirUseSums != null
..subdirectives = dirUseSums
+ ..pipesUse = pipeUseSums
..constDirectiveStrategyOffset = constDirectivesSourceRange?.offset
..constDirectiveStrategyLength = constDirectivesSourceRange?.length);
}
diff --git a/angular_analyzer_plugin/lib/src/ng_expr_parser.dart b/angular_analyzer_plugin/lib/src/ng_expr_parser.dart
index 78ada41..2202537 100644
--- a/angular_analyzer_plugin/lib/src/ng_expr_parser.dart
+++ b/angular_analyzer_plugin/lib/src/ng_expr_parser.dart
@@ -5,6 +5,7 @@
import 'package:analyzer/src/dart/ast/token.dart';
import 'package:analyzer/src/generated/parser.dart';
import 'package:analyzer/src/generated/source.dart';
+import 'package:angular_analyzer_plugin/src/tuple.dart';
class NgExprParser extends Parser {
NgExprParser(Source source, AnalysisErrorListener errorListener)
@@ -25,21 +26,23 @@
while (_currentToken.type == TokenType.BAR) {
beforePipeToken ??= _currentToken.previous;
getAndAdvance();
- parsePipeExpression();
- }
- if (beforePipeToken != null) {
- final asToken = new KeywordToken(Keyword.AS, 0);
- final dynamicIdToken =
- new StringToken(TokenType.IDENTIFIER, "dynamic", 0);
+ final pipeData = parsePipeExpression();
+ if (beforePipeToken != null) {
+ final asToken = new KeywordToken(Keyword.AS, 0);
+ final dynamicIdToken = new SyntheticStringToken(TokenType.IDENTIFIER,
+ "dynamic", _currentToken.offset - "dynamic".length);
- beforePipeToken.setNext(asToken);
- asToken.setNext(dynamicIdToken);
- dynamicIdToken.setNext(_currentToken);
+ beforePipeToken.setNext(asToken);
+ asToken.setNext(dynamicIdToken);
+ dynamicIdToken.setNext(_currentToken);
- final dynamicIdentifier = astFactory.simpleIdentifier(dynamicIdToken);
+ final dynamicIdentifier = astFactory.simpleIdentifier(dynamicIdToken);
- expression = astFactory.asExpression(
- expression, asToken, astFactory.typeName(dynamicIdentifier, null));
+ expression = astFactory.asExpression(
+ expression, asToken, astFactory.typeName(dynamicIdentifier, null))
+ ..setProperty('_ng_pipeName', pipeData.item1)
+ ..setProperty('_ng_pipeArgs', pipeData.item2);
+ }
}
return expression;
}
@@ -48,11 +51,15 @@
/// Return the resolved left-hand expression as a dynamic type.
///
/// pipeExpression ::= identifier[':' expression]*
- void parsePipeExpression() {
- parseIdentifierList();
+ Tuple2<SimpleIdentifier, List<Expression>> parsePipeExpression() {
+ final identifiers = parseIdentifierList();
+ final expressions = <Expression>[];
while (_currentToken.type == TokenType.COLON) {
getAndAdvance();
- parseExpression2();
+ expressions.add(parseExpression2());
}
+
+ return Tuple2<SimpleIdentifier, List<Expression>>(
+ identifiers.first, expressions);
}
}
diff --git a/angular_analyzer_plugin/lib/src/pipe_extraction.dart b/angular_analyzer_plugin/lib/src/pipe_extraction.dart
index 8d76ae5..81f0912 100644
--- a/angular_analyzer_plugin/lib/src/pipe_extraction.dart
+++ b/angular_analyzer_plugin/lib/src/pipe_extraction.dart
@@ -28,7 +28,7 @@
for (final annotationNode in unitMember.metadata) {
final pipe = _createPipe(unitMember, annotationNode);
if (pipe != null) {
- pipes.add(_loadTransformInformation(pipe));
+ pipes.add(loadTransformInformation(pipe));
}
}
}
@@ -36,6 +36,44 @@
return pipes;
}
+ /// Looks for a 'transform' function, and if found, finds all the
+ /// important type information needed for resolution of pipe.
+ Pipe loadTransformInformation(Pipe pipe) {
+ final classElement = pipe.classElement;
+ if (classElement == null) {
+ return pipe;
+ }
+
+ final transformMethod =
+ classElement.lookUpMethod('transform', classElement.library);
+ if (transformMethod == null) {
+ errorReporter.reportErrorForElement(
+ AngularWarningCode.PIPE_REQUIRES_TRANSFORM_METHOD, classElement);
+ return pipe;
+ }
+
+ pipe.transformReturnType = transformMethod.returnType;
+ final parameters = transformMethod.parameters;
+ if (parameters == null || parameters.isEmpty) {
+ errorReporter.reportErrorForElement(
+ AngularWarningCode.PIPE_TRANSFORM_REQ_ONE_ARG, transformMethod);
+ }
+ for (final parameter in parameters) {
+ // If named or positional
+ if (parameter.parameterKind == analyzer.ParameterKind.NAMED) {
+ errorReporter.reportErrorForElement(
+ AngularWarningCode.PIPE_TRANSFORM_NO_NAMED_ARGS, parameter);
+ continue;
+ }
+ if (parameters.first == parameter) {
+ pipe.requiredArgumentType = parameter.type;
+ } else {
+ pipe.optionalArgumentTypes.add(parameter.type);
+ }
+ }
+ return pipe;
+ }
+
/// Returns an Angular [Pipe] for the given [node].
/// Returns `null` if not an Angular @Pipe annotation.
Pipe _createPipe(ast.ClassDeclaration classDeclaration, ast.Annotation node) {
@@ -95,42 +133,4 @@
}
return null;
}
-
- /// Looks for a 'transform' function, and if found, finds all the
- /// important type information needed for resolution of pipe.
- Pipe _loadTransformInformation(Pipe pipe) {
- final classElement = pipe.classElement;
- if (classElement == null) {
- return pipe;
- }
-
- final transformMethod =
- classElement.lookUpMethod('transform', classElement.library);
- if (transformMethod == null) {
- errorReporter.reportErrorForElement(
- AngularWarningCode.PIPE_REQUIRES_TRANSFORM_METHOD, classElement);
- return pipe;
- }
-
- pipe.transformReturnType = transformMethod.returnType;
- final parameters = transformMethod.parameters;
- if (parameters == null || parameters.isEmpty) {
- errorReporter.reportErrorForElement(
- AngularWarningCode.PIPE_TRANSFORM_REQ_ONE_ARG, transformMethod);
- }
- for (final parameter in parameters) {
- // If named or positional
- if (parameter.parameterKind == analyzer.ParameterKind.NAMED) {
- errorReporter.reportErrorForElement(
- AngularWarningCode.PIPE_TRANSFORM_NO_NAMED_ARGS, parameter);
- continue;
- }
- if (parameters.first == parameter) {
- pipe.requiredArgumentType = parameter.type;
- } else {
- pipe.optionalArgumentTypes.add(parameter.type);
- }
- }
- return pipe;
- }
}
diff --git a/angular_analyzer_plugin/lib/src/resolver.dart b/angular_analyzer_plugin/lib/src/resolver.dart
index e9b3174..294f667 100644
--- a/angular_analyzer_plugin/lib/src/resolver.dart
+++ b/angular_analyzer_plugin/lib/src/resolver.dart
@@ -1,5 +1,3 @@
-library angular2.src.analysis.analyzer_plugin.src.resolver;
-
import 'dart:collection';
import 'package:analyzer/dart/ast/ast.dart' hide Directive;
@@ -10,6 +8,7 @@
import 'package:analyzer/error/error.dart';
import 'package:analyzer/error/listener.dart';
import 'package:analyzer/src/dart/element/element.dart';
+import 'package:analyzer/src/error/codes.dart';
import 'package:analyzer/src/generated/error_verifier.dart';
import 'package:analyzer/src/generated/resolver.dart';
import 'package:analyzer/src/generated/source.dart';
@@ -107,10 +106,11 @@
class AngularResolverVisitor extends _IntermediateResolverVisitor
with ReportUnacceptableNodesMixin {
final bool acceptAssignment;
+ final List<Pipe> pipes;
AngularResolverVisitor(LibraryElement library, Source source,
TypeProvider typeProvider, AnalysisErrorListener errorListener,
- {@required this.acceptAssignment})
+ {@required this.acceptAssignment, @required this.pipes})
: super(library, source, typeProvider, errorListener);
@override
@@ -118,6 +118,24 @@
// This means we generated this in a pipe, and its OK.
if (exp.asOperator.offset == 0) {
super.visitAsExpression(exp);
+ final pipeName = exp.getProperty<SimpleIdentifier>('_ng_pipeName');
+ final matchingPipes =
+ pipes.where((pipe) => pipe.pipeName == pipeName.name);
+ if (matchingPipes.length != 1) {
+ errorReporter.reportErrorForNode(
+ AngularWarningCode.PIPE_NOT_FOUND, pipeName, [pipeName]);
+ } else {
+ final matchingPipe = matchingPipes.single;
+ exp.staticType = matchingPipe.transformReturnType;
+
+ if (!typeSystem.isAssignableTo(
+ exp.expression.staticType, matchingPipe.requiredArgumentType)) {
+ errorReporter.reportErrorForNode(
+ StaticWarningCode.ARGUMENT_TYPE_NOT_ASSIGNABLE,
+ exp.expression,
+ [exp.expression.staticType, matchingPipe.requiredArgumentType]);
+ }
+ }
} else {
_reportUnacceptableNode(exp, "As expression");
}
@@ -1207,6 +1225,7 @@
static final RegExp _cssIdentifierRegexp =
new RegExp(r"^(-?[a-zA-Z_]|\\.)([a-zA-Z0-9\-_]|\\.)*$");
final Map<String, InputElement> standardHtmlAttributes;
+ final List<Pipe> pipes;
List<AbstractDirective> directives;
View view;
Template template;
@@ -1223,6 +1242,7 @@
SingleScopeResolver(
this.standardHtmlAttributes,
+ this.pipes,
this.view,
this.template,
this.templateSource,
@@ -1466,7 +1486,7 @@
}
final resolver = new AngularResolverVisitor(
library, templateSource, typeProvider, errorListener,
- acceptAssignment: acceptAssignment);
+ acceptAssignment: acceptAssignment, pipes: pipes);
// fill the name scope
final classScope = new ClassScope(resolver.nameScope, classElement);
final localScope = new EnclosedScope(classScope);
@@ -1787,6 +1807,7 @@
// Resolve the scopes
..accept(new SingleScopeResolver(
standardHtmlAttributes,
+ view.pipes,
view,
template,
templateSource,
diff --git a/angular_analyzer_plugin/test/resolver_test.dart b/angular_analyzer_plugin/test/resolver_test.dart
index 36194e9..6f19d92 100644
--- a/angular_analyzer_plugin/test/resolver_test.dart
+++ b/angular_analyzer_plugin/test/resolver_test.dart
@@ -1541,10 +1541,14 @@
@Input() int value;
}
@Component(selector: 'test-panel', templateUrl: 'test_panel.html',
- directives: const [NamePanel])
+ directives: const [NamePanel], pipes: [Pipe1])
class TestPanel {
int value;
}
+@Pipe('pipe1')
+class Pipe1 extends PipeTransform {
+ int transform(int x) => "";
+}
''');
_addHtmlSource(r"""
<name-panel [value]='value | pipe1'></name-panel>
@@ -1556,13 +1560,16 @@
// ignore: non_constant_identifier_names
Future test_expression_pipe_in_moustache() async {
_addDartSource(r'''
-@Component(selector: 'test-panel', templateUrl: 'test_panel.html')
+@Component(selector: 'test-panel', templateUrl: 'test_panel.html', pipes: [Pipe1])
class TestPanel {
- String name = "TestPanel";
+}
+@Pipe('pipe1')
+class Pipe1 extends PipeTransform {
+ String transform(int x) => "";
}
''');
final code = r"""
-<p>{{((1 | pipe1:(2+2):(5 | pipe2:1:2)) + (2 | pipe3:4:2))}}</p>
+<p>{{((1 | pipe1:(2+2):(5 | pipe1:1:2)) + (2 | pipe1:4:2))}}</p>
""";
_addHtmlSource(code);
await _resolveSingleTemplate(dartSource);
@@ -1570,15 +1577,160 @@
}
// ignore: non_constant_identifier_names
- Future test_expression_pipe_in_moustache_with_error() async {
+ @failingTest
+ Future test_expression_pipe_in_moustache_extraArg() async {
_addDartSource(r'''
@Component(selector: 'test-panel', templateUrl: 'test_panel.html')
class TestPanel {
- String name = "TestPanel";
+}
+@Pipe('stringPipe')
+class StringPipe extends PipeTransform {
+ String transform(String x) => "";
}
''');
final code = r"""
-<p>{{((1 | pipe1:(2+2):(5 | pipe2:1:2)) + (error1 | pipe3:4:2))}}</p>
+{{"" | stringPipe: "extra"}}
+""";
+ _addHtmlSource(code);
+ await _resolveSingleTemplate(dartSource);
+ assertErrorInCodeAtPosition(
+ StaticWarningCode.EXTRA_POSITIONAL_ARGUMENTS, code, '"extra"');
+ }
+
+ // ignore: non_constant_identifier_names
+ @failingTest
+ Future test_expression_pipe_in_moustache_extraExtraArg() async {
+ _addDartSource(r'''
+@Component(selector: 'test-panel', templateUrl: 'test_panel.html')
+class TestPanel {
+}
+@Pipe('stringPipe')
+class StringPipe extends PipeTransform {
+ String transform(String x, [String y]) => "";
+}
+''');
+ final code = r"""
+{{"" | stringPipe: "": "extra"}}
+""";
+ _addHtmlSource(code);
+ await _resolveSingleTemplate(dartSource);
+ assertErrorInCodeAtPosition(
+ StaticWarningCode.EXTRA_POSITIONAL_ARGUMENTS, code, '"extra"');
+ }
+
+ // ignore: non_constant_identifier_names
+ Future test_expression_pipe_in_moustache_noSuchPipe() async {
+ _addDartSource(r'''
+@Component(selector: 'test-panel', templateUrl: 'test_panel.html')
+class TestPanel {
+}
+''');
+ final code = r"""
+{{1 | noSuchPipe}}
+""";
+ _addHtmlSource(code);
+ await _resolveSingleTemplate(dartSource);
+ assertErrorInCodeAtPosition(
+ AngularWarningCode.PIPE_NOT_FOUND, code, 'noSuchPipe');
+ }
+
+ // ignore: non_constant_identifier_names
+ Future test_expression_pipe_in_moustache_typeErrorInput() async {
+ _addDartSource(r'''
+@Component(selector: 'test-panel', templateUrl: 'test_panel.html', pipes: [StringPipe])
+class TestPanel {
+}
+@Pipe('stringPipe')
+class StringPipe extends PipeTransform {
+ String transform(String x) => "";
+}
+''');
+ final code = r"""
+{{1 | stringPipe}}
+""";
+ _addHtmlSource(code);
+ await _resolveSingleTemplate(dartSource);
+ assertErrorInCodeAtPosition(
+ StaticWarningCode.ARGUMENT_TYPE_NOT_ASSIGNABLE, code, '1');
+ }
+
+ // ignore: non_constant_identifier_names
+ @failingTest
+ Future test_expression_pipe_in_moustache_typeErrorOptionalArg() async {
+ _addDartSource(r'''
+@Component(selector: 'test-panel', templateUrl: 'test_panel.html', pipes: [StringPipe])
+class TestPanel {
+}
+@Pipe('stringPipe')
+class StringPipe extends PipeTransform {
+ String transform(String x, [String y]) => "";
+}
+''');
+ final code = r"""
+{{"" | stringPipe: 1}}
+""";
+ _addHtmlSource(code);
+ await _resolveSingleTemplate(dartSource);
+ assertErrorInCodeAtPosition(
+ StaticWarningCode.ARGUMENT_TYPE_NOT_ASSIGNABLE, code, '1');
+ }
+
+ // ignore: non_constant_identifier_names
+ Future test_expression_pipe_in_moustache_typeErrorResult() async {
+ _addDartSource(r'''
+@Component(selector: 'test-panel', templateUrl: 'test_panel.html', pipes: [StringPipe])
+class TestPanel {
+ String takeInt(int x) => "";
+}
+@Pipe('stringPipe')
+class StringPipe extends PipeTransform {
+ String transform(String x) => "";
+}
+''');
+ final code = r"""
+{{takeInt("" | stringPipe)}}
+""";
+ _addHtmlSource(code);
+ await _resolveSingleTemplate(dartSource);
+ assertErrorInCodeAtPosition(StaticWarningCode.ARGUMENT_TYPE_NOT_ASSIGNABLE,
+ code, '"" | stringPipe');
+ }
+
+ // ignore: non_constant_identifier_names
+ Future test_expression_pipe_in_moustache_with_error() async {
+ _addDartSource(r'''
+@Component(selector: 'test-panel', templateUrl: 'test_panel.html', pipes: [Pipe1])
+class TestPanel {
+ String name = "TestPanel";
+}
+@Pipe('pipe1')
+class Pipe1 extends PipeTransform {
+ int transform(int x) => "";
+}
+''');
+ final code = r"""
+<p>{{((1 | pipe1:(2+2):(5 | pipe1:1:2)) + (error1 | pipe1:4:2))}}</p>
+""";
+ _addHtmlSource(code);
+ await _resolveSingleTemplate(dartSource);
+ assertErrorInCodeAtPosition(
+ StaticWarningCode.UNDEFINED_IDENTIFIER, code, "error1");
+ }
+
+ // ignore: non_constant_identifier_names
+ @failingTest
+ Future test_expression_pipe_in_moustache_with_error_inArg() async {
+ _addDartSource(r'''
+@Component(selector: 'test-panel', templateUrl: 'test_panel.html', pipes: [Pipe1])
+class TestPanel {
+}
+@Pipe('pipe1')
+class Pipe1 extends PipeTransform {
+ String transform(String x) => "";
+}
+''');
+ final code = r"""
+<p>{{1 | pipe1:error1}}
""";
_addHtmlSource(code);
await _resolveSingleTemplate(dartSource);
@@ -1590,10 +1742,14 @@
Future test_expression_pipe_in_ngFor() async {
_addDartSource(r'''
@Component(selector: 'test-panel', templateUrl: 'test_panel.html',
- directives: const [NgFor])
+ directives: const [NgFor], pipes: [Pipe1])
class TestPanel {
List<String> operators = [];
}
+@Pipe('pipe1')
+class Pipe1 extends PipeTransform {
+ List<String> transform(List<String> x) => [];
+}
''');
_addHtmlSource(r"""
<li *ngFor='let operator of (operators | pipe1)'>
