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 exported members of all project files in the global completion list #19069

Merged
45 commits merged into from
Oct 17, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
d99675b
checker.ts: Remove null check on symbols
minestarks Aug 10, 2017
5bef866
tsserverProjectSystem.ts: add two tests
minestarks Aug 10, 2017
087de79
client.ts, completions.ts, types.ts: Add codeActions member to Comple…
minestarks Aug 10, 2017
a84b5b5
protocol.ts, session.ts: Add codeActions member to CompletionEntryDet…
minestarks Aug 10, 2017
15b73d0
protocol.ts, session.ts, types.ts: add hasAction to CompletionEntry
minestarks Aug 10, 2017
8a1a124
session.ts, services.ts, types.ts: Add formattingOptions parameter to…
minestarks Aug 10, 2017
0aa865f
completions.ts: define SymbolOriginInfo type
minestarks Aug 10, 2017
25831a8
completions.ts, services.ts: Add allSourceFiles parameter to getCompl…
minestarks Aug 10, 2017
9940c92
completions.ts, services.ts: Plumb allSourceFiles into new function g…
minestarks Aug 10, 2017
c838093
completions.ts: add symbolToOriginInfoMap parameter to getCompletionE…
minestarks Aug 10, 2017
c5cc2f1
utilities.ts: Add getOtherModuleSymbols, getUniqueSymbolIdAsString, g…
minestarks Aug 10, 2017
b024285
completions.ts: Set CompletionEntry.hasAction when symbol is found in…
minestarks Aug 10, 2017
041302f
completions.ts: Populate list with possible exports (implement getSym…
minestarks Aug 10, 2017
abe1fdb
completions.ts, services.ts: Plumb host and rulesProvider into getCom…
minestarks Aug 10, 2017
ae0ab47
completions.ts: Add TODO comment
minestarks Aug 10, 2017
95a9c01
importFixes.ts: Add types ImportDeclarationMap and ImportCodeFixContext
minestarks Aug 10, 2017
380b299
Move getImportDeclarations into getCodeActionForImport, immediately a…
minestarks Aug 10, 2017
22c3373
importFixes.ts: Move createChangeTracker into getCodeActionForImport,…
minestarks Aug 10, 2017
8d5e075
importFixes.ts: Add convertToImportCodeFixContext function and refere…
minestarks Aug 10, 2017
2875c15
importFixes.ts: Add context: ImportCodeFixContext parameter to getCod…
minestarks Aug 10, 2017
e7d966b
importFixes.ts: Remove moduleSymbol parameter from getImportDeclarati…
minestarks Aug 10, 2017
e912a76
importFixes.ts: Use cachedImportDeclarations from context in getCodeA…
minestarks Aug 10, 2017
de7b821
importFixes.ts: Move createCodeAction out, immediately above convertT…
minestarks Aug 10, 2017
fa33d50
Move the declaration for lastImportDeclaration out of the getCodeActi…
minestarks Aug 10, 2017
13a47e2
importFixes.ts: Use symbolToken in getCodeActionForImport
minestarks Aug 10, 2017
8e5febb
importFixes.ts: Remove useCaseSensitiveFileNames altogether from getC…
minestarks Aug 10, 2017
2ea36a6
importFixes.ts: Remove local getUniqueSymbolId function and add check…
minestarks Aug 10, 2017
b11f6e8
importFixes.ts: Move getCodeActionForImport out into an export, immed…
minestarks Aug 10, 2017
72dd99f
completions.ts: In getCompletionEntryDetails, if there's symbolOrigin…
minestarks Aug 10, 2017
e68c951
importFixes.ts: Create and use importFixContext within getCodeActions…
minestarks Aug 10, 2017
bc14bb0
importFixes.ts: Use local newLineCharacter instead of context.newLine…
minestarks Aug 10, 2017
d1bdc25
importFixes.ts: Use local host instead of context.host in getCodeActi…
minestarks Aug 10, 2017
f0c983a
importFixes.ts: Remove dummy getCanonicalFileName line
minestarks Aug 10, 2017
a41f3df
Filter symbols after gathering exports instead of before
minestarks Aug 16, 2017
49bb2b8
Merge branch 'master' of https://github.com/Microsoft/TypeScript into…
minestarks Aug 16, 2017
211e3f9
Lint
minestarks Aug 16, 2017
71c7c3f
Merge branch 'exportsincompletionlist' of https://github.com/minestar…
Sep 27, 2017
64e30db
Test, fix bugs, refactor
Sep 27, 2017
30ebde4
Merge branch 'master' into exportsincompletionlist3
Oct 10, 2017
cb6e302
Merge branch 'master' into exportsincompletionlist4
mhegazy Oct 10, 2017
c844297
Suggestions from code review
Oct 10, 2017
5da1595
Update api baseline
Oct 11, 2017
13351db
Merge branch 'master' into exportsincompletionlist4
Oct 13, 2017
a5b846f
Fix bug if previousToken is not an Identifier
Oct 13, 2017
4c607e7
Replace `startsWith` with `stringContainsCharactersInOrder`
Oct 16, 2017
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
18 changes: 11 additions & 7 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,7 @@ namespace ts {
const jsObjectLiteralIndexInfo = createIndexInfo(anyType, /*isReadonly*/ false);

const globals = createSymbolTable();
let ambientModulesCache: Symbol[] | undefined;
/**
* List of every ambient module with a "*" wildcard.
* Unlike other ambient modules, these can't be stored in `globals` because symbol tables only deal with exact matches.
Expand Down Expand Up @@ -25585,13 +25586,16 @@ namespace ts {
}

function getAmbientModules(): Symbol[] {
const result: Symbol[] = [];
globals.forEach((global, sym) => {
if (ambientModuleSymbolRegex.test(unescapeLeadingUnderscores(sym))) {
result.push(global);
}
});
return result;
if (!ambientModulesCache) {
ambientModulesCache = [];
globals.forEach((global, sym) => {
// No need to `unescapeLeadingUnderscores`, an escaped symbol is never an ambient module.
if (ambientModuleSymbolRegex.test(sym as string)) {
ambientModulesCache.push(global);
}
});
}
return ambientModulesCache;
}

function checkGrammarImportCallExpression(node: ImportCall): boolean {
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/commandLineParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1183,8 +1183,8 @@ namespace ts {
}
}

function isDoubleQuotedString(node: Node) {
return node.kind === SyntaxKind.StringLiteral && getSourceTextOfNodeFromSourceFile(sourceFile, node).charCodeAt(0) === CharacterCodes.doubleQuote;
function isDoubleQuotedString(node: Node): boolean {
return isStringLiteral(node) && isStringDoubleQuoted(node, sourceFile);
}
}

Expand Down
30 changes: 30 additions & 0 deletions src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,18 @@ namespace ts {
}
return undefined;
}

/** Like `forEach`, but suitable for use with numbers and strings (which may be falsy). */
export function firstDefined<T, U>(array: ReadonlyArray<T> | undefined, callback: (element: T, index: number) => U | undefined): U | undefined {
for (let i = 0; i < array.length; i++) {
const result = callback(array[i], i);
if (result !== undefined) {
return result;
}
}
return undefined;
}

/**
* Iterates through the parent chain of a node and performs the callback on each parent until the callback
* returns a truthy value, then returns that value.
Expand Down Expand Up @@ -261,6 +273,16 @@ namespace ts {
return undefined;
}

export function findLast<T>(array: ReadonlyArray<T>, predicate: (element: T, index: number) => boolean): T | undefined {
for (let i = array.length - 1; i >= 0; i--) {
const value = array[i];
if (predicate(value, i)) {
return value;
}
}
return undefined;
}

/** Works like Array.prototype.findIndex, returning `-1` if no element satisfying the predicate is found. */
export function findIndex<T>(array: ReadonlyArray<T>, predicate: (element: T, index: number) => boolean): number {
for (let i = 0; i < array.length; i++) {
Expand Down Expand Up @@ -1147,6 +1169,14 @@ namespace ts {
return result;
}

export function arrayToNumericMap<T>(array: ReadonlyArray<T>, makeKey: (value: T) => number): T[] {
const result: T[] = [];
for (const value of array) {
result[makeKey(value)] = value;
}
return result;
}

/**
* Creates a set from the elements of an array.
*
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -3661,15 +3661,15 @@
"category": "Error",
"code": 90010
},
"Import {0} from {1}.": {
"Import '{0}' from \"{1}\".": {
"category": "Message",
"code": 90013
},
"Change '{0}' to '{1}'.": {
"category": "Message",
"code": 90014
},
"Add {0} to existing import declaration from {1}.": {
"Add '{0}' to existing import declaration from \"{1}\".": {
"category": "Message",
"code": 90015
},
Expand Down
6 changes: 5 additions & 1 deletion src/compiler/moduleNameResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,11 @@ namespace ts {
}
}

export function getEffectiveTypeRoots(options: CompilerOptions, host: { directoryExists?: (directoryName: string) => boolean, getCurrentDirectory?: () => string }): string[] | undefined {
export interface GetEffectiveTypeRootsHost {
directoryExists?(directoryName: string): boolean;
getCurrentDirectory?(): string;
}
export function getEffectiveTypeRoots(options: CompilerOptions, host: GetEffectiveTypeRootsHost): string[] | undefined {
if (options.typeRoots) {
return options.typeRoots;
}
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1055,6 +1055,7 @@ namespace ts {
export interface StringLiteral extends LiteralExpression {
kind: SyntaxKind.StringLiteral;
/* @internal */ textSourceNode?: Identifier | StringLiteral | NumericLiteral; // Allows a StringLiteral to get its text from another node (used by transforms).
/** Note: this is only set when synthesizing a node, not during parsing. */
/* @internal */ singleQuote?: boolean;
}

Expand Down
15 changes: 15 additions & 0 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,17 @@ namespace ts {
}
}

/* @internal */
export function isAnyImportSyntax(node: Node): node is AnyImportSyntax {
switch (node.kind) {
case SyntaxKind.ImportDeclaration:
case SyntaxKind.ImportEqualsDeclaration:
return true;
default:
return false;
}
}

// Gets the nearest enclosing block scope container that has the provided node
// as a descendant, that is not the provided node.
export function getEnclosingBlockScopeContainer(node: Node): Node {
Expand Down Expand Up @@ -1375,6 +1386,10 @@ namespace ts {
return charCode === CharacterCodes.singleQuote || charCode === CharacterCodes.doubleQuote;
}

export function isStringDoubleQuoted(string: StringLiteral, sourceFile: SourceFile): boolean {
return getSourceTextOfNodeFromSourceFile(sourceFile, string).charCodeAt(0) === CharacterCodes.doubleQuote;
}

/**
* Returns true if the node is a variable declaration whose initializer is a function expression.
* This function does not test if the node is in a JavaScript file or not.
Expand Down
67 changes: 58 additions & 9 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -783,10 +783,10 @@ namespace FourSlash {
});
}

public verifyCompletionListContains(symbol: string, text?: string, documentation?: string, kind?: string, spanIndex?: number) {
public verifyCompletionListContains(symbol: string, text?: string, documentation?: string, kind?: string, spanIndex?: number, hasAction?: boolean) {
const completions = this.getCompletionListAtCaret();
if (completions) {
this.assertItemInCompletionList(completions.entries, symbol, text, documentation, kind, spanIndex);
this.assertItemInCompletionList(completions.entries, symbol, text, documentation, kind, spanIndex, hasAction);
}
else {
this.raiseError(`No completions at position '${this.currentCaretPosition}' when looking for '${symbol}'.`);
Expand Down Expand Up @@ -1127,7 +1127,7 @@ Actual: ${stringify(fullActual)}`);
}

private getCompletionEntryDetails(entryName: string) {
return this.languageService.getCompletionEntryDetails(this.activeFile.fileName, this.currentCaretPosition, entryName);
return this.languageService.getCompletionEntryDetails(this.activeFile.fileName, this.currentCaretPosition, entryName, this.formatCodeSettings);
}

private getReferencesAtCaret() {
Expand Down Expand Up @@ -2289,6 +2289,29 @@ Actual: ${stringify(fullActual)}`);
this.applyCodeActions(this.getCodeFixActions(fileName, errorCode), index);
}

public applyCodeActionFromCompletion(markerName: string, options: FourSlashInterface.VerifyCompletionActionOptions) {
this.goToMarker(markerName);

const actualCompletion = this.getCompletionListAtCaret().entries.find(e => e.name === options.name);

if (!actualCompletion.hasAction) {
this.raiseError(`Completion for ${options.name} does not have an associated action.`);
}

const details = this.getCompletionEntryDetails(options.name);
if (details.codeActions.length !== 1) {
this.raiseError(`Expected one code action, got ${details.codeActions.length}`);
}

if (details.codeActions[0].description !== options.description) {
this.raiseError(`Expected description to be:\n${options.description}\ngot:\n${details.codeActions[0].description}`);
}

this.applyCodeActions(details.codeActions);

this.verifyNewContent(options);
}

public verifyRangeIs(expectedText: string, includeWhiteSpace?: boolean) {
const ranges = this.getRanges();
if (ranges.length !== 1) {
Expand Down Expand Up @@ -2360,6 +2383,10 @@ Actual: ${stringify(fullActual)}`);
this.applyEdits(change.fileName, change.textChanges, /*isFormattingEdit*/ false);
}

this.verifyNewContent(options);
}

