Skip to content

Completion for default export should be '.default' #16742

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
5 commits merged into from
Jul 11, 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
9 changes: 3 additions & 6 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11876,6 +11876,8 @@ namespace ts {
}

function getTypeOfSymbolAtLocation(symbol: Symbol, location: Node) {
symbol = symbol.exportSymbol || symbol;

// If we have an identifier or a property access at the given location, if the location is
// an dotted name expression, and if the location is not an assignment target, obtain the type
// of the expression (which will reflect control flow analysis). If the expression indeed
Expand Down Expand Up @@ -22282,11 +22284,6 @@ namespace ts {
}

switch (location.kind) {
case SyntaxKind.SourceFile:
if (!isExternalOrCommonJsModule(<SourceFile>location)) {
break;
}
// falls through
case SyntaxKind.ModuleDeclaration:
copySymbols(getSymbolOfNode(location).exports, meaning & SymbolFlags.ModuleMember);
break;
Expand Down Expand Up @@ -22338,7 +22335,7 @@ namespace ts {
* @param meaning meaning of symbol to filter by before adding to symbol table
*/
function copySymbol(symbol: Symbol, meaning: SymbolFlags): void {
if (symbol.flags & meaning) {
if (getCombinedLocalAndExportSymbolFlags(symbol) & meaning) {
const id = symbol.name;
// We will copy all symbol regardless of its reserved name because
// symbolsToArray will check whether the key is a reserved name and
Expand Down
5 changes: 5 additions & 0 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3590,6 +3590,11 @@ namespace ts {
}
return previous[previous.length - 1];
}

/** See comment on `declareModuleMember` in `binder.ts`. */
export function getCombinedLocalAndExportSymbolFlags(symbol: Symbol): SymbolFlags {
return symbol.exportSymbol ? symbol.exportSymbol.flags | symbol.flags : symbol.flags;
Copy link
Member

Choose a reason for hiding this comment

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

why combine flags in the true case instead of just returning symbol.exportSymbol.flags?

Copy link
Author

Choose a reason for hiding this comment

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

In findAllRefsForDefaultExport08 (and findAllRefsForDefaultExport02), we have an exported class and a non-exported namespace merged with it. So the local symbol will have flags that the exported symbol is lacking; it has NamespaceModule|ExportValue|ExportType while the exported symbol just has Class.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense

}
}

namespace ts {
Expand Down
24 changes: 24 additions & 0 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -949,6 +949,22 @@ namespace FourSlash {
this.verifySymbol(symbol, declarationRanges);
}

public symbolsInScope(range: Range): ts.Symbol[] {
const node = this.goToAndGetNode(range);
return this.getChecker().getSymbolsInScope(node, ts.SymbolFlags.Value | ts.SymbolFlags.Type | ts.SymbolFlags.Namespace);
Copy link
Member

Choose a reason for hiding this comment

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

nit: doesn't SymbolFlags have an alias defined like SymbolFlags.All that's equivalent?

Copy link
Author

@ghost ghost Jul 10, 2017

Choose a reason for hiding this comment

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

Not currently; All wouldn't be a good name since that excludes some (Constructor, Signature, and everything from ExportValue on), but this combination is often useful; I'm tempted to just name it ValueTypeNamespace.

}

public verifyTypeOfSymbolAtLocation(range: Range, symbol: ts.Symbol, expected: string): void {
const node = this.goToAndGetNode(range);
const checker = this.getChecker();
const type = checker.getTypeOfSymbolAtLocation(symbol, node);

const actual = checker.typeToString(type);
if (actual !== expected) {
this.raiseError(`Expected: '${expected}', actual: '${actual}'`);
}
}

private verifyReferencesAre(expectedReferences: Range[]) {
const actualReferences = this.getReferencesAtCaret() || [];

Expand Down Expand Up @@ -3426,6 +3442,10 @@ namespace FourSlashInterface {
public markerByName(s: string): FourSlash.Marker {
return this.state.getMarkerByName(s);
}

public symbolsInScope(range: FourSlash.Range): ts.Symbol[] {
return this.state.symbolsInScope(range);
}
}

export class GoTo {
Expand Down Expand Up @@ -3694,6 +3714,10 @@ namespace FourSlashInterface {
this.state.verifySymbolAtLocation(startRange, declarationRanges);
}

public typeOfSymbolAtLocation(range: FourSlash.Range, symbol: ts.Symbol, expected: string) {
Copy link
Member

Choose a reason for hiding this comment

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

can't we get this from the quick info? I don't know whether we need a separate method for comparing the stringified type of a symbol.

Copy link
Author

Choose a reason for hiding this comment

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

Well, quick info gets us the type of the symbol under the cursor -- this lets us call getTypeOfSymbolAtLocation on any symbol, at the current location.

this.state.verifyTypeOfSymbolAtLocation(range, symbol, expected);
}

public referencesOf(start: FourSlash.Range, references: FourSlash.Range[]) {
this.state.verifyReferencesOf(start, references);
}
Expand Down
95 changes: 40 additions & 55 deletions src/services/completions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ namespace ts.Completions {
}
else {
if ((!symbols || symbols.length === 0) && keywordFilters === KeywordCompletionFilters.None) {
return undefined;
return undefined;
}

getCompletionEntriesFromSymbols(symbols, entries, location, /*performCharacterChecks*/ true, typeChecker, compilerOptions.target, log);
Expand Down Expand Up @@ -112,7 +112,7 @@ namespace ts.Completions {
// Try to get a valid display name for this symbol, if we could not find one, then ignore it.
// We would like to only show things that can be added after a dot, so for instance numeric properties can
// not be accessed with a dot (a.1 <- invalid)
const displayName = getCompletionEntryDisplayNameForSymbol(typeChecker, symbol, target, performCharacterChecks, location);
const displayName = getCompletionEntryDisplayNameForSymbol(symbol, target, performCharacterChecks);
if (!displayName) {
return undefined;
}
Expand Down Expand Up @@ -307,7 +307,7 @@ namespace ts.Completions {
// We don't need to perform character checks here because we're only comparing the
// name against 'entryName' (which is known to be good), not building a new
// completion entry.
const symbol = forEach(symbols, s => getCompletionEntryDisplayNameForSymbol(typeChecker, s, compilerOptions.target, /*performCharacterChecks*/ false, location) === entryName ? s : undefined);
const symbol = forEach(symbols, s => getCompletionEntryDisplayNameForSymbol(s, compilerOptions.target, /*performCharacterChecks*/ false) === entryName ? s : undefined);

if (symbol) {
const { displayParts, documentation, symbolKind, tags } = SymbolDisplay.getSymbolDisplayPartsDocumentationAndSymbolKind(typeChecker, symbol, sourceFile, location, location, SemanticMeaning.All);
Expand Down Expand Up @@ -341,20 +341,14 @@ namespace ts.Completions {
return undefined;
}

export function getCompletionEntrySymbol(typeChecker: TypeChecker, log: (message: string) => void, compilerOptions: CompilerOptions, sourceFile: SourceFile, position: number, entryName: string): Symbol {
export function getCompletionEntrySymbol(typeChecker: TypeChecker, log: (message: string) => void, compilerOptions: CompilerOptions, sourceFile: SourceFile, position: number, entryName: string): Symbol | undefined {
// Compute all the completion symbols again.
const completionData = getCompletionData(typeChecker, log, sourceFile, position);
if (completionData) {
const { symbols, location } = completionData;

// Find the symbol with the matching entry name.
// We don't need to perform character checks here because we're only comparing the
// name against 'entryName' (which is known to be good), not building a new
// completion entry.
return forEach(symbols, s => getCompletionEntryDisplayNameForSymbol(typeChecker, s, compilerOptions.target, /*performCharacterChecks*/ false, location) === entryName ? s : undefined);
}

return undefined;
// Find the symbol with the matching entry name.
// We don't need to perform character checks here because we're only comparing the
// name against 'entryName' (which is known to be good), not building a new
// completion entry.
return completionData && forEach(completionData.symbols, s => getCompletionEntryDisplayNameForSymbol(s, compilerOptions.target, /*performCharacterChecks*/ false) === entryName ? s : undefined);
}

interface CompletionData {
Expand All @@ -369,7 +363,7 @@ namespace ts.Completions {
}
type Request = { kind: "JsDocTagName" } | { kind: "JsDocTag" } | { kind: "JsDocParameterName", tag: JSDocParameterTag };

function getCompletionData(typeChecker: TypeChecker, log: (message: string) => void, sourceFile: SourceFile, position: number): CompletionData {
function getCompletionData(typeChecker: TypeChecker, log: (message: string) => void, sourceFile: SourceFile, position: number): CompletionData | undefined {
const isJavaScriptFile = isSourceFileJavaScript(sourceFile);

let request: Request | undefined;
Expand Down Expand Up @@ -615,7 +609,7 @@ namespace ts.Completions {
// 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 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) :
Expand All @@ -630,7 +624,7 @@ namespace ts.Completions {

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

Expand All @@ -642,17 +636,17 @@ namespace ts.Completions {
symbols.push(symbol);
}
}
}

if (isJavaScriptFile && type.flags & TypeFlags.Union) {
// In javascript files, for union types, we don't just get the members that
// the individual types have in common, we also include all the members that
// each individual type has. This is because we're going to add all identifiers
// anyways. So we might as well elevate the members that were at least part
// of the individual types to a higher status since we know what they are.
const unionType = <UnionType>type;
for (const elementType of unionType.types) {
addTypeProperties(elementType);
}
if (isJavaScriptFile && type.flags & TypeFlags.Union) {
// In javascript files, for union types, we don't just get the members that
// the individual types have in common, we also include all the members that
// each individual type has. This is because we're going to add all identifiers
// anyways. So we might as well elevate the members that were at least part
// of the individual types to a higher status since we know what they are.
const unionType = <UnionType>type;
for (const elementType of unionType.types) {
addTypeProperties(elementType);
}
}
}
Expand Down Expand Up @@ -777,12 +771,12 @@ namespace ts.Completions {
(!isContextTokenValueLocation(contextToken) &&
(isPartOfTypeNode(location) || isContextTokenTypeLocation(contextToken)))) {
// Its a type, but you can reach it by namespace.type as well
return symbolCanbeReferencedAtTypeLocation(symbol);
return symbolCanBeReferencedAtTypeLocation(symbol);
}
}

// expressions are value space (which includes the value namespaces)
return !!(symbol.flags & SymbolFlags.Value);
return !!(getCombinedLocalAndExportSymbolFlags(symbol) & SymbolFlags.Value);
});
}

Expand Down Expand Up @@ -812,7 +806,9 @@ namespace ts.Completions {
}
}

function symbolCanbeReferencedAtTypeLocation(symbol: Symbol): boolean {
function symbolCanBeReferencedAtTypeLocation(symbol: Symbol): boolean {
symbol = symbol.exportSymbol || symbol;

// This is an alias, follow what it aliases
if (symbol && symbol.flags & SymbolFlags.Alias) {
symbol = typeChecker.getAliasedSymbol(symbol);
Expand All @@ -826,7 +822,7 @@ namespace ts.Completions {
const exportedSymbols = typeChecker.getExportsOfModule(symbol);
// If the exported symbols contains type,
// symbol can be referenced at locations where type is allowed
return forEach(exportedSymbols, symbolCanbeReferencedAtTypeLocation);
return forEach(exportedSymbols, symbolCanBeReferencedAtTypeLocation);
}
}

Expand Down Expand Up @@ -1598,47 +1594,36 @@ namespace ts.Completions {
/**
* Get the name to be display in completion from a given symbol.
*
* @return undefined if the name is of external module otherwise a name with striped of any quote
* @return undefined if the name is of external module
*/
function getCompletionEntryDisplayNameForSymbol(typeChecker: TypeChecker, symbol: Symbol, target: ScriptTarget, performCharacterChecks: boolean, location: Node): string {
const displayName: string = getDeclaredName(typeChecker, symbol, location);

if (displayName) {
const firstCharCode = displayName.charCodeAt(0);
// First check of the displayName is not external module; if it is an external module, it is not valid entry
if ((symbol.flags & SymbolFlags.Namespace) && (firstCharCode === CharacterCodes.singleQuote || firstCharCode === CharacterCodes.doubleQuote)) {
function getCompletionEntryDisplayNameForSymbol(symbol: Symbol, target: ScriptTarget, performCharacterChecks: boolean): string | undefined {
const name = symbol.getUnescapedName();
if (!name) return undefined;

// First check of the displayName is not external module; if it is an external module, it is not valid entry
if (symbol.flags & SymbolFlags.Namespace) {
const firstCharCode = name.charCodeAt(0);
if (firstCharCode === CharacterCodes.singleQuote || firstCharCode === CharacterCodes.doubleQuote) {
// If the symbol is external module, don't show it in the completion list
// (i.e declare module "http" { const x; } | // <= request completion here, "http" should not be there)
return undefined;
}
}

return getCompletionEntryDisplayName(displayName, target, performCharacterChecks);
return getCompletionEntryDisplayName(name, target, performCharacterChecks);
}

/**
* Get a displayName from a given for completion list, performing any necessary quotes stripping
* and checking whether the name is valid identifier name.
*/
function getCompletionEntryDisplayName(name: string, target: ScriptTarget, performCharacterChecks: boolean): string {
if (!name) {
return undefined;
}

name = stripQuotes(name);

if (!name) {
return undefined;
}

// If the user entered name for the symbol was quoted, removing the quotes is not enough, as the name could be an
// invalid identifier name. We need to check if whatever was inside the quotes is actually a valid identifier name.
// e.g "b a" is valid quoted name but when we strip off the quotes, it is invalid.
// We, thus, need to check if whatever was inside the quotes is actually a valid identifier name.
if (performCharacterChecks) {
if (!isIdentifierText(name, target)) {
return undefined;
}
if (performCharacterChecks && !isIdentifierText(name, target)) {
return undefined;
}

return name;
Expand Down
57 changes: 24 additions & 33 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2063,42 +2063,33 @@ namespace ts {
}

function initializeNameTable(sourceFile: SourceFile): void {
const nameTable = createUnderscoreEscapedMap<number>();

walk(sourceFile);
sourceFile.nameTable = nameTable;
const nameTable = sourceFile.nameTable = createUnderscoreEscapedMap<number>();
sourceFile.forEachChild(function walk(node) {
if ((isIdentifier(node) || isStringOrNumericLiteral(node) && literalIsName(node)) && node.text) {
const text = getEscapedTextOfIdentifierOrLiteral(node);
nameTable.set(text, nameTable.get(text) === undefined ? node.pos : -1);
}

function walk(node: Node) {
switch (node.kind) {
case SyntaxKind.Identifier:
setNameTable((<Identifier>node).text, node);
break;
case SyntaxKind.StringLiteral:
case SyntaxKind.NumericLiteral:
// We want to store any numbers/strings if they were a name that could be
// related to a declaration. So, if we have 'import x = require("something")'
// then we want 'something' to be in the name table. Similarly, if we have
// "a['propname']" then we want to store "propname" in the name table.
if (isDeclarationName(node) ||
node.parent.kind === SyntaxKind.ExternalModuleReference ||
isArgumentOfElementAccessExpression(node) ||
isLiteralComputedPropertyDeclarationName(node)) {
setNameTable(getEscapedTextOfIdentifierOrLiteral((<LiteralExpression>node)), node);
}
break;
default:
forEachChild(node, walk);
if (node.jsDoc) {
for (const jsDoc of node.jsDoc) {
forEachChild(jsDoc, walk);
}
}
forEachChild(node, walk);
if (node.jsDoc) {
for (const jsDoc of node.jsDoc) {
forEachChild(jsDoc, walk);
}
}
}
});
}

function setNameTable(text: __String, node: ts.Node): void {
nameTable.set(text, nameTable.get(text) === undefined ? node.pos : -1);
}
/**
* We want to store any numbers/strings if they were a name that could be
* related to a declaration. So, if we have 'import x = require("something")'
* then we want 'something' to be in the name table. Similarly, if we have
* "a['propname']" then we want to store "propname" in the name table.
*/
function literalIsName(node: ts.StringLiteral | ts.NumericLiteral): boolean {
return isDeclarationName(node) ||
node.parent.kind === SyntaxKind.ExternalModuleReference ||
isArgumentOfElementAccessExpression(node) ||
isLiteralComputedPropertyDeclarationName(node);
}

function isObjectLiteralElement(node: Node): node is ObjectLiteralElement {
Expand Down
Loading