Skip to content

When adding completions for a module, don't get the type of the module if not necessary. #16768

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
3 commits merged into from
Jul 13, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3599,6 +3599,10 @@ namespace ts {
return previous[previous.length - 1];
}

export function skipAlias(symbol: Symbol, checker: TypeChecker) {
return symbol.flags & SymbolFlags.Alias ? checker.getAliasedSymbol(symbol) : symbol;
}

/** See comment on `declareModuleMember` in `binder.ts`. */
export function getCombinedLocalAndExportSymbolFlags(symbol: Symbol): SymbolFlags {
return symbol.exportSymbol ? symbol.exportSymbol.flags | symbol.flags : symbol.flags;
Expand Down
5 changes: 1 addition & 4 deletions src/services/codefixes/importFixes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,10 +222,7 @@ namespace ts.codefix {
}

function getUniqueSymbolId(symbol: Symbol) {
if (symbol.flags & SymbolFlags.Alias) {
return getSymbolId(checker.getAliasedSymbol(symbol));
}
return getSymbolId(symbol);
return getSymbolId(skipAlias(symbol, checker));
}

function checkSymbolHasMeaning(symbol: Symbol, meaning: SemanticMeaning) {
Expand Down
64 changes: 32 additions & 32 deletions src/services/completions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -596,46 +596,48 @@ namespace ts.Completions {
isNewIdentifierLocation = false;

// Since this is qualified name check its a type node location
const isTypeLocation = isPartOfTypeNode(node.parent) || insideJsDocTagTypeExpression;
const isTypeLocation = insideJsDocTagTypeExpression || isPartOfTypeNode(node.parent);
const isRhsOfImportDeclaration = isInRightSideOfInternalImportEqualsDeclaration(node);
if (node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.QualifiedName || node.kind === SyntaxKind.PropertyAccessExpression) {
if (isEntityName(node)) {
let symbol = typeChecker.getSymbolAtLocation(node);
if (symbol) {
symbol = skipAlias(symbol, typeChecker);

if (symbol.flags & (SymbolFlags.Module | SymbolFlags.Enum)) {
// Extract module or enum members
const exportedSymbols = typeChecker.getExportsOfModule(symbol);
const isValidValueAccess = (symbol: Symbol) => typeChecker.isValidPropertyAccess(<PropertyAccessExpression>(node.parent), symbol.getUnescapedName());
const isValidTypeAccess = (symbol: Symbol) => symbolCanBeReferencedAtTypeLocation(symbol);
const isValidAccess = isRhsOfImportDeclaration ?
// Any kind is allowed when dotting off namespace in internal import equals declaration
(symbol: Symbol) => isValidTypeAccess(symbol) || isValidValueAccess(symbol) :
isTypeLocation ? isValidTypeAccess : isValidValueAccess;
for (const symbol of exportedSymbols) {
if (isValidAccess(symbol)) {
symbols.push(symbol);
}
}

// This is an alias, follow what it aliases
if (symbol && symbol.flags & SymbolFlags.Alias) {
symbol = typeChecker.getAliasedSymbol(symbol);
}

if (symbol && symbol.flags & SymbolFlags.HasExports) {
// Extract module or enum members
const exportedSymbols = typeChecker.getExportsOfModule(symbol);
const isValidValueAccess = (symbol: Symbol) => typeChecker.isValidPropertyAccess(<PropertyAccessExpression>(node.parent), symbol.getUnescapedName());
const isValidTypeAccess = (symbol: Symbol) => symbolCanBeReferencedAtTypeLocation(symbol);
const isValidAccess = isRhsOfImportDeclaration ?
// Any kind is allowed when dotting off namespace in internal import equals declaration
(symbol: Symbol) => isValidTypeAccess(symbol) || isValidValueAccess(symbol) :
isTypeLocation ? isValidTypeAccess : isValidValueAccess;
forEach(exportedSymbols, symbol => {
if (isValidAccess(symbol)) {
symbols.push(symbol);
// If the module is merged with a value, we must get the type of the class and add its propertes (for inherited static methods).
if (!isTypeLocation && symbol.declarations.some(d => d.kind !== SyntaxKind.SourceFile && d.kind !== SyntaxKind.ModuleDeclaration && d.kind !== SyntaxKind.EnumDeclaration)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't possible to merge enum with anything else right so adding ! might be useful to not enumerate through the declarations?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible to merge enums and namespaces. But we could handle enums separately, and assert that getExportsOfModule is only called for modules.

addTypeProperties(typeChecker.getTypeOfSymbolAtLocation(symbol, node));
}
});

return;
}
}
}

if (!isTypeLocation) {
const type = typeChecker.getTypeAtLocation(node);
if (type) addTypeProperties(type);
addTypeProperties(typeChecker.getTypeAtLocation(node));
}
}

function addTypeProperties(type: Type) {
if (type) {
// Filter private properties
for (const symbol of type.getApparentProperties()) {
if (typeChecker.isValidPropertyAccess(<PropertyAccessExpression>(node.parent), symbol.getUnescapedName())) {
symbols.push(symbol);
}
// Filter private properties
for (const symbol of type.getApparentProperties()) {
if (typeChecker.isValidPropertyAccess(<PropertyAccessExpression>(node.parent), symbol.getUnescapedName())) {
symbols.push(symbol);
}
}

Expand Down Expand Up @@ -811,15 +813,13 @@ namespace ts.Completions {
symbol = symbol.exportSymbol || symbol;

// This is an alias, follow what it aliases
if (symbol && symbol.flags & SymbolFlags.Alias) {
symbol = typeChecker.getAliasedSymbol(symbol);
}
symbol = skipAlias(symbol, typeChecker);

if (symbol.flags & SymbolFlags.Type) {
return true;
}

if (symbol.flags & (SymbolFlags.ValueModule | SymbolFlags.NamespaceModule)) {
if (symbol.flags & SymbolFlags.Module) {
const exportedSymbols = typeChecker.getExportsOfModule(symbol);
// If the exported symbols contains type,
// symbol can be referenced at locations where type is allowed
Expand Down
21 changes: 21 additions & 0 deletions tests/cases/fourslash/completionsNamespaceMergedWithClass.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/// <reference path='fourslash.ts'/>

////class C {
//// static m() { }
////}
////
////class D extends C {}
////namespace D {
//// export type T = number;
////}
////
////let x: D./*type*/;
////D./*value*/

goTo.marker("type");
verify.completionListContains("T");
verify.not.completionListContains("m");

goTo.marker("value");
verify.not.completionListContains("T");
verify.completionListContains("m");
16 changes: 16 additions & 0 deletions tests/cases/fourslash/completionsNamespaceMergedWithObject.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/// <reference path='fourslash.ts'/>

////namespace N {
//// export type T = number;
////}
////const N = { m() {} };
////let x: N./*type*/;
////N./*value*/;

goTo.marker("type");
verify.completionListContains("T");
verify.not.completionListContains("m");

goTo.marker("value");
verify.not.completionListContains("T");
verify.completionListContains("m");