Skip to content

Fixed some string literal argument completions depending on resolved signature #53996

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
merged 4 commits into from
Jun 13, 2023

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Apr 24, 2023

fixes #53997

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Apr 24, 2023
@@ -32436,7 +32440,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {

for (let i = 0; i < argCount; i++) {
const arg = args[i];
if (arg.kind !== SyntaxKind.OmittedExpression && !(checkMode & CheckMode.IsForStringLiteralArgumentCompletions && hasSkipDirectInferenceFlag(arg))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is quite nice (IMHO) because it eliminates the need for this special CheckMode almost entirely (in fact, the first commit in this PR does exactly that - it just doesn't fix the issue).

With this we can fully rely just on "node blocking" to alter the default inference behavior for the completion purposes.

@@ -25198,7 +25202,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const constraint = getConstraintOfTypeParameter(inference.typeParameter);
if (constraint) {
const instantiatedConstraint = instantiateType(constraint, context.nonFixingMapper);
if (!inferredType || !context.compareTypes(inferredType, getTypeWithThisArgument(instantiatedConstraint, inferredType))) {
if (!inferredType || inferredType === wildcardType || !context.compareTypes(inferredType, getTypeWithThisArgument(instantiatedConstraint, inferredType))) {
Copy link
Contributor Author

@Andarist Andarist Apr 24, 2023

Choose a reason for hiding this comment

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

This change might be best reviewed in isolation by reviewing the first commit: 7ab8679

@@ -33318,7 +33321,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// If one or more context sensitive arguments were excluded, we start including
// them now (and keeping do so for any subsequent candidates) and perform a second
// round of type inference and applicability checking for this particular candidate.
argCheckMode = checkMode & CheckMode.IsForStringLiteralArgumentCompletions;
argCheckMode = CheckMode.Normal;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks to the introduced changes this is just a revert of another PR: https://github.com/microsoft/TypeScript/pull/48811/files . The patch from that other PR is not needed anymore since we now rely on wildcardType returned by checkExpressionWorker for blocked string literals

@@ -387,7 +388,7 @@ function getStringLiteralCompletionEntries(sourceFile: SourceFile, node: StringL
// Get string literal completions from specialized signatures of the target
// i.e. declare function f(a: 'A');
// f("/*completion position*/")
return argumentInfo && getStringLiteralCompletionsFromSignature(argumentInfo.invocation, node, argumentInfo, typeChecker) || fromContextualType();
return argumentInfo && (getStringLiteralCompletionsFromSignature(argumentInfo.invocation, node, argumentInfo, typeChecker) || getStringLiteralCompletionsFromSignature(argumentInfo.invocation, node, argumentInfo, typeChecker, CheckMode.Normal)) || fromContextualType();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This essentially implements the same "double request" strategy as the one introduced in #52875 (and later also used in #53481 and #53554 )

@@ -1657,8 +1657,12 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
getFullyQualifiedName,
getResolvedSignature: (node, candidatesOutArray, argumentCount) =>
getResolvedSignatureWorker(node, candidatesOutArray, argumentCount, CheckMode.Normal),
getResolvedSignatureForStringLiteralCompletions: (call, editingArgument, candidatesOutArray) =>
runWithInferenceBlockedFromSourceNode(editingArgument, () => getResolvedSignatureWorker(call, candidatesOutArray, /*argumentCount*/ undefined, CheckMode.IsForStringLiteralArgumentCompletions)),
getResolvedSignatureForStringLiteralCompletions: (call, editingArgument, candidatesOutArray, checkMode = CheckMode.IsForStringLiteralArgumentCompletions) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a second thought... this checkMode parameter could just be a boolean flag or something. We don't need this special CheckMode.IsForStringLiteralArgumentCompletions for anything now - it's only used to toggle the "inference blocking" behavior in this very function here. This is also reflected in the fact that I remove it from the checkMode that is passed to getResolvedSignatureWorker

//// }
//// const parse = <def>(def: validate<def>) => def
//// const shallowExpression = parse("foo|/*ts*/")
//// const nestedExpression = parse({ prop: "foo|/*ts2*/" })
Copy link
Contributor Author

@Andarist Andarist Apr 24, 2023

Choose a reason for hiding this comment

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

it's worth pointing out here that this nestedExpression was already working OK, the problem was in the shallowExpression's case as that was using a completely different part of the algorithm (getStringLiteralCompletionsFromSignature, and not getContextualType~)

@sandersn sandersn requested a review from andrewbranch May 2, 2023 23:02
@typescript-bot typescript-bot removed the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 2, 2023
@sandersn sandersn requested a review from navya9singh May 2, 2023 23:02
@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label May 2, 2023
@jakebailey
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 3, 2023

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 322f6cd. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 3, 2023

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/153805/artifacts?artifactName=tgz&fileId=E3D26348E1B679191216EF370BA7D1DCABCE3C70282A566490FC335882A4992C02&fileName=/typescript-5.1.0-insiders.20230503.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.1.0-pr-53996-2".;

…tion

# Conflicts:
#	src/services/stringCompletions.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Validated completions available for string props are not available for shallow strings
5 participants