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.

Sign up for GitHub

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

Fix string literal completions when a partially-typed string fixes inference to a type parameter #48410

Merged
merged 2 commits into from
Mar 29, 2022
Merged

Fix string literal completions when a partially-typed string fixes inference to a type parameter #48410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c13d850
Add failing test
andrewbranch Feb 11, 2022
0d266cd
Fix string literal completions when a partially-typed string fixes in…
andrewbranch Mar 23, 2022
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
90 changes: 52 additions & 38 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,15 +175,16 @@ namespace ts {
}

const enum CheckMode {
Normal = 0, // Normal type checking
Contextual = 1 << 0, // Explicitly assigned contextual type, therefore not cacheable
Inferential = 1 << 1, // Inferential typing
SkipContextSensitive = 1 << 2, // Skip context sensitive function expressions
SkipGenericFunctions = 1 << 3, // Skip single signature generic functions
IsForSignatureHelp = 1 << 4, // Call resolution for purposes of signature help
RestBindingElement = 1 << 5, // Checking a type that is going to be used to determine the type of a rest binding element
// e.g. in `const { a, ...rest } = foo`, when checking the type of `foo` to determine the type of `rest`,
// we need to preserve generic types instead of substituting them for constraints
Normal = 0, // Normal type checking
Contextual = 1 << 0, // Explicitly assigned contextual type, therefore not cacheable
Inferential = 1 << 1, // Inferential typing
SkipContextSensitive = 1 << 2, // Skip context sensitive function expressions
SkipGenericFunctions = 1 << 3, // Skip single signature generic functions
IsForSignatureHelp = 1 << 4, // Call resolution for purposes of signature help
IsForStringLiteralArgumentCompletions = 1 << 5, // Do not infer from the argument currently being typed
RestBindingElement = 1 << 6, // Checking a type that is going to be used to determine the type of a rest binding element
// e.g. in `const { a, ...rest } = foo`, when checking the type of `foo` to determine the type of `rest`,
// we need to preserve generic types instead of substituting them for constraints
}

