Skip to content

Commit

Permalink
fix(language-service): remove asserts for non-null expressions (angul…
Browse files Browse the repository at this point in the history 
…ar#16422)

Reworked some of the code so asserts are no longer necessary.
Added additional and potentially redundant checks
Added checks where the null checker found real problems.

PR Close angular#16422
  • Loading branch information
@chuckjaz @juleskremer
chuckjaz authored and juleskremer committed Aug 24, 2017
1 parent 2a1e12c commit 9138d4b
Show file tree
Hide file tree
Showing 12 changed files with 241 additions and 195 deletions.
4 changes: 3 additions & 1 deletion packages/language-service/src/ast_path.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ export class AstPath<T> {
get head(): T|undefined { return this.path[0]; }
get tail(): T|undefined { return this.path[this.path.length - 1]; }

parentOf(node: T): T|undefined { return this.path[this.path.indexOf(node) - 1]; }
parentOf(node: T|undefined): T|undefined {
return node && this.path[this.path.indexOf(node) - 1];
}
childOf(node: T): T|undefined { return this.path[this.path.indexOf(node) + 1]; }

first<N extends T>(ctor: {new (...args: any[]): N}): N|undefined {
Expand Down
155 changes: 81 additions & 74 deletions packages/language-service/src/completions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,71 +33,73 @@ export function getTemplateCompletions(templateInfo: TemplateInfo): Completions|
let result: Completions|undefined = undefined;
let {htmlAst, templateAst, template} = templateInfo;
// The templateNode starts at the delimiter character so we add 1 to skip it.
let templatePosition = templateInfo.position ! - template.span.start;
let path = new HtmlAstPath(htmlAst, templatePosition);
let mostSpecific = path.tail;
if (path.empty) {
result = elementCompletions(templateInfo, path);
} else {
let astPosition = templatePosition - mostSpecific !.sourceSpan !.start.offset;
mostSpecific !.visit(
{
visitElement(ast) {
let startTagSpan = spanOf(ast.sourceSpan);
let tagLen = ast.name.length;
if (templatePosition <=
startTagSpan !.start + tagLen + 1 /* 1 for the opening angle bracked */) {
// If we are in the tag then return the element completions.
result = elementCompletions(templateInfo, path);
} else if (templatePosition < startTagSpan !.end) {
// We are in the attribute section of the element (but not in an attribute).
// Return the attribute completions.
result = attributeCompletions(templateInfo, path);
}
},
visitAttribute(ast) {
if (!ast.valueSpan || !inSpan(templatePosition, spanOf(ast.valueSpan))) {
// We are in the name of an attribute. Show attribute completions.
result = attributeCompletions(templateInfo, path);
} else if (ast.valueSpan && inSpan(templatePosition, spanOf(ast.valueSpan))) {
result = attributeValueCompletions(templateInfo, templatePosition, ast);
}
},
visitText(ast) {
// Check if we are in a entity.
result = entityCompletions(getSourceText(template, spanOf(ast) !), astPosition);
if (result) return result;
result = interpolationCompletions(templateInfo, templatePosition);
if (result) return result;
let element = path.first(Element);
if (element) {
let definition = getHtmlTagDefinition(element.name);
if (definition.contentType === TagContentType.PARSABLE_DATA) {
if (templateInfo.position != null) {
let templatePosition = templateInfo.position - template.span.start;
let path = new HtmlAstPath(htmlAst, templatePosition);
let mostSpecific = path.tail;
if (path.empty || !mostSpecific) {
result = elementCompletions(templateInfo, path);
} else {
let astPosition = templatePosition - mostSpecific.sourceSpan.start.offset;
mostSpecific.visit(
{
visitElement(ast) {
let startTagSpan = spanOf(ast.sourceSpan);
let tagLen = ast.name.length;
if (templatePosition <=
startTagSpan.start + tagLen + 1 /* 1 for the opening angle bracked */) {
// If we are in the tag then return the element completions.
result = elementCompletions(templateInfo, path);
} else if (templatePosition < startTagSpan.end) {
// We are in the attribute section of the element (but not in an attribute).
// Return the attribute completions.
result = attributeCompletions(templateInfo, path);
}
},
visitAttribute(ast) {
if (!ast.valueSpan || !inSpan(templatePosition, spanOf(ast.valueSpan))) {
// We are in the name of an attribute. Show attribute completions.
result = attributeCompletions(templateInfo, path);
} else if (ast.valueSpan && inSpan(templatePosition, spanOf(ast.valueSpan))) {
result = attributeValueCompletions(templateInfo, templatePosition, ast);
}
},
visitText(ast) {
// Check if we are in a entity.
result = entityCompletions(getSourceText(template, spanOf(ast)), astPosition);
if (result) return result;
result = interpolationCompletions(templateInfo, templatePosition);
if (result) return result;
let element = path.first(Element);
if (element) {
let definition = getHtmlTagDefinition(element.name);
if (definition.contentType === TagContentType.PARSABLE_DATA) {
result = voidElementAttributeCompletions(templateInfo, path);
if (!result) {
// If the element can hold content Show element completions.
result = elementCompletions(templateInfo, path);
}
}
} else {
// If no element container, implies parsable data so show elements.
result = voidElementAttributeCompletions(templateInfo, path);
if (!result) {
// If the element can hold content Show element completions.
result = elementCompletions(templateInfo, path);
}
}
} else {
// If no element container, implies parsable data so show elements.
result = voidElementAttributeCompletions(templateInfo, path);
if (!result) {
result = elementCompletions(templateInfo, path);
}
}
},
visitComment(ast) {},
visitExpansion(ast) {},
visitExpansionCase(ast) {}
},
visitComment(ast) {},
visitExpansion(ast) {},
visitExpansionCase(ast) {}
},
null);
null);
}
}
return result;
}

function attributeCompletions(info: TemplateInfo, path: HtmlAstPath): Completions|undefined {
let item = path.tail instanceof Element ? path.tail : path.parentOf(path.tail !);
let item = path.tail instanceof Element ? path.tail : path.parentOf(path.tail);
if (item instanceof Element) {
return attributeCompletionsForElement(info, item.name, item);
}
Expand Down Expand Up @@ -213,11 +215,12 @@ function elementCompletions(info: TemplateInfo, path: HtmlAstPath): Completions|
let htmlNames = elementNames().filter(name => !(name in hiddenHtmlElements));

// Collect the elements referenced by the selectors
let directiveElements =
getSelectors(info).selectors.map(selector => selector.element).filter(name => !!name);
let directiveElements = getSelectors(info)
.selectors.map(selector => selector.element)
.filter(name => !!name) as string[];

let components =
directiveElements.map<Completion>(name => ({kind: 'component', name: name !, sort: name !}));
directiveElements.map<Completion>(name => ({kind: 'component', name, sort: name}));
let htmlElements = htmlNames.map<Completion>(name => ({kind: 'element', name: name, sort: name}));

// Return components and html elements
Expand Down Expand Up @@ -262,25 +265,25 @@ function voidElementAttributeCompletions(info: TemplateInfo, path: HtmlAstPath):
undefined {
let tail = path.tail;
if (tail instanceof Text) {
let match = tail.value.match(/<(\w(\w|\d|-)*:)?(\w(\w|\d|-)*)\s/) !;
let match = tail.value.match(/<(\w(\w|\d|-)*:)?(\w(\w|\d|-)*)\s/);
// The position must be after the match, otherwise we are still in a place where elements
// are expected (such as `<|a` or `<a|`; we only want attributes for `<a |` or after).
if (match && path.position >= match.index ! + match[0].length + tail.sourceSpan.start.offset) {
if (match &&
path.position >= (match.index || 0) + match[0].length + tail.sourceSpan.start.offset) {
return attributeCompletionsForElement(info, match[3]);
}
}
}

class ExpressionVisitor extends NullTemplateVisitor {
private getExpressionScope: () => SymbolTable;
result: Completions;

constructor(
private info: TemplateInfo, private position: number, private attr?: Attribute,
private getExpressionScope?: () => SymbolTable) {
getExpressionScope?: () => SymbolTable) {
super();
if (!getExpressionScope) {
this.getExpressionScope = () => info.template.members;
}
this.getExpressionScope = getExpressionScope || (() => info.template.members);
}

visitDirectiveProperty(ast: BoundDirectivePropertyAst): void {
Expand Down Expand Up @@ -311,7 +314,8 @@ class ExpressionVisitor extends NullTemplateVisitor {
this.info.expressionParser.parseTemplateBindings(key, this.attr.value, null);

// find the template binding that contains the position
const valueRelativePosition = this.position - this.attr.valueSpan !.start.offset - 1;
if (!this.attr.valueSpan) return;
const valueRelativePosition = this.position - this.attr.valueSpan.start.offset - 1;
const bindings = templateBindingResult.templateBindings;
const binding =
bindings.find(
Expand Down Expand Up @@ -340,10 +344,12 @@ class ExpressionVisitor extends NullTemplateVisitor {
// We are after the '=' in a let clause. The valid values here are the members of the
// template reference's type parameter.
const directiveMetadata = selectorInfo.map.get(selector);
const contextTable =
this.info.template.query.getTemplateContext(directiveMetadata !.type.reference);
if (contextTable) {
this.result = this.symbolsToCompletions(contextTable.values());
if (directiveMetadata) {
const contextTable =
this.info.template.query.getTemplateContext(directiveMetadata.type.reference);
if (contextTable) {
this.result = this.symbolsToCompletions(contextTable.values());
}
}
} else if (binding.key && valueRelativePosition <= (binding.key.length - key.length)) {
keyCompletions();
Expand Down Expand Up @@ -371,7 +377,7 @@ class ExpressionVisitor extends NullTemplateVisitor {
const expressionPosition = this.position - ast.sourceSpan.start.offset;
if (inSpan(expressionPosition, ast.value.span)) {
const completions = getExpressionCompletions(
this.getExpressionScope !(), ast.value, expressionPosition, this.info.template.query);
this.getExpressionScope(), ast.value, expressionPosition, this.info.template.query);
if (completions) {
this.result = this.symbolsToCompletions(completions);
}
Expand All @@ -380,8 +386,8 @@ class ExpressionVisitor extends NullTemplateVisitor {

private attributeValueCompletions(value: AST, position?: number) {
const symbols = getExpressionCompletions(
this.getExpressionScope !(), value,
position == null ? this.attributeValuePosition : position, this.info.template.query);
this.getExpressionScope(), value, position == null ? this.attributeValuePosition : position,
this.info.template.query);
if (symbols) {
this.result = this.symbolsToCompletions(symbols);
}
Expand All @@ -393,12 +399,13 @@ class ExpressionVisitor extends NullTemplateVisitor {
}

private get attributeValuePosition() {
return this.position - this.attr !.valueSpan !.start.offset - 1;
if (this.attr && this.attr.valueSpan) {
return this.position - this.attr.valueSpan.start.offset - 1;
}
return 0;
}
}



function getSourceText(template: TemplateSource, span: Span): string {
return template.source.substring(span.start, span.end);
}
Expand Down
50 changes: 28 additions & 22 deletions packages/language-service/src/diagnostics.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export function getTemplateDiagnostics(
results.push(...ast.parseErrors.map<Diagnostic>(
e => ({
kind: DiagnosticKind.Error,
span: offsetSpan(spanOf(e.span) !, template.span.start),
span: offsetSpan(spanOf(e.span), template.span.start),
message: e.msg
})));
} else if (ast.templateAst) {
Expand Down Expand Up @@ -91,20 +91,24 @@ export function getDeclarationDiagnostics(

function getTemplateExpressionDiagnostics(
template: TemplateSource, astResult: AstResult): Diagnostics {
const info: TemplateInfo = {
template,
htmlAst: astResult.htmlAst !,
directive: astResult.directive !,
directives: astResult.directives !,
pipes: astResult.pipes !,
templateAst: astResult.templateAst !,
expressionParser: astResult.expressionParser !
};
const visitor = new ExpressionDiagnosticsVisitor(
info, (path: TemplateAstPath, includeEvent: boolean) =>
getExpressionScope(info, path, includeEvent));
templateVisitAll(visitor, astResult.templateAst !);
return visitor.diagnostics;
if (astResult.htmlAst && astResult.directive && astResult.directives && astResult.pipes &&
astResult.templateAst && astResult.expressionParser) {
const info: TemplateInfo = {
template,
htmlAst: astResult.htmlAst,
directive: astResult.directive,
directives: astResult.directives,
pipes: astResult.pipes,
templateAst: astResult.templateAst,
expressionParser: astResult.expressionParser
};
const visitor = new ExpressionDiagnosticsVisitor(
info, (path: TemplateAstPath, includeEvent: boolean) =>
getExpressionScope(info, path, includeEvent));
templateVisitAll(visitor, astResult.templateAst !);
return visitor.diagnostics;
}
return [];
}

class ExpressionDiagnosticsVisitor extends TemplateAstChildVisitor {
Expand Down Expand Up @@ -158,11 +162,11 @@ class ExpressionDiagnosticsVisitor extends TemplateAstChildVisitor {
if (context && !context.has(ast.value)) {
if (ast.value === '$implicit') {
this.reportError(
'The template context does not have an implicit value', spanOf(ast.sourceSpan) !);
'The template context does not have an implicit value', spanOf(ast.sourceSpan));
} else {
this.reportError(
`The template context does not defined a member called '${ast.value}'`,
spanOf(ast.sourceSpan) !);
spanOf(ast.sourceSpan));
}
}
}
Expand Down Expand Up @@ -233,11 +237,13 @@ class ExpressionDiagnosticsVisitor extends TemplateAstChildVisitor {
}
}

private reportError(message: string, span: Span) {
this.diagnostics.push({
span: offsetSpan(span, this.info.template.span.start),
kind: DiagnosticKind.Error, message
});
private reportError(message: string, span: Span|undefined) {
if (span) {
this.diagnostics.push({
span: offsetSpan(span, this.info.template.span.start),
kind: DiagnosticKind.Error, message
});
}
}

private reportWarning(message: string, span: Span) {
Expand Down
18 changes: 12 additions & 6 deletions packages/language-service/src/expressions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -495,8 +495,9 @@ class AstType implements ExpressionVisitor {
// The type of a method is the selected methods result type.
const method = receiverType.members().get(ast.name);
if (!method) return this.reportError(`Unknown method ${ast.name}`, ast);
if (!method.type !.callable) return this.reportError(`Member ${ast.name} is not callable`, ast);
const signature = method.type !.selectSignature(ast.args.map(arg => this.getType(arg)));
if (!method.type) return this.reportError(`Could not find a type for ${ast.name}`, ast);
if (!method.type.callable) return this.reportError(`Member ${ast.name} is not callable`, ast);
const signature = method.type.selectSignature(ast.args.map(arg => this.getType(arg)));
if (!signature)
return this.reportError(`Unable to resolve signature for call of method ${ast.name}`, ast);
return signature.result;
Expand Down Expand Up @@ -601,7 +602,9 @@ function visitChildren(ast: AST, visitor: ExpressionVisitor) {
visit(ast.falseExp);
},
visitFunctionCall(ast) {
visit(ast.target !);
if (ast.target) {
visit(ast.target);
}
visitAll(ast.args);
},
visitImplicitReceiver(ast) {},
Expand Down Expand Up @@ -676,7 +679,7 @@ function getReferences(info: TemplateInfo): SymbolDeclaration[] {

function processReferences(references: ReferenceAst[]) {
for (const reference of references) {
let type: Symbol = undefined !;
let type: Symbol|undefined = undefined;
if (reference.value) {
type = info.template.query.getTypeSymbol(tokenReference(reference.value));
}
Expand Down Expand Up @@ -721,7 +724,7 @@ function getVarDeclarations(info: TemplateInfo, path: TemplateAstPath): SymbolDe
.find(c => !!c);

// Determine the type of the context field referenced by variable.value.
let type: Symbol = undefined !;
let type: Symbol|undefined = undefined;
if (context) {
const value = context.get(variable.value);
if (value) {
Expand Down Expand Up @@ -762,7 +765,10 @@ function refinedVariableType(
const bindingType =
new AstType(info.template.members, info.template.query, {}).getType(ngForOfBinding.value);
if (bindingType) {
return info.template.query.getElementType(bindingType) !;
const result = info.template.query.getElementType(bindingType);
if (result) {
return result;
}
}
}
}
Expand Down
Loading

0 comments on commit 9138d4b

Please sign in to comment.