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 1 commit
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
25 changes: 19 additions & 6 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,15 @@ namespace ts {
getAccessibleSymbolChain,
getTypePredicateOfSignature,
resolveExternalModuleSymbol,
tryGetThisTypeAt: node => {
node = getParseTreeNode(node);
return node && tryGetThisTypeAt(node);
},
isMemberSymbol: symbol =>
symbol.flags & SymbolFlags.ClassMember
Copy link
Author

Choose a reason for hiding this comment

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

Asked @sandersn and @weswigham and they didn't know why these are properties, but if I try to change undefined to a variable I can't build the compiler.

Copy link
Contributor

Choose a reason for hiding this comment

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

seems that this should be in utilities instead on the checker. nothing here is about the checker internal state, but rather how we mark symbols.

&& symbol !== argumentsSymbol
&& symbol !== undefinedSymbol
&& !(symbol.parent && symbol.parent.flags & SymbolFlags.Module),
};

const tupleTypes: GenericType[] = [];
Expand Down Expand Up @@ -13268,6 +13277,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 +13325,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
3 changes: 3 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2919,6 +2919,9 @@ 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 */ isMemberSymbol(symbol: Symbol): boolean;
}

/* @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
34 changes: 26 additions & 8 deletions src/services/completions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,6 @@ namespace ts.Completions {
return undefined;
}
const { name, needsConvertPropertyAccess } = info;
Debug.assert(!(needsConvertPropertyAccess && !propertyAccessToConvert));
if (needsConvertPropertyAccess && !includeInsertTextCompletions) {
return undefined;
}
Expand All @@ -186,14 +185,24 @@ 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 !== undefined),
isRecommended: trueOrUndefined(isRecommendedCompletionMatch(symbol, recommendedCompletion, typeChecker)),
...getInsertTextAndReplacementSpan(),
Copy link
Contributor

Choose a reason for hiding this comment

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

can we optimize this a bit instead of generating an extra object allocation on every entry.

};

function getInsertTextAndReplacementSpan(): { insertText?: string, replacementSpan?: TextSpan } {
if (kind === CompletionKind.Global) {
if (typeChecker.isMemberSymbol(symbol)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

seems kinda weird that we add the symbol, then later on ask the checker if they were the ones we added.. i wounder if we can do something different here..

Copy link
Contributor

Choose a reason for hiding this comment

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

alas.. i could not come up with a better idea here myself. ignore my previous rant.

Copy link
Author

@ghost ghost Jan 17, 2018

Choose a reason for hiding this comment

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

We could use SymbolOriginInfoMap with a union type { type: "this-type" } | { type: "export", ... }, would that be a good idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah.. that is not bad..

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


Expand Down Expand Up @@ -1097,6 +1106,15 @@ 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) {
symbols.push(...getPropertiesForCompletion(thisType, typeChecker, /*isForAccess*/ true));
}
}

if (options.includeExternalModuleExports) {
getSymbolsFromOtherSourceFileExports(symbols, previousToken && isIdentifier(previousToken) ? previousToken.text : "", target);
}
Expand Down Expand Up @@ -2052,13 +2070,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