Skip to content

Commit

Permalink
fix(43796): Sort @deprecated completions lower than others (#43880)
Browse files Browse the repository at this point in the history
* fix(43796): sort deprecated completions lower than others

* Update src/services/completions.ts

Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>
  • Loading branch information
a-tarasyuk and DanielRosenwasser committed May 18, 2021
1 parent db01e84 commit 3f9e724
Show file tree
Hide file tree
Showing 14 changed files with 255 additions and 46 deletions.
83 changes: 55 additions & 28 deletions src/services/completions.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/* @internal */
namespace ts.Completions {
export type Log = (message: string) => void;

export enum SortText {
LocalDeclarationPriority = "0",
LocationPriority = "1",
Expand All @@ -8,9 +10,28 @@ namespace ts.Completions {
SuggestedClassMembers = "4",
GlobalsOrKeywords = "5",
AutoImportSuggestions = "6",
JavascriptIdentifiers = "7"
JavascriptIdentifiers = "7",
DeprecatedLocalDeclarationPriority = "8",
DeprecatedLocationPriority = "9",
DeprecatedOptionalMember = "10",
DeprecatedMemberDeclaredBySpreadAssignment = "11",
DeprecatedSuggestedClassMembers = "12",
DeprecatedGlobalsOrKeywords = "13",
DeprecatedAutoImportSuggestions = "14"
}

enum SortTextId {
LocalDeclarationPriority,
LocationPriority,
OptionalMember,
MemberDeclaredBySpreadAssignment,
SuggestedClassMembers,
GlobalsOrKeywords,
AutoImportSuggestions
}
export type Log = (message: string) => void;

// for JavaScript identifiers since they are preferred over deprecated symbols
const DeprecatedSortTextStart = SortTextId.AutoImportSuggestions + 2;

/**
* Special values for `CompletionInfo['source']` used to disambiguate
Expand Down Expand Up @@ -105,8 +126,8 @@ namespace ts.Completions {
*/
type SymbolOriginInfoMap = Record<number, SymbolOriginInfo>;

/** Map from symbol id -> SortText. */
type SymbolSortTextMap = (SortText | undefined)[];
/** Map from symbol id -> SortTextId. */
type SymbolSortTextIdMap = (SortTextId | undefined)[];

const enum KeywordCompletionFilters {
None, // No keywords
Expand Down Expand Up @@ -221,7 +242,7 @@ namespace ts.Completions {
isJsxIdentifierExpected,
importCompletionNode,
insideJsDocTagTypeExpression,
symbolToSortTextMap,
symbolToSortTextIdMap,
} = completionData;

// Verify if the file is JSX language variant
Expand Down Expand Up @@ -254,7 +275,7 @@ namespace ts.Completions {
importCompletionNode,
recommendedCompletion,
symbolToOriginInfoMap,
symbolToSortTextMap
symbolToSortTextIdMap
);
getJSCompletionEntries(sourceFile, location.pos, uniqueNames, compilerOptions.target!, entries); // TODO: GH#18217
}
Expand Down Expand Up @@ -282,7 +303,7 @@ namespace ts.Completions {
importCompletionNode,
recommendedCompletion,
symbolToOriginInfoMap,
symbolToSortTextMap
symbolToSortTextIdMap
);
}

Expand Down Expand Up @@ -582,7 +603,7 @@ namespace ts.Completions {
importCompletionNode?: Node,
recommendedCompletion?: Symbol,
symbolToOriginInfoMap?: SymbolOriginInfoMap,
symbolToSortTextMap?: SymbolSortTextMap,
symbolToSortTextIdMap?: SymbolSortTextIdMap,
): UniqueNameSet {
const start = timestamp();
const variableDeclaration = getVariableDeclaration(location);
Expand All @@ -596,14 +617,16 @@ namespace ts.Completions {
const symbol = symbols[i];
const origin = symbolToOriginInfoMap?.[i];
const info = getCompletionEntryDisplayNameForSymbol(symbol, target, origin, kind, !!jsxIdentifierExpected);
if (!info || uniques.get(info.name) || kind === CompletionKind.Global && symbolToSortTextMap && !shouldIncludeSymbol(symbol, symbolToSortTextMap)) {
if (!info || uniques.get(info.name) || kind === CompletionKind.Global && symbolToSortTextIdMap && !shouldIncludeSymbol(symbol, symbolToSortTextIdMap)) {
continue;
}

const { name, needsConvertPropertyAccess } = info;
const sortTextId = symbolToSortTextIdMap?.[getSymbolId(symbol)] ?? SortTextId.LocationPriority;
const sortText = (isDeprecated(symbol, typeChecker) ? DeprecatedSortTextStart + sortTextId : sortTextId).toString() as SortText;
const entry = createCompletionEntry(
symbol,
symbolToSortTextMap && symbolToSortTextMap[getSymbolId(symbol)] || SortText.LocationPriority,
sortText,
contextToken,
location,
sourceFile,
Expand Down Expand Up @@ -639,7 +662,7 @@ namespace ts.Completions {
add: name => uniques.set(name, true),
};

function shouldIncludeSymbol(symbol: Symbol, symbolToSortTextMap: SymbolSortTextMap): boolean {
function shouldIncludeSymbol(symbol: Symbol, symbolToSortTextIdMap: SymbolSortTextIdMap): boolean {
if (!isSourceFile(location)) {
// export = /**/ here we want to get all meanings, so any symbol is ok
if (isExportAssignment(location.parent)) {
Expand All @@ -661,9 +684,9 @@ namespace ts.Completions {
// Auto Imports are not available for scripts so this conditional is always false
if (!!sourceFile.externalModuleIndicator
&& !compilerOptions.allowUmdGlobalAccess
&& symbolToSortTextMap[getSymbolId(symbol)] === SortText.GlobalsOrKeywords
&& (symbolToSortTextMap[getSymbolId(symbolOrigin)] === SortText.AutoImportSuggestions
|| symbolToSortTextMap[getSymbolId(symbolOrigin)] === SortText.LocationPriority)) {
&& symbolToSortTextIdMap[getSymbolId(symbol)] === SortTextId.GlobalsOrKeywords
&& (symbolToSortTextIdMap[getSymbolId(symbolOrigin)] === SortTextId.AutoImportSuggestions
|| symbolToSortTextIdMap[getSymbolId(symbolOrigin)] === SortTextId.LocationPriority)) {
return false;
}
// Continue with origin symbol
Expand All @@ -685,8 +708,6 @@ namespace ts.Completions {
}
}



function getLabelCompletionAtPosition(node: BreakOrContinueStatement): CompletionInfo | undefined {
const entries = getLabelStatementCompletions(node);
if (entries.length) {
Expand Down Expand Up @@ -930,7 +951,7 @@ namespace ts.Completions {
readonly previousToken: Node | undefined;
readonly isJsxInitializer: IsJsxInitializer;
readonly insideJsDocTagTypeExpression: boolean;
readonly symbolToSortTextMap: SymbolSortTextMap;
readonly symbolToSortTextIdMap: SymbolSortTextIdMap;
readonly isTypeOnlyLocation: boolean;
/** In JSX tag name and attribute names, identifiers like "my-tag" or "aria-name" is valid identifier. */
readonly isJsxIdentifierExpected: boolean;
Expand Down Expand Up @@ -1264,7 +1285,7 @@ namespace ts.Completions {
// This also gets mutated in nested-functions after the return
let symbols: Symbol[] = [];
const symbolToOriginInfoMap: SymbolOriginInfoMap = [];
const symbolToSortTextMap: SymbolSortTextMap = [];
const symbolToSortTextIdMap: SymbolSortTextIdMap = [];
const seenPropertySymbols = new Map<SymbolId, true>();
const isTypeOnly = isTypeOnlyCompletion();
const getModuleSpecifierResolutionHost = memoizeOne((isFromPackageJson: boolean) => {
Expand Down Expand Up @@ -1320,7 +1341,7 @@ namespace ts.Completions {
previousToken,
isJsxInitializer,
insideJsDocTagTypeExpression,
symbolToSortTextMap,
symbolToSortTextIdMap,
isTypeOnlyLocation: isTypeOnly,
isJsxIdentifierExpected,
importCompletionNode,
Expand Down Expand Up @@ -1509,7 +1530,7 @@ namespace ts.Completions {

function addSymbolSortInfo(symbol: Symbol) {
if (isStaticProperty(symbol)) {
symbolToSortTextMap[getSymbolId(symbol)] = SortText.LocalDeclarationPriority;
symbolToSortTextIdMap[getSymbolId(symbol)] = SortTextId.LocalDeclarationPriority;
}
}

Expand Down Expand Up @@ -1626,7 +1647,7 @@ namespace ts.Completions {
for (const symbol of symbols) {
if (!typeChecker.isArgumentsSymbol(symbol) &&
!some(symbol.declarations, d => d.getSourceFile() === sourceFile)) {
symbolToSortTextMap[getSymbolId(symbol)] = SortText.GlobalsOrKeywords;
symbolToSortTextIdMap[getSymbolId(symbol)] = SortTextId.GlobalsOrKeywords;
}
}

Expand All @@ -1637,7 +1658,7 @@ namespace ts.Completions {
for (const symbol of getPropertiesForCompletion(thisType, typeChecker)) {
symbolToOriginInfoMap[symbols.length] = { kind: SymbolOriginInfoKind.ThisType };
symbols.push(symbol);
symbolToSortTextMap[getSymbolId(symbol)] = SortText.SuggestedClassMembers;
symbolToSortTextIdMap[getSymbolId(symbol)] = SortTextId.SuggestedClassMembers;
}
}
}
Expand Down Expand Up @@ -1719,7 +1740,7 @@ namespace ts.Completions {
return false;
}

/** Mutates `symbols`, `symbolToOriginInfoMap`, and `symbolToSortTextMap` */
/** Mutates `symbols`, `symbolToOriginInfoMap`, and `symbolToSortTextIdMap` */
function collectAutoImports(resolveModuleSpecifiers: boolean) {
if (!shouldOfferImportCompletions()) return;
Debug.assert(!detailsEntryId?.data);
Expand Down Expand Up @@ -1790,12 +1811,12 @@ namespace ts.Completions {

function pushAutoImportSymbol(symbol: Symbol, origin: SymbolOriginInfoResolvedExport | SymbolOriginInfoExport) {
const symbolId = getSymbolId(symbol);
if (symbolToSortTextMap[symbolId] === SortText.GlobalsOrKeywords) {
if (symbolToSortTextIdMap[symbolId] === SortTextId.GlobalsOrKeywords) {
// If an auto-importable symbol is available as a global, don't add the auto import
return;
}
symbolToOriginInfoMap[symbols.length] = origin;
symbolToSortTextMap[symbolId] = importCompletionNode ? SortText.LocationPriority : SortText.AutoImportSuggestions;
symbolToSortTextIdMap[symbolId] = importCompletionNode ? SortTextId.LocationPriority : SortTextId.AutoImportSuggestions;
symbols.push(symbol);
}

Expand Down Expand Up @@ -2110,7 +2131,7 @@ namespace ts.Completions {
localsContainer.locals?.forEach((symbol, name) => {
symbols.push(symbol);
if (localsContainer.symbol?.exports?.has(name)) {
symbolToSortTextMap[getSymbolId(symbol)] = SortText.OptionalMember;
symbolToSortTextIdMap[getSymbolId(symbol)] = SortTextId.OptionalMember;
}
});
return GlobalsSearch.Success;
Expand Down Expand Up @@ -2558,7 +2579,8 @@ namespace ts.Completions {
function setSortTextToOptionalMember() {
symbols.forEach(m => {
if (m.flags & SymbolFlags.Optional) {
symbolToSortTextMap[getSymbolId(m)] = symbolToSortTextMap[getSymbolId(m)] || SortText.OptionalMember;
const symbolId = getSymbolId(m);
symbolToSortTextIdMap[symbolId] = symbolToSortTextIdMap[symbolId] ?? SortTextId.OptionalMember;
}
});
}
Expand All @@ -2570,7 +2592,7 @@ namespace ts.Completions {
}
for (const contextualMemberSymbol of contextualMemberSymbols) {
if (membersDeclaredBySpreadAssignment.has(contextualMemberSymbol.name)) {
symbolToSortTextMap[getSymbolId(contextualMemberSymbol)] = SortText.MemberDeclaredBySpreadAssignment;
symbolToSortTextIdMap[getSymbolId(contextualMemberSymbol)] = SortTextId.MemberDeclaredBySpreadAssignment;
}
}
}
Expand Down Expand Up @@ -3120,4 +3142,9 @@ namespace ts.Completions {
addToSeen(seenModules, getSymbolId(sym)) &&
checker.getExportsOfModule(sym).some(e => symbolCanBeReferencedAtTypeLocation(e, checker, seenModules));
}

function isDeprecated(symbol: Symbol, checker: TypeChecker) {
const declarations = skipAlias(symbol, checker).declarations;
return !!length(declarations) && every(declarations, isDeprecatedDeclaration);
}
}
4 changes: 0 additions & 4 deletions src/services/symbolDisplay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,6 @@ namespace ts.SymbolDisplay {
return ScriptElementKind.unknown;
}

function isDeprecatedDeclaration(decl: Declaration) {
return !!(getCombinedNodeFlagsAlwaysIncludeJSDoc(decl) & ModifierFlags.Deprecated);
}

function getNormalizedSymbolModifiers(symbol: Symbol) {
if (symbol.declarations && symbol.declarations.length) {
const [declaration, ...declarations] = symbol.declarations;
Expand Down
4 changes: 4 additions & 0 deletions src/services/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3354,5 +3354,9 @@ namespace ts {
|| (!!globalCachePath && startsWith(getCanonicalFileName(globalCachePath), toNodeModulesParent));
}

export function isDeprecatedDeclaration(decl: Declaration) {
return !!(getCombinedNodeFlagsAlwaysIncludeJSDoc(decl) & ModifierFlags.Deprecated);
}

// #endregion
}
13 changes: 6 additions & 7 deletions tests/cases/fourslash/completionsWithDeprecatedTag1.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,27 +25,26 @@
verify.completions({
marker: "1",
includes: [
{ name: "Foo", kind: "interface", kindModifiers: "deprecated" }
{ name: "Foo", kind: "interface", kindModifiers: "deprecated", sortText: completion.SortText.DeprecatedLocationPriority }
]
}, {
marker: "2",
includes: [
{ name: "bar", kind: "method", kindModifiers: "deprecated" }
{ name: "bar", kind: "method", kindModifiers: "deprecated", sortText: completion.SortText.DeprecatedLocationPriority }
]
}, {
marker: "3",
includes: [
{ name: "prop", kind: "property", kindModifiers: "deprecated" }
{ name: "prop", kind: "property", kindModifiers: "deprecated", sortText: completion.SortText.DeprecatedLocationPriority }
]
}, {
marker: "4",
includes: [
{ name: "foobar", kind: "function", kindModifiers: "export,deprecated" }
{ name: "foobar", kind: "function", kindModifiers: "export,deprecated", sortText: completion.SortText.DeprecatedLocationPriority }
]
}, {
marker: "5",
includes: [
{ name: "foobar", kind: "alias", kindModifiers: "export,deprecated" }
{ name: "foobar", kind: "alias", kindModifiers: "export,deprecated", sortText: completion.SortText.DeprecatedLocationPriority }
]
}
);
});
24 changes: 24 additions & 0 deletions tests/cases/fourslash/completionsWithDeprecatedTag10.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/// <reference path="fourslash.ts" />

// @Filename: /foo.ts
/////** @deprecated foo */
////export const foo = 0;

// @Filename: /index.ts
/////**/

verify.completions({
marker: "",
includes: [{
name: "foo",
source: "/foo",
sourceDisplay: "./foo",
hasAction: true,
kind: "const",
kindModifiers: "export,deprecated",
sortText: completion.SortText.DeprecatedAutoImportSuggestions
}],
preferences: {
includeCompletionsForModuleExports: true,
},
});
9 changes: 6 additions & 3 deletions tests/cases/fourslash/completionsWithDeprecatedTag2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@

verify.completions({
marker: "",
includes: [
{ name: "foo", kind: "function", kindModifiers: "deprecated,declare" }
]
includes: [{
name: "foo",
kind: "function",
kindModifiers: "deprecated,declare",
sortText: completion.SortText.DeprecatedLocationPriority
}]
});
9 changes: 6 additions & 3 deletions tests/cases/fourslash/completionsWithDeprecatedTag3.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@

verify.completions({
marker: "",
includes: [
{ name: "foo", kind: "function", kindModifiers: "declare" }
]
includes: [{
name: "foo",
kind: "function",
kindModifiers: "declare",
sortText: completion.SortText.LocationPriority
}]
});
23 changes: 23 additions & 0 deletions tests/cases/fourslash/completionsWithDeprecatedTag4.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/// <reference path="fourslash.ts" />

// @noLib: true

////f({
//// a/**/
//// xyz: ``,
////});
////declare function f(options: {
//// /** @deprecated abc */
//// abc?: number,
//// xyz?: string
////}): void;

verify.completions({
marker: "",
exact: [{
name: "abc",
kind: "property",
kindModifiers: "deprecated,declare,optional",
sortText: completion.SortText.DeprecatedOptionalMember
}],
});

0 comments on commit 3f9e724

Please sign in to comment.