private verifyNewContent(options: FourSlashInterface.NewContentOptions) {
if (options.newFileContent) {
assert(!options.newRangeContent);
this.verifyCurrentFileContent(options.newFileContent);
Expand Down Expand Up @@ -2933,7 +2960,15 @@ Actual: ${stringify(fullActual)}`);
return text.substring(startPos, endPos);
}

private assertItemInCompletionList(items: ts.CompletionEntry[], name: string, text?: string, documentation?: string, kind?: string, spanIndex?: number) {
private assertItemInCompletionList(
items: ts.CompletionEntry[],
name: string,
text: string | undefined,
documentation: string | undefined,
kind: string | undefined,
spanIndex: number | undefined,
hasAction: boolean | undefined,
) {
for (const item of items) {
if (item.name === name) {
if (documentation !== undefined || text !== undefined) {
Expand All @@ -2956,6 +2991,8 @@ Actual: ${stringify(fullActual)}`);
assert.isTrue(TestState.textSpansEqual(span, item.replacementSpan), this.assertionMessageAtLastKnownMarker(stringify(span) + " does not equal " + stringify(item.replacementSpan) + " replacement span for " + name));
}

assert.equal(item.hasAction, hasAction);

return;
}
}
Expand Down Expand Up @@ -3669,12 +3706,12 @@ namespace FourSlashInterface {

// Verifies the completion list contains the specified symbol. The
// completion list is brought up if necessary
public completionListContains(symbol: string, text?: string, documentation?: string, kind?: string, spanIndex?: number) {
public completionListContains(symbol: string, text?: string, documentation?: string, kind?: string, spanIndex?: number, hasAction?: boolean) {
if (this.negative) {
this.state.verifyCompletionListDoesNotContain(symbol, text, documentation, kind, spanIndex);
}
else {
this.state.verifyCompletionListContains(symbol, text, documentation, kind, spanIndex);
this.state.verifyCompletionListContains(symbol, text, documentation, kind, spanIndex, hasAction);
}
}

Expand Down Expand Up @@ -3999,6 +4036,10 @@ namespace FourSlashInterface {
this.state.getAndApplyCodeActions(errorCode, index);
}

public applyCodeActionFromCompletion(markerName: string, options: VerifyCompletionActionOptions): void {
this.state.applyCodeActionFromCompletion(markerName, options);
}

public importFixAtPosition(expectedTextArray: string[], errorCode?: number): void {
this.state.verifyImportFixAtPosition(expectedTextArray, errorCode);
}
Expand Down Expand Up @@ -4396,12 +4437,20 @@ namespace FourSlashInterface {
isNewIdentifierLocation?: boolean;
}

export interface VerifyCodeFixOptions {
description: string;
// One of these should be defined.
export interface NewContentOptions {
// Exactly one of these should be defined.
newFileContent?: string;
newRangeContent?: string;
}

export interface VerifyCodeFixOptions extends NewContentOptions {
description: string;
errorCode?: number;
index?: number;
}

export interface VerifyCompletionActionOptions extends NewContentOptions {
name: string;
description: string;
}
}
4 changes: 2 additions & 2 deletions src/harness/harnessLanguageService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -405,8 +405,8 @@ namespace Harness.LanguageService {
getCompletionsAtPosition(fileName: string, position: number): ts.CompletionInfo {
return unwrapJSONCallResult(this.shim.getCompletionsAtPosition(fileName, position));
}
getCompletionEntryDetails(fileName: string, position: number, entryName: string): ts.CompletionEntryDetails {
return unwrapJSONCallResult(this.shim.getCompletionEntryDetails(fileName, position, entryName));
getCompletionEntryDetails(fileName: string, position: number, entryName: string, options: ts.FormatCodeOptions): ts.CompletionEntryDetails {
return unwrapJSONCallResult(this.shim.getCompletionEntryDetails(fileName, position, entryName, JSON.stringify(options)));
}
getCompletionEntrySymbol(): ts.Symbol {
throw new Error("getCompletionEntrySymbol not implemented across the shim layer.");
Expand Down
4 changes: 3 additions & 1 deletion src/server/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,9 @@ namespace ts.server {
const request = this.processRequest<protocol.CompletionDetailsRequest>(CommandNames.CompletionDetails, args);
const response = this.processResponse<protocol.CompletionDetailsResponse>(request);
Debug.assert(response.body.length === 1, "Unexpected length of completion details response body.");
return response.body[0];

const convertedCodeActions = map(response.body[0].codeActions, codeAction => this.convertCodeActions(codeAction, fileName));
return { ...response.body[0], codeActions: convertedCodeActions };
}

getCompletionEntrySymbol(_fileName: string, _position: number, _entryName: string): Symbol {
Expand Down
10 changes: 10 additions & 0 deletions src/server/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1658,6 +1658,11 @@ namespace ts.server.protocol {
* this span should be used instead of the default one.
*/
replacementSpan?: TextSpan;
/**
* Indicates whether commiting this completion entry will require additional code actions to be
* made to avoid errors. The CompletionEntryDetails will have these actions.
*/
hasAction?: true;
}

/**
Expand Down Expand Up @@ -1690,6 +1695,11 @@ namespace ts.server.protocol {
* JSDoc tags for the symbol.
*/
tags: JSDocTagInfo[];

/**
* The associated code actions for this entry
*/
codeActions?: CodeAction[];
}

export interface CompletionsResponse extends Response {
Expand Down
23 changes: 17 additions & 6 deletions src/server/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1178,11 +1178,12 @@ namespace ts.server {

const completions = project.getLanguageService().getCompletionsAtPosition(file, position);
if (simplifiedResult) {
return mapDefined(completions && completions.entries, entry => {
return mapDefined<CompletionEntry, protocol.CompletionEntry>(completions && completions.entries, entry => {
if (completions.isMemberCompletion || (entry.name.toLowerCase().indexOf(prefix.toLowerCase()) === 0)) {
const { name, kind, kindModifiers, sortText, replacementSpan } = entry;
const { name, kind, kindModifiers, sortText, replacementSpan, hasAction } = entry;
const convertedSpan = replacementSpan ? this.decorateSpan(replacementSpan, scriptInfo) : undefined;
return { name, kind, kindModifiers, sortText, replacementSpan: convertedSpan };
// Use `hasAction || undefined` to avoid serializing `false`.
return { name, kind, kindModifiers, sortText, replacementSpan: convertedSpan, hasAction: hasAction || undefined };
}
}).sort((a, b) => compareStrings(a.name, b.name));
}
Expand All @@ -1193,10 +1194,20 @@ namespace ts.server {

private getCompletionEntryDetails(args: protocol.CompletionDetailsRequestArgs): ReadonlyArray<protocol.CompletionEntryDetails> {
const { file, project } = this.getFileAndProject(args);
const position = this.getPositionInFile(args, file);
const scriptInfo = this.projectService.getScriptInfoForNormalizedPath(file);
const position = this.getPosition(args, scriptInfo);
const formattingOptions = project.projectService.getFormatCodeOptions(file);

return mapDefined(args.entryNames, entryName =>
project.getLanguageService().getCompletionEntryDetails(file, position, entryName));
return mapDefined(args.entryNames, entryName => {
const details = project.getLanguageService().getCompletionEntryDetails(file, position, entryName, formattingOptions);
if (details) {
const mappedCodeActions = map(details.codeActions, action => this.mapCodeAction(action, scriptInfo));
Copy link
Contributor

Choose a reason for hiding this comment

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

i would check if details.codeActions is not defined, and return details.

Copy link
Contributor

Choose a reason for hiding this comment

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

or conisder:

if (details) {
    details.codeActions = map(details.codeActions, action => this.mapCodeAction(action, scriptInfo));
}

this avoids the extra object allocation, and will not be marshaled to the caller, since JSON will not marshal undefined properties.

Copy link
Author

Choose a reason for hiding this comment

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

There's no type-safe way to mutate an object of one type into an object of a different type -- I would have to use any and that would cause errors if we needed to transform some other part of details and forgot about it. I don't think the extra allocation is a big deal, since it's only a single object per completionEntryDetails call.

Copy link
Member

Choose a reason for hiding this comment

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

The getCodeFix and GetEditsForRefactor have option of getting simplified result through those methods and full result through GetCodeFixesFull and GetEditsForRefactorFull. Would we want something similar here too?

Copy link
Member

Choose a reason for hiding this comment

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

Also I agree with @mhegazy we shouldnt be allocating object if there is no need. Also looking at this method it looks like there could be multiple entries requested which means those many more new objects?

Copy link
Author

Choose a reason for hiding this comment

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

The simplified results appear to be trying to reduce the amount of JSON sent? That would be more important for "completions" (which has a simplified version) than for "completionDetails".
The mapDefined is in case the client makes multiple requests together, as in trying to get details on "foo" and "fop" at the same time. I don't think it would ever be in the thousands. I think there's a genuine need for a new object since it would have a different type -- see decorateNavigationTree for a similar function that returns an object of different type from its input.

return { ...details, codeActions: mappedCodeActions };
}
else {
return undefined;
}
});
}

private getCompileOnSaveAffectedFileList(args: protocol.FileRequestArgs): ReadonlyArray<protocol.CompileOnSaveAffectedFileListSingleProject> {
Expand Down
Loading