Skip to content
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

Add completions from the 'this' type #21231

Merged
2 commits merged into from
Jan 17, 2018
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
20 changes: 14 additions & 6 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,10 @@ namespace ts {
getAccessibleSymbolChain,
getTypePredicateOfSignature,
resolveExternalModuleSymbol,
tryGetThisTypeAt: node => {
node = getParseTreeNode(node);
return node && tryGetThisTypeAt(node);
},
};

const tupleTypes: GenericType[] = [];
Expand Down Expand Up @@ -13268,6 +13272,16 @@ namespace ts {
if (needToCaptureLexicalThis) {
captureLexicalThis(node, container);
}

const type = tryGetThisTypeAt(node, container);
if (!type && noImplicitThis) {
// With noImplicitThis, functions may not reference 'this' if it has type 'any'
error(node, Diagnostics.this_implicitly_has_type_any_because_it_does_not_have_a_type_annotation);
}
return type || anyType;
}

function tryGetThisTypeAt(node: Node, container = getThisContainer(node, /*includeArrowFunctions*/ false)): Type | undefined {
if (isFunctionLike(container) &&
(!isInParameterInitializerBeforeContainingFunction(node) || getThisParameter(container))) {
// Note: a parameter initializer should refer to class-this unless function-this is explicitly annotated.
Expand Down Expand Up @@ -13306,12 +13320,6 @@ namespace ts {
return type;
}
}

if (noImplicitThis) {
// With noImplicitThis, functions may not reference 'this' if it has type 'any'
error(node, Diagnostics.this_implicitly_has_type_any_because_it_does_not_have_a_type_annotation);
}
return anyType;
}