const enum SignatureCheckMode {
Expand Down Expand Up @@ -532,26 +533,10 @@ namespace ts {
if (!node) {
return undefined;
}
const containingCall = findAncestor(node, isCallLikeExpression);
const containingCallResolvedSignature = containingCall && getNodeLinks(containingCall).resolvedSignature;
if (contextFlags! & ContextFlags.Completions && containingCall) {
let toMarkSkip = node as Node;
do {
getNodeLinks(toMarkSkip).skipDirectInference = true;
toMarkSkip = toMarkSkip.parent;
} while (toMarkSkip && toMarkSkip !== containingCall);
getNodeLinks(containingCall).resolvedSignature = undefined;
}
const result = getContextualType(node, contextFlags);
if (contextFlags! & ContextFlags.Completions && containingCall) {
let toMarkSkip = node as Node;
do {
getNodeLinks(toMarkSkip).skipDirectInference = undefined;
toMarkSkip = toMarkSkip.parent;
} while (toMarkSkip && toMarkSkip !== containingCall);
getNodeLinks(containingCall).resolvedSignature = containingCallResolvedSignature;
if (contextFlags! & ContextFlags.Completions) {
return runWithInferenceBlockedFromSourceNode(node, () => getContextualType(node, contextFlags));
}
return result;
return getContextualType(node, contextFlags);
},
getContextualTypeForObjectLiteralElement: nodeIn => {
const node = getParseTreeNode(nodeIn, isObjectLiteralElementLike);
Expand All @@ -570,6 +555,8 @@ namespace ts {
getFullyQualifiedName,
getResolvedSignature: (node, candidatesOutArray, argumentCount) =>
getResolvedSignatureWorker(node, candidatesOutArray, argumentCount, CheckMode.Normal),
getResolvedSignatureForStringLiteralCompletions: (call, editingArgument, candidatesOutArray) =>
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 just add an @internal overload for getResolvedSignature that allows CheckMode to be passed in?

Copy link
Member Author

Choose a reason for hiding this comment

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

editingArgument is also needed, and argumentCount isn’t needed... and I like that the name implies the signature you’re going to get is in fact not at all the same signature you would get for normal checking purposes.

getResolvedSignatureWorker(call, candidatesOutArray, /*argumentCount*/ undefined, CheckMode.IsForStringLiteralArgumentCompletions, editingArgument),
getResolvedSignatureForSignatureHelp: (node, candidatesOutArray, argumentCount) =>
getResolvedSignatureWorker(node, candidatesOutArray, argumentCount, CheckMode.IsForSignatureHelp),
getExpandedParameters,
Expand Down Expand Up @@ -739,10 +726,36 @@ namespace ts {
getMemberOverrideModifierStatus,
};

function getResolvedSignatureWorker(nodeIn: CallLikeExpression, candidatesOutArray: Signature[] | undefined, argumentCount: number | undefined, checkMode: CheckMode): Signature | undefined {
function runWithInferenceBlockedFromSourceNode<T>(node: Node | undefined, fn: () => T): T {
const containingCall = findAncestor(node, isCallLikeExpression);
const containingCallResolvedSignature = containingCall && getNodeLinks(containingCall).resolvedSignature;
if (containingCall) {
let toMarkSkip = node!;
do {
getNodeLinks(toMarkSkip).skipDirectInference = true;
toMarkSkip = toMarkSkip.parent;
} while (toMarkSkip && toMarkSkip !== containingCall);
getNodeLinks(containingCall).resolvedSignature = undefined;
}
const result = fn();
if (containingCall) {
let toMarkSkip = node!;
do {
getNodeLinks(toMarkSkip).skipDirectInference = undefined;
toMarkSkip = toMarkSkip.parent;
} while (toMarkSkip && toMarkSkip !== containingCall);
getNodeLinks(containingCall).resolvedSignature = containingCallResolvedSignature;
}
return result;
}

function getResolvedSignatureWorker(nodeIn: CallLikeExpression, candidatesOutArray: Signature[] | undefined, argumentCount: number | undefined, checkMode: CheckMode, editingArgument?: Node): Signature | undefined {
const node = getParseTreeNode(nodeIn, isCallLikeExpression);
apparentArgumentCount = argumentCount;
const res = node ? getResolvedSignature(node, candidatesOutArray, checkMode) : undefined;
const res =
!node ? undefined :
editingArgument ? runWithInferenceBlockedFromSourceNode(editingArgument, () => getResolvedSignature(node, candidatesOutArray, checkMode)) :
getResolvedSignature(node, candidatesOutArray, checkMode);
apparentArgumentCount = undefined;
return res;
}
Expand Down Expand Up @@ -22662,7 +22675,7 @@ namespace ts {
const properties = getPropertiesOfObjectType(target);
for (const targetProp of properties) {
const sourceProp = getPropertyOfType(source, targetProp.escapedName);
if (sourceProp) {
if (sourceProp && !some(sourceProp.declarations, hasSkipDirectInferenceFlag)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe it would be nice to skip this at least in non-services calls, but it would be a lot of plumbing to get checkMode through. I can make a local in createTypeChecker if it seems worthwhile.

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure you could translate the CheckMode to an InferenceFlag if you'd like.

Copy link
Member Author

Choose a reason for hiding this comment

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

inferTypes doesn’t have access to the inference flags, so this doesn’t actually improve the plumbing situation 😕

Copy link
Member

Choose a reason for hiding this comment

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

A dummy InferencePriority that's unset by the start of inference then? 😅

inferFromTypes(getTypeOfSymbol(sourceProp), getTypeOfSymbol(targetProp));
}
}
Expand Down Expand Up @@ -29760,7 +29773,7 @@ namespace ts {

for (let i = 0; i < argCount; i++) {
const arg = args[i];
if (arg.kind !== SyntaxKind.OmittedExpression) {
if (arg.kind !== SyntaxKind.OmittedExpression && !(checkMode & CheckMode.IsForStringLiteralArgumentCompletions && hasSkipDirectInferenceFlag(arg))) {
const paramType = getTypeAtPosition(signature, i);
const argType = checkExpressionWithContextualType(arg, paramType, context, checkMode);
inferTypes(context.inferences, argType, paramType);
Expand Down Expand Up @@ -30500,7 +30513,7 @@ namespace ts {
}
}

return getCandidateForOverloadFailure(node, candidates, args, !!candidatesOutArray);
return getCandidateForOverloadFailure(node, candidates, args, !!candidatesOutArray, checkMode);

function addImplementationSuccessElaboration(failed: Signature, diagnostic: Diagnostic) {
const oldCandidatesForArgumentError = candidatesForArgumentError;
Expand Down Expand Up @@ -30614,14 +30627,15 @@ namespace ts {
candidates: Signature[],
args: readonly Expression[],
hasCandidatesOutArray: boolean,
checkMode: CheckMode,
): Signature {
Debug.assert(candidates.length > 0); // Else should not have called this.
checkNodeDeferred(node);
// Normally we will combine overloads. Skip this if they have type parameters since that's hard to combine.
// Don't do this if there is a `candidatesOutArray`,
// because then we want the chosen best candidate to be one of the overloads, not a combination.
return hasCandidatesOutArray || candidates.length === 1 || candidates.some(c => !!c.typeParameters)
? pickLongestCandidateSignature(node, candidates, args)
? pickLongestCandidateSignature(node, candidates, args, checkMode)
: createUnionOfSignaturesForOverloadFailure(candidates);
}

Expand Down Expand Up @@ -30675,7 +30689,7 @@ namespace ts {
return createSymbolWithType(first(sources), type);
}

function pickLongestCandidateSignature(node: CallLikeExpression, candidates: Signature[], args: readonly Expression[]): Signature {
function pickLongestCandidateSignature(node: CallLikeExpression, candidates: Signature[], args: readonly Expression[], checkMode: CheckMode): Signature {
// Pick the longest signature. This way we can get a contextual type for cases like:
// declare function f(a: { xa: number; xb: number; }, b: number);
// f({ |
Expand All @@ -30692,7 +30706,7 @@ namespace ts {
const typeArgumentNodes: readonly TypeNode[] | undefined = callLikeExpressionMayHaveTypeArguments(node) ? node.typeArguments : undefined;
const instantiated = typeArgumentNodes
? createSignatureInstantiation(candidate, getTypeArgumentsFromNodes(typeArgumentNodes, typeParameters, isInJSFile(node)))
: inferSignatureInstantiationForOverloadFailure(node, typeParameters, candidate, args);
: inferSignatureInstantiationForOverloadFailure(node, typeParameters, candidate, args, checkMode);
candidates[bestIndex] = instantiated;
return instantiated;
}
Expand All @@ -30708,9 +30722,9 @@ namespace ts {
return typeArguments;
}

function inferSignatureInstantiationForOverloadFailure(node: CallLikeExpression, typeParameters: readonly TypeParameter[], candidate: Signature, args: readonly Expression[]): Signature {
function inferSignatureInstantiationForOverloadFailure(node: CallLikeExpression, typeParameters: readonly TypeParameter[], candidate: Signature, args: readonly Expression[], checkMode: CheckMode): Signature {
const inferenceContext = createInferenceContext(typeParameters, candidate, /*flags*/ isInJSFile(node) ? InferenceFlags.AnyDefault : InferenceFlags.None);
const typeArgumentTypes = inferTypeArguments(node, candidate, args, CheckMode.SkipContextSensitive | CheckMode.SkipGenericFunctions, inferenceContext);
const typeArgumentTypes = inferTypeArguments(node, candidate, args, checkMode | CheckMode.SkipContextSensitive | CheckMode.SkipGenericFunctions, inferenceContext);
return createSignatureInstantiation(candidate, typeArgumentTypes);
}

Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4279,6 +4279,7 @@ namespace ts {
*/
getResolvedSignature(node: CallLikeExpression, candidatesOutArray?: Signature[], argumentCount?: number): Signature | undefined;
/* @internal */ getResolvedSignatureForSignatureHelp(node: CallLikeExpression, candidatesOutArray?: Signature[], argumentCount?: number): Signature | undefined;
/* @internal */ getResolvedSignatureForStringLiteralCompletions(call: CallLikeExpression, editingArgument: Node, candidatesOutArray: Signature[]): Signature | undefined;
/* @internal */ getExpandedParameters(sig: Signature): readonly (readonly Symbol[])[];
/* @internal */ hasEffectiveRestParameter(sig: Signature): boolean;
/* @internal */ containsArgumentsReference(declaration: SignatureDeclaration): boolean;
Expand Down
10 changes: 5 additions & 5 deletions src/harness/fourslashImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ namespace FourSlash {
// The position of the end of the current selection, or -1 if nothing is selected
public selectionEnd = -1;

public lastKnownMarker = "";
public lastKnownMarker: string | undefined;

// The file that's currently 'opened'
public activeFile!: FourSlashFile;
Expand Down Expand Up @@ -400,7 +400,7 @@ namespace FourSlash {
continue;
}
const memo = Utils.memoize(
(_version: number, _active: string, _caret: number, _selectEnd: number, _marker: string, ...args: any[]) => (ls[key] as Function)(...args),
(_version: number, _active: string, _caret: number, _selectEnd: number, _marker: string | undefined, ...args: any[]) => (ls[key] as Function)(...args),
(...args) => args.map(a => a && typeof a === "object" ? JSON.stringify(a) : a).join("|,|")
);
proxy[key] = (...args: any[]) => memo(
Expand Down Expand Up @@ -540,8 +540,8 @@ namespace FourSlash {
}

private messageAtLastKnownMarker(message: string) {
const locationDescription = this.lastKnownMarker ? this.lastKnownMarker : this.getLineColStringAtPosition(this.currentCaretPosition);
return `At ${locationDescription}: ${message}`;
const locationDescription = this.lastKnownMarker !== undefined ? this.lastKnownMarker : this.getLineColStringAtPosition(this.currentCaretPosition);
return `At marker '${locationDescription}': ${message}`;
}

private assertionMessageAtLastKnownMarker(msg: string) {
Expand Down Expand Up @@ -864,7 +864,7 @@ namespace FourSlash {
else {
for (const marker of toArray(options.marker)) {
this.goToMarker(marker);
this.verifyCompletionsWorker(options);
this.verifyCompletionsWorker({ ...options, marker });
}
}
}
Expand Down
19 changes: 13 additions & 6 deletions src/services/stringCompletions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,12 +210,13 @@ namespace ts.Completions.StringCompletions {

case SyntaxKind.CallExpression:
case SyntaxKind.NewExpression:
case SyntaxKind.JsxAttribute:
if (!isRequireCallArgument(node) && !isImportCall(parent)) {
const argumentInfo = SignatureHelp.getArgumentInfoForCompletions(node, position, sourceFile);
const argumentInfo = SignatureHelp.getArgumentInfoForCompletions(parent.kind === SyntaxKind.JsxAttribute ? parent.parent : node, position, sourceFile);
// Get string literal completions from specialized signatures of the target
// i.e. declare function f(a: 'A');
// f("/*completion position*/")
return argumentInfo ? getStringLiteralCompletionsFromSignature(argumentInfo, typeChecker) : fromContextualType();
return argumentInfo ? getStringLiteralCompletionsFromSignature(argumentInfo.invocation, node, argumentInfo, typeChecker) : fromContextualType();
}
// falls through (is `require("")` or `require(""` or `import("")`)

Expand Down Expand Up @@ -257,15 +258,21 @@ namespace ts.Completions.StringCompletions {
type !== current && isLiteralTypeNode(type) && isStringLiteral(type.literal) ? type.literal.text : undefined);
}

function getStringLiteralCompletionsFromSignature(argumentInfo: SignatureHelp.ArgumentInfoForCompletions, checker: TypeChecker): StringLiteralCompletionsFromTypes {
function getStringLiteralCompletionsFromSignature(call: CallLikeExpression, arg: StringLiteralLike, argumentInfo: SignatureHelp.ArgumentInfoForCompletions, checker: TypeChecker): StringLiteralCompletionsFromTypes {
let isNewIdentifier = false;

const uniques = new Map<string, true>();
const candidates: Signature[] = [];
checker.getResolvedSignature(argumentInfo.invocation, candidates, argumentInfo.argumentCount);
const editingArgument = isJsxOpeningLikeElement(call) ? Debug.checkDefined(findAncestor(arg.parent, isJsxAttribute)) : arg;
checker.getResolvedSignatureForStringLiteralCompletions(call, editingArgument, candidates);
const types = flatMap(candidates, candidate => {
if (!signatureHasRestParameter(candidate) && argumentInfo.argumentCount > candidate.parameters.length) return;
const type = candidate.getTypeParameterAtPosition(argumentInfo.argumentIndex);
let type = candidate.getTypeParameterAtPosition(argumentInfo.argumentIndex);
if (isJsxOpeningLikeElement(call)) {
const propType = checker.getTypeOfPropertyOfType(type, (editingArgument as JsxAttribute).name.text);
if (propType) {
type = propType;
}
}
isNewIdentifier = isNewIdentifier || !!(type.flags & TypeFlags.String);
return getStringLiteralTypes(type, uniques);
});
Expand Down
23 changes: 23 additions & 0 deletions tests/cases/fourslash/completionsLiteralOverload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/// <reference path="fourslash.ts" />

// @allowJs: true

// @Filename: /a.tsx
//// interface Events {
//// "": any;
//// drag: any;
//// dragenter: any;
//// }
//// declare function addListener<K extends keyof Events>(type: K, listener: (ev: Events[K]) => any): void;
////
//// declare function ListenerComponent<K extends keyof Events>(props: { type: K, onWhatever: (ev: Events[K]) => void }): JSX.Element;
////
//// addListener("/*ts*/");
//// (<ListenerComponent type="/*tsx*/" />);

// @Filename: /b.js
//// addListener("/*js*/");

verify.completions({ marker: ["ts", "tsx", "js"], exact: ["", "drag", "dragenter"] });
edit.insert("drag");
verify.completions({ marker: ["ts", "tsx", "js"], exact: ["", "drag", "dragenter"] });
Copy link
Member

Choose a reason for hiding this comment

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

Should we also have a test where drag is already entered and we check that we don't output ""? I mean, I guess we should just so we know what our current behavior is, I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do output "", but editors filter it out. This is intentional, because it means if the user backspaces, the editor doesn’t have to re-request completions given a different prefix.

Copy link
Member Author

Choose a reason for hiding this comment

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

(The test you’re suggesting is already what’s happening on this line)

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, ok, I misread - thanks!