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

Jump to bottom

Fix import statement completions followed by interface declaration #50350

Merged
merged 4 commits into from Sep 20, 2022
Merged

Fix import statement completions followed by interface declaration #50350

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2d3c426
Fix import statement completions followed by interface declaration
andrewbranch Aug 17, 2022
ebb1138
Fix stuff
andrewbranch Aug 18, 2022
003320e
Linty
andrewbranch Aug 18, 2022
c030dbf
Fix when named imports is missing closing brace
andrewbranch Sep 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Expand Up @@ -41175,7 +41175,7 @@ namespace ts {
}
else {
Debug.assert(node.kind !== SyntaxKind.VariableDeclaration);
const importDeclaration = findAncestor(node, or(isImportDeclaration, isImportEqualsDeclaration)) as ImportDeclaration | ImportEqualsDeclaration | undefined;
const importDeclaration = findAncestor(node, or(isImportDeclaration, isImportEqualsDeclaration));
const moduleSpecifier = (importDeclaration && tryGetModuleSpecifierFromDeclaration(importDeclaration)?.text) ?? "...";
const importedIdentifier = unescapeLeadingUnderscores(isIdentifier(errorNode) ? errorNode.escapedText : symbol.escapedName);
error(
Expand Down
3 changes: 3 additions & 0 deletions src/compiler/core.ts
View file
Open in desktop
Expand Up @@ -2371,6 +2371,9 @@ namespace ts {
return (arg: T) => f(arg) && g(arg);
}

export function or<P, R1 extends P, R2 extends P>(f1: (p1: P) => p1 is R1, f2: (p2: P) => p2 is R2): (p: P) => p is R1 | R2;
export function or<P, R1 extends P, R2 extends P, R3 extends P>(f1: (p1: P) => p1 is R1, f2: (p2: P) => p2 is R2, f3: (p3: P) => p3 is R3): (p: P) => p is R1 | R2 | R3;
export function or<T extends unknown[], U>(...fs: ((...args: T) => U)[]): (...args: T) => U;
export function or<T extends unknown[], U>(...fs: ((...args: T) => U)[]): (...args: T) => U {
return (...args) => {
let lastResult: U;
Expand Down
110 changes: 68 additions & 42 deletions src/services/completions.ts
View file
Open in desktop
Expand Up @@ -492,7 +492,7 @@ namespace ts.Completions {
isTypeOnlyLocation,
isJsxIdentifierExpected,
isRightOfOpenTag,
importCompletionNode,
importStatementCompletion,
insideJsDocTagTypeExpression,
symbolToSortTextMap: symbolToSortTextMap,
hasUnresolvedAutoImports,
Expand Down Expand Up @@ -530,7 +530,7 @@ namespace ts.Completions {
propertyAccessToConvert,
isJsxIdentifierExpected,
isJsxInitializer,
importCompletionNode,
importStatementCompletion,
recommendedCompletion,
symbolToOriginInfoMap,
symbolToSortTextMap,
Expand Down Expand Up @@ -685,7 +685,7 @@ namespace ts.Completions {
recommendedCompletion: Symbol | undefined,
propertyAccessToConvert: PropertyAccessExpression | undefined,
isJsxInitializer: IsJsxInitializer | undefined,
importCompletionNode: Node | undefined,
importStatementCompletion: ImportStatementCompletionInfo | undefined,
useSemicolons: boolean,
options: CompilerOptions,
preferences: UserPreferences,
Expand Down Expand Up @@ -751,8 +751,8 @@ namespace ts.Completions {

if (originIsResolvedExport(origin)) {
sourceDisplay = [textPart(origin.moduleSpecifier)];
if (importCompletionNode) {
({ insertText, replacementSpan } = getInsertTextAndReplacementSpanForImportCompletion(name, importCompletionNode, contextToken, origin, useSemicolons, options, preferences));
if (importStatementCompletion) {
({ insertText, replacementSpan } = getInsertTextAndReplacementSpanForImportCompletion(name, importStatementCompletion, origin, useSemicolons, sourceFile, options, preferences));
isSnippet = preferences.includeCompletionsWithSnippetText ? true : undefined;
}
}
Expand Down Expand Up @@ -816,7 +816,7 @@ namespace ts.Completions {

if (originIsExport(origin) || originIsResolvedExport(origin)) {
data = originToCompletionEntryData(origin);
hasAction = !importCompletionNode;
hasAction = !importStatementCompletion;
}

// TODO(drosen): Right now we just permit *all* semantic meanings when calling
Expand All @@ -841,7 +841,7 @@ namespace ts.Completions {
labelDetails,
isSnippet,
isPackageJsonImport: originIsPackageJsonImport(origin) || undefined,
isImportStatementCompletion: !!importCompletionNode || undefined,
isImportStatementCompletion: !!importStatementCompletion || undefined,
data,
};
}
Expand Down Expand Up @@ -1338,19 +1338,17 @@ namespace ts.Completions {
return unresolvedOrigin;
}

function getInsertTextAndReplacementSpanForImportCompletion(name: string, importCompletionNode: Node, contextToken: Node | undefined, origin: SymbolOriginInfoResolvedExport, useSemicolons: boolean, options: CompilerOptions, preferences: UserPreferences) {
const sourceFile = importCompletionNode.getSourceFile();
const replacementSpan = createTextSpanFromNode(findAncestor(importCompletionNode, or(isImportDeclaration, isImportEqualsDeclaration)) || importCompletionNode, sourceFile);
function getInsertTextAndReplacementSpanForImportCompletion(name: string, importStatementCompletion: ImportStatementCompletionInfo, origin: SymbolOriginInfoResolvedExport, useSemicolons: boolean, sourceFile: SourceFile, options: CompilerOptions, preferences: UserPreferences) {
const replacementSpan = importStatementCompletion.replacementSpan;
const quotedModuleSpecifier = quote(sourceFile, preferences, origin.moduleSpecifier);
const exportKind =
origin.isDefaultExport ? ExportKind.Default :
origin.exportName === InternalSymbolName.ExportEquals ? ExportKind.ExportEquals :
ExportKind.Named;
const tabStop = preferences.includeCompletionsWithSnippetText ? "$1" : "";
const importKind = codefix.getImportKind(sourceFile, exportKind, options, /*forceImportKeyword*/ true);
const isTopLevelTypeOnly = tryCast(importCompletionNode, isImportDeclaration)?.importClause?.isTypeOnly || tryCast(importCompletionNode, isImportEqualsDeclaration)?.isTypeOnly;
const isImportSpecifierTypeOnly = couldBeTypeOnlyImportSpecifier(importCompletionNode, contextToken);
const topLevelTypeOnlyText = isTopLevelTypeOnly ? ` ${tokenToString(SyntaxKind.TypeKeyword)} ` : " ";
const isImportSpecifierTypeOnly = importStatementCompletion.couldBeTypeOnlyImportSpecifier;
const topLevelTypeOnlyText = importStatementCompletion.isTopLevelTypeOnly ? ` ${tokenToString(SyntaxKind.TypeKeyword)} ` : " ";
const importSpecifierTypeOnlyText = isImportSpecifierTypeOnly ? `${tokenToString(SyntaxKind.TypeKeyword)} ` : "";
const suffix = useSemicolons ? ";" : "";
switch (importKind) {
Expand Down Expand Up @@ -1408,7 +1406,7 @@ namespace ts.Completions {
propertyAccessToConvert?: PropertyAccessExpression,
jsxIdentifierExpected?: boolean,
isJsxInitializer?: IsJsxInitializer,
importCompletionNode?: Node,
importStatementCompletion?: ImportStatementCompletionInfo,
recommendedCompletion?: Symbol,
symbolToOriginInfoMap?: SymbolOriginInfoMap,
symbolToSortTextMap?: SymbolSortTextMap,
Expand Down Expand Up @@ -1450,7 +1448,7 @@ namespace ts.Completions {
recommendedCompletion,
propertyAccessToConvert,
isJsxInitializer,
importCompletionNode,
importStatementCompletion,
useSemicolons,
compilerOptions,
preferences,
Expand Down Expand Up @@ -1724,7 +1722,7 @@ namespace ts.Completions {
source: string | undefined,
): CodeActionsAndSourceDisplay {
if (data?.moduleSpecifier) {
if (previousToken && getImportStatementCompletionInfo(contextToken || previousToken).replacementNode) {
if (previousToken && getImportStatementCompletionInfo(contextToken || previousToken).replacementSpan) {
// Import statement completion: 'import c|'
return { codeActions: undefined, sourceDisplay: [textPart(data.moduleSpecifier)] };
}
Expand Down Expand Up @@ -1829,7 +1827,7 @@ namespace ts.Completions {
/** In JSX tag name and attribute names, identifiers like "my-tag" or "aria-name" is valid identifier. */
readonly isJsxIdentifierExpected: boolean;
readonly isRightOfOpenTag: boolean;
readonly importCompletionNode?: Node;
readonly importStatementCompletion?: ImportStatementCompletionInfo;
readonly hasUnresolvedAutoImports?: boolean;
readonly flags: CompletionInfoFlags;
}
Expand Down Expand Up @@ -2012,35 +2010,35 @@ namespace ts.Completions {
let isStartingCloseTag = false;
let isJsxInitializer: IsJsxInitializer = false;
let isJsxIdentifierExpected = false;
let importCompletionNode: Node | undefined;
let importStatementCompletion: ImportStatementCompletionInfo | undefined;
let location = getTouchingPropertyName(sourceFile, position);
let keywordFilters = KeywordCompletionFilters.None;
let isNewIdentifierLocation = false;
let flags = CompletionInfoFlags.None;

if (contextToken) {
const importStatementCompletion = getImportStatementCompletionInfo(contextToken);
isNewIdentifierLocation = importStatementCompletion.isNewIdentifierLocation;
if (importStatementCompletion.keywordCompletion) {
if (importStatementCompletion.isKeywordOnlyCompletion) {
const importStatementCompletionInfo = getImportStatementCompletionInfo(contextToken);
if (importStatementCompletionInfo.keywordCompletion) {
if (importStatementCompletionInfo.isKeywordOnlyCompletion) {
return {
kind: CompletionDataKind.Keywords,
keywordCompletions: [keywordToCompletionEntry(importStatementCompletion.keywordCompletion)],
isNewIdentifierLocation,
keywordCompletions: [keywordToCompletionEntry(importStatementCompletionInfo.keywordCompletion)],
isNewIdentifierLocation: importStatementCompletionInfo.isNewIdentifierLocation,
};
}
keywordFilters = keywordFiltersFromSyntaxKind(importStatementCompletion.keywordCompletion);
keywordFilters = keywordFiltersFromSyntaxKind(importStatementCompletionInfo.keywordCompletion);
}
if (importStatementCompletion.replacementNode && preferences.includeCompletionsForImportStatements && preferences.includeCompletionsWithInsertText) {
if (importStatementCompletionInfo.replacementSpan && preferences.includeCompletionsForImportStatements && preferences.includeCompletionsWithInsertText) {
// Import statement completions use `insertText`, and also require the `data` property of `CompletionEntryIdentifier`
// added in TypeScript 4.3 to be sent back from the client during `getCompletionEntryDetails`. Since this feature
// is not backward compatible with older clients, the language service defaults to disabling it, allowing newer clients
// to opt in with the `includeCompletionsForImportStatements` user preference.
importCompletionNode = importStatementCompletion.replacementNode;
flags |= CompletionInfoFlags.IsImportStatementCompletion;
importStatementCompletion = importStatementCompletionInfo;
isNewIdentifierLocation = importStatementCompletionInfo.isNewIdentifierLocation;
}
// Bail out if this is a known invalid completion location
if (!importCompletionNode && isCompletionListBlocker(contextToken)) {
if (!importStatementCompletionInfo.replacementSpan && isCompletionListBlocker(contextToken)) {
log("Returning an empty list because completion was requested in an invalid position.");
return keywordFilters
? keywordCompletionData(keywordFilters, isJsOnlyLocation, isNewIdentifierDefinitionLocation())
Expand Down Expand Up @@ -2087,7 +2085,7 @@ namespace ts.Completions {
return undefined;
}
}
else if (!importCompletionNode && sourceFile.languageVariant === LanguageVariant.JSX) {
else if (!importStatementCompletion) {
// <UI.Test /* completion position */ />
// If the tagname is a property access expression, we will then walk up to the top most of property access expression.
// Then, try to get a JSX container and its associated attributes type.
Expand Down Expand Up @@ -2244,7 +2242,7 @@ namespace ts.Completions {
isTypeOnlyLocation,
isJsxIdentifierExpected,
isRightOfOpenTag,
importCompletionNode,
importStatementCompletion,
hasUnresolvedAutoImports,
flags,
};
Expand Down Expand Up @@ -2520,7 +2518,7 @@ namespace ts.Completions {
}

function tryGetImportCompletionSymbols(): GlobalsSearch {
if (!importCompletionNode) return GlobalsSearch.Continue;
if (!importStatementCompletion) return GlobalsSearch.Continue;
isNewIdentifierLocation = true;
collectAutoImports();
return GlobalsSearch.Success;
Expand Down Expand Up @@ -2609,7 +2607,7 @@ namespace ts.Completions {

function shouldOfferImportCompletions(): boolean {
// If already typing an import statement, provide completions for it.
if (importCompletionNode) return true;
if (importStatementCompletion) return true;
// If current completion is for non-contextual Object literal shortahands, ignore auto-import symbols
if (isNonContextualObjectLiteral) return false;
// If not already a module, must have modules enabled.
Expand All @@ -2636,7 +2634,7 @@ namespace ts.Completions {

function isTypeOnlyCompletion(): boolean {
return insideJsDocTagTypeExpression
|| !!importCompletionNode && isTypeOnlyImportOrExportDeclaration(location.parent)
|| !!importStatementCompletion && isTypeOnlyImportOrExportDeclaration(location.parent)
|| !isContextTokenValueLocation(contextToken) &&
(isPossiblyTypeArgumentPosition(contextToken, sourceFile, typeChecker)
|| isPartOfTypeNode(location)
Expand Down Expand Up @@ -2690,8 +2688,7 @@ namespace ts.Completions {
flags |= CompletionInfoFlags.MayIncludeAutoImports;
// import { type | -> token text should be blank
const isAfterTypeOnlyImportSpecifierModifier = previousToken === contextToken
&& importCompletionNode
&& couldBeTypeOnlyImportSpecifier(importCompletionNode, contextToken);
&& importStatementCompletion;

const lowerCaseTokenText =
isAfterTypeOnlyImportSpecifierModifier ? "" :
Expand All @@ -2709,7 +2706,7 @@ namespace ts.Completions {
program,
position,
preferences,
!!importCompletionNode,
!!importStatementCompletion,
isValidTypeOnlyAliasUseSite(location),
context => {
exportInfo.search(
Expand All @@ -2718,7 +2715,7 @@ namespace ts.Completions {
(symbolName, targetFlags) => {
if (!isIdentifierText(symbolName, getEmitScriptTarget(host.getCompilationSettings()))) return false;
if (!detailsEntryId && isStringANonContextualKeyword(symbolName)) return false;
if (!isTypeOnlyLocation && !importCompletionNode && !(targetFlags & SymbolFlags.Value)) return false;
if (!isTypeOnlyLocation && !importStatementCompletion && !(targetFlags & SymbolFlags.Value)) return false;
if (isTypeOnlyLocation && !(targetFlags & (SymbolFlags.Module | SymbolFlags.Type))) return false;
// Do not try to auto-import something with a lowercase first letter for a JSX tag
const firstChar = symbolName.charCodeAt(0);
Expand Down Expand Up @@ -2812,7 +2809,7 @@ namespace ts.Completions {
return;
}
symbolToOriginInfoMap[symbols.length] = origin;
symbolToSortTextMap[symbolId] = importCompletionNode ? SortText.LocationPriority : SortText.AutoImportSuggestions;
symbolToSortTextMap[symbolId] = importStatementCompletion ? SortText.LocationPriority : SortText.AutoImportSuggestions;
symbols.push(symbol);
}

Expand Down Expand Up @@ -4241,7 +4238,9 @@ namespace ts.Completions {
isKeywordOnlyCompletion: boolean;
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
keywordCompletion: TokenSyntaxKind | undefined;
isNewIdentifierLocation: boolean;
replacementNode: ImportEqualsDeclaration | ImportDeclaration | ImportSpecifier | Token<SyntaxKind.ImportKeyword> | undefined;
isTopLevelTypeOnly: boolean;
couldBeTypeOnlyImportSpecifier: boolean;
replacementSpan: TextSpan | undefined;
}

function getImportStatementCompletionInfo(contextToken: Node): ImportStatementCompletionInfo {
Expand All @@ -4252,9 +4251,9 @@ namespace ts.Completions {
isKeywordOnlyCompletion,
keywordCompletion,
isNewIdentifierLocation: !!(candidate || keywordCompletion === SyntaxKind.TypeKeyword),
replacementNode: candidate && rangeIsOnSingleLine(candidate, candidate.getSourceFile())
? candidate
: undefined
isTopLevelTypeOnly: !!tryCast(candidate, isImportDeclaration)?.importClause?.isTypeOnly || !!tryCast(candidate, isImportEqualsDeclaration)?.isTypeOnly,
couldBeTypeOnlyImportSpecifier: !!candidate && couldBeTypeOnlyImportSpecifier(candidate, contextToken),
replacementSpan: getSingleLineReplacementSpanForImportCompletionNode(candidate),
};

function getCandidate() {
Expand Down Expand Up @@ -4301,6 +4300,33 @@ namespace ts.Completions {
}
}

function getSingleLineReplacementSpanForImportCompletionNode(node: ImportDeclaration | ImportEqualsDeclaration | ImportSpecifier | Token<SyntaxKind.ImportKeyword> | undefined) {
if (!node) return undefined;
const top = findAncestor(node, or(isImportDeclaration, isImportEqualsDeclaration)) ?? node;
const sourceFile = top.getSourceFile();
if (rangeIsOnSingleLine(top, sourceFile)) {
return createTextSpanFromNode(top, sourceFile);
}
// ImportKeyword was necessarily on one line; ImportSpecifier was necessarily parented in an ImportDeclaration
Debug.assert(top.kind !== SyntaxKind.ImportKeyword && top.kind !== SyntaxKind.ImportSpecifier);
const withoutModuleSpecifier: TextRange = {
pos: top.getFirstToken()!.getStart(),
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
end: top.kind === SyntaxKind.ImportDeclaration ? top.moduleSpecifier.pos : top.moduleReference.pos,
};
// The module specifier/reference was previously found to be missing, empty, or
// not a string literal - in this last case, it's likely that statement on a following
// line was parsed as the module specifier of a partially-typed import, e.g.
// import Foo|
// interface Blah {}
// This appears to be a multiline-import, and editors can't replace multiple lines.
Copy link
Member

Choose a reason for hiding this comment

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

editors can't replace multiple lines

Is this detail actually relevant to the change? It sounds like even if editors could do that, you would still want to assume that the next line is a separate statement and recover appropriately, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, fair; but it explains why the failure mode is “no completions” rather than “overwriting later declarations.”

// But if everything but the "module specifier" is on one line, by this point we can
// assume that the "module specifier" is actually just another statement, and return
// the single-line range of the import excluding that probable statement.
if (rangeIsOnSingleLine(withoutModuleSpecifier, sourceFile)) {
return createTextSpanFromRange(withoutModuleSpecifier);
}
}

function couldBeTypeOnlyImportSpecifier(importSpecifier: Node, contextToken: Node | undefined): importSpecifier is ImportSpecifier {
return isImportSpecifier(importSpecifier)
&& (importSpecifier.isTypeOnly || contextToken === importSpecifier.name && isTypeKeywordTokenOrIdentifier(contextToken));
Expand Down