function getTypeForThisExpressionFromJSDoc(node: Node) {
Expand Down
2 changes: 2 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2919,6 +2919,8 @@ namespace ts {
/* @internal */ getAccessibleSymbolChain(symbol: Symbol, enclosingDeclaration: Node | undefined, meaning: SymbolFlags, useOnlyExternalAliasing: boolean): Symbol[] | undefined;
/* @internal */ getTypePredicateOfSignature(signature: Signature): TypePredicate;
/* @internal */ resolveExternalModuleSymbol(symbol: Symbol): Symbol;
/** @param node A location where we might consider accessing `this`. Not necessarily a ThisExpression. */
/* @internal */ tryGetThisTypeAt(node: Node): Type | undefined;
}

/* @internal */
Expand Down
4 changes: 3 additions & 1 deletion src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3152,8 +3152,9 @@ Actual: ${stringify(fullActual)}`);
assert.isTrue(TestState.textSpansEqual(span, item.replacementSpan), this.assertionMessageAtLastKnownMarker(stringify(span) + " does not equal " + stringify(item.replacementSpan) + " replacement span for " + entryId));
}

assert.equal(item.hasAction, hasAction);
assert.equal(item.hasAction, hasAction, "hasAction");
assert.equal(item.isRecommended, options && options.isRecommended, "isRecommended");
assert.equal(item.insertText, options && options.insertText, "insertText");
}

private findFile(indexOrName: string | number) {
Expand Down Expand Up @@ -4615,6 +4616,7 @@ namespace FourSlashInterface {
export interface VerifyCompletionListContainsOptions extends ts.GetCompletionsAtPositionOptions {
sourceDisplay: string;
isRecommended?: true;
insertText?: string;
}

export interface NewContentOptions {
Expand Down
51 changes: 36 additions & 15 deletions src/services/completions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
namespace ts.Completions {
export type Log = (message: string) => void;

interface SymbolOriginInfo {
type SymbolOriginInfo = { type: "this-type" } | SymbolOriginInfoExport;
interface SymbolOriginInfoExport {
type: "export";
moduleSymbol: Symbol;
isDefaultExport: boolean;
}
Expand Down Expand Up @@ -167,11 +169,21 @@ namespace ts.Completions {
return undefined;
}
const { name, needsConvertPropertyAccess } = info;
Debug.assert(!(needsConvertPropertyAccess && !propertyAccessToConvert));
if (needsConvertPropertyAccess && !includeInsertTextCompletions) {
return undefined;
}

let insertText: string | undefined;
let replacementSpan: TextSpan | undefined;
if (kind === CompletionKind.Global && origin && origin.type === "this-type") {
insertText = needsConvertPropertyAccess ? `this["${name}"]` : `this.${name}`;
Copy link
Member

Choose a reason for hiding this comment

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

do you not need replacement span eg. if refering to property x and you want to replace it with this.x ?

Copy link
Author

Choose a reason for hiding this comment

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

The default replacement span should already be the identifier itself. We only need to set replacementSpan if we are replacing something else, too, such as replacing .x with ["x"].

}
else if (needsConvertPropertyAccess) {
// TODO: GH#20619 Use configured quote style
insertText = `["${name}"]`;
replacementSpan = createTextSpanFromBounds(findChildOfKind(propertyAccessToConvert!, SyntaxKind.DotToken, sourceFile)!.getStart(sourceFile), propertyAccessToConvert!.name.end);
}

// TODO(drosen): Right now we just permit *all* semantic meanings when calling
// 'getSymbolKind' which is permissible given that it is backwards compatible; but
// really we should consider passing the meaning for the node so that we don't report
Expand All @@ -186,13 +198,10 @@ namespace ts.Completions {
kindModifiers: SymbolDisplay.getSymbolModifiers(symbol),
sortText: "0",
source: getSourceFromOrigin(origin),
// TODO: GH#20619 Use configured quote style
insertText: needsConvertPropertyAccess ? `["${name}"]` : undefined,
replacementSpan: needsConvertPropertyAccess
? createTextSpanFromBounds(findChildOfKind(propertyAccessToConvert, SyntaxKind.DotToken, sourceFile)!.getStart(sourceFile), propertyAccessToConvert.name.end)
: undefined,
hasAction: trueOrUndefined(needsConvertPropertyAccess || origin !== undefined),
hasAction: trueOrUndefined(!!origin && origin.type === "export"),
isRecommended: trueOrUndefined(isRecommendedCompletionMatch(symbol, recommendedCompletion, typeChecker)),
insertText,
replacementSpan,
};
}

Expand All @@ -207,7 +216,7 @@ namespace ts.Completions {
}

function getSourceFromOrigin(origin: SymbolOriginInfo | undefined): string | undefined {
return origin && stripQuotes(origin.moduleSymbol.name);
return origin && origin.type === "export" ? stripQuotes(origin.moduleSymbol.name) : undefined;
}

function getCompletionEntriesFromSymbols(
Expand Down Expand Up @@ -484,7 +493,7 @@ namespace ts.Completions {
}

function getSymbolName(symbol: Symbol, origin: SymbolOriginInfo | undefined, target: ScriptTarget): string {
return origin && origin.isDefaultExport && symbol.escapedName === InternalSymbolName.Default
return origin && origin.type === "export" && origin.isDefaultExport && symbol.escapedName === InternalSymbolName.Default
// Name of "export default foo;" is "foo". Name of "export default 0" is the filename converted to camelCase.
? firstDefined(symbol.declarations, d => isExportAssignment(d) && isIdentifier(d.expression) ? d.expression.text : undefined)
|| codefix.moduleSymbolToValidIdentifier(origin.moduleSymbol, target)
Expand Down Expand Up @@ -570,13 +579,13 @@ namespace ts.Completions {
allSourceFiles: ReadonlyArray<SourceFile>,
): CodeActionsAndSourceDisplay {
const symbolOriginInfo = symbolToOriginInfoMap[getSymbolId(symbol)];
return symbolOriginInfo
return symbolOriginInfo && symbolOriginInfo.type === "export"
? getCodeActionsAndSourceDisplayForImport(symbolOriginInfo, symbol, program, checker, host, compilerOptions, sourceFile, previousToken, formatContext, getCanonicalFileName, allSourceFiles)
: { codeActions: undefined, sourceDisplay: undefined };
}

function getCodeActionsAndSourceDisplayForImport(
symbolOriginInfo: SymbolOriginInfo,
symbolOriginInfo: SymbolOriginInfoExport,
symbol: Symbol,
program: Program,
checker: TypeChecker,
Expand Down Expand Up @@ -1097,6 +1106,18 @@ namespace ts.Completions {
const symbolMeanings = SymbolFlags.Type | SymbolFlags.Value | SymbolFlags.Namespace | SymbolFlags.Alias;

symbols = typeChecker.getSymbolsInScope(scopeNode, symbolMeanings);

// Need to insert 'this.' before properties of `this` type, so only do that if `includeInsertTextCompletions`
if (options.includeInsertTextCompletions && scopeNode.kind !== SyntaxKind.SourceFile) {
const thisType = typeChecker.tryGetThisTypeAt(scopeNode);
if (thisType) {
for (const symbol of getPropertiesForCompletion(thisType, typeChecker, /*isForAccess*/ true)) {
symbolToOriginInfoMap[getSymbolId(symbol)] = { type: "this-type" };
symbols.push(symbol);
}
}
}

if (options.includeExternalModuleExports) {
getSymbolsFromOtherSourceFileExports(symbols, previousToken && isIdentifier(previousToken) ? previousToken.text : "", target);
}
Expand Down Expand Up @@ -1210,10 +1231,10 @@ namespace ts.Completions {
symbol = getLocalSymbolForExportDefault(symbol) || symbol;
}

const origin: SymbolOriginInfo = { moduleSymbol, isDefaultExport };
const origin: SymbolOriginInfo = { type: "export", moduleSymbol, isDefaultExport };
if (stringContainsCharactersInOrder(getSymbolName(symbol, origin, target).toLowerCase(), tokenTextLowerCase)) {
symbols.push(symbol);
symbolToOriginInfoMap[getSymbolId(symbol)] = { moduleSymbol, isDefaultExport };
symbolToOriginInfoMap[getSymbolId(symbol)] = origin;
}
}
});
Expand Down Expand Up @@ -2052,13 +2073,13 @@ namespace ts.Completions {
if (isIdentifierText(name, target)) return validIdentiferResult;
switch (kind) {
case CompletionKind.None:
case CompletionKind.Global:
case CompletionKind.MemberLike:
return undefined;
case CompletionKind.ObjectPropertyDeclaration:
// TODO: GH#18169
return { name: JSON.stringify(name), needsConvertPropertyAccess: false };
case CompletionKind.PropertyAccess:
case CompletionKind.Global:
// Don't add a completion for a name starting with a space. See https://github.com/Microsoft/TypeScript/pull/20547
return name.charCodeAt(0) === CharacterCodes.space ? undefined : { name, needsConvertPropertyAccess: true };
case CompletionKind.String:
Expand Down
4 changes: 2 additions & 2 deletions tests/cases/fourslash/completionListInScope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
//// interface localInterface {}
//// export interface exportedInterface {}
////
//// module localModule {
//// module localModule {
//// export var x = 0;
//// }
//// export module exportedModule {
Expand All @@ -38,7 +38,7 @@
//// interface localInterface2 {}
//// export interface exportedInterface2 {}
////
//// module localModule2 {
//// module localModule2 {
//// export var x = 0;
//// }
//// export module exportedModule2 {
Expand Down
29 changes: 29 additions & 0 deletions tests/cases/fourslash/completionsThisType.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/// <reference path="fourslash.ts" />

////class C {
//// "foo bar": number;
//// xyz() {
//// /**/
//// }
////}
////
////function f(this: { x: number }) { /*f*/ }

goTo.marker("");

verify.completionListContains("xyz", "(method) C.xyz(): void", "", "method", undefined, undefined, {
includeInsertTextCompletions: true,
insertText: "this.xyz",
});

verify.completionListContains("foo bar", '(property) C["foo bar"]: number', "", "property", undefined, undefined, {
includeInsertTextCompletions: true,
insertText: 'this["foo bar"]',
});

goTo.marker("f");

verify.completionListContains("x", "(property) x: number", "", "property", undefined, undefined, {
includeInsertTextCompletions: true,
insertText: "this.x",
});
8 changes: 7 additions & 1 deletion tests/cases/fourslash/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,13 @@ declare namespace FourSlashInterface {
kind?: string | { kind?: string, kindModifiers?: string },
spanIndex?: number,
hasAction?: boolean,
options?: { includeExternalModuleExports?: boolean, sourceDisplay?: string, isRecommended?: true },
options?: {
includeExternalModuleExports?: boolean,
includeInsertTextCompletions?: boolean,
sourceDisplay?: string,
isRecommended?: true,
insertText?: string,
},
): void;
completionListItemsCountIsGreaterThan(count: number): void;
completionListIsEmpty(): void;
Expand Down