Skip to content

Don't show the currently-completing thing at the cursor in JS files #6707

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

Merged
merged 2 commits into from
Feb 1, 2016
Merged

Don't show the currently-completing thing at the cursor in JS files #6707

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
124bd51
Don't show the currently-completing thing at the cursor in JS files
RyanCavanaugh Jan 28, 2016
1231c9e
Fix case when position === 0
RyanCavanaugh Jan 29, 2016
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
25 changes: 14 additions & 11 deletions src/services/services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ namespace ts {
export interface SourceFile {
/* @internal */ version: string;
/* @internal */ scriptSnapshot: IScriptSnapshot;
/* @internal */ nameTable: Map<string>;
/* @internal */ nameTable: Map<number>;

/* @internal */ getNamedDeclarations(): Map<Declaration[]>;

Expand Down Expand Up @@ -808,7 +808,7 @@ namespace ts {
public languageVersion: ScriptTarget;
public languageVariant: LanguageVariant;
public identifiers: Map<string>;
public nameTable: Map<string>;
public nameTable: Map<number>;
public resolvedModules: Map<ResolvedModule>;
public imports: LiteralExpression[];
public moduleAugmentations: LiteralExpression[];
Expand Down Expand Up @@ -1954,8 +1954,6 @@ namespace ts {
const text = scriptSnapshot.getText(0, scriptSnapshot.getLength());
const sourceFile = createSourceFile(fileName, text, scriptTarget, setNodeParents);
setSourceFileFields(sourceFile, scriptSnapshot, version);
// after full parsing we can use table with interned strings as name table
sourceFile.nameTable = sourceFile.identifiers;
return sourceFile;
}

Expand Down Expand Up @@ -3834,7 +3832,7 @@ namespace ts {

if (isRightOfDot && isSourceFileJavaScript(sourceFile)) {
const uniqueNames = getCompletionEntriesFromSymbols(symbols, entries);
addRange(entries, getJavaScriptCompletionEntries(sourceFile, uniqueNames));
addRange(entries, getJavaScriptCompletionEntries(sourceFile, location.pos, uniqueNames));
}
else {
if (!symbols || symbols.length === 0) {
Expand Down Expand Up @@ -3867,12 +3865,17 @@ namespace ts {

return { isMemberCompletion, isNewIdentifierLocation, entries };

function getJavaScriptCompletionEntries(sourceFile: SourceFile, uniqueNames: Map<string>): CompletionEntry[] {
function getJavaScriptCompletionEntries(sourceFile: SourceFile, position: number, uniqueNames: Map<string>): CompletionEntry[] {
const entries: CompletionEntry[] = [];
const target = program.getCompilerOptions().target;

const nameTable = getNameTable(sourceFile);
for (const name in nameTable) {
// Skip identifiers produced only from the current location
if (nameTable[name] === position) {
continue;
}

if (!uniqueNames[name]) {
uniqueNames[name] = name;
const displayName = getCompletionEntryDisplayName(name, target, /*performCharacterChecks*/ true);
Expand Down Expand Up @@ -5487,7 +5490,7 @@ namespace ts {

const nameTable = getNameTable(sourceFile);

if (lookUp(nameTable, internedName)) {
if (lookUp(nameTable, internedName) !== undefined) {
result = result || [];
getReferencesInNode(sourceFile, symbol, declaredName, node, searchMeaning, findInStrings, findInComments, result, symbolToIndex);
}
Expand Down Expand Up @@ -7513,7 +7516,7 @@ namespace ts {
}

/* @internal */
export function getNameTable(sourceFile: SourceFile): Map<string> {
export function getNameTable(sourceFile: SourceFile): Map<number> {
if (!sourceFile.nameTable) {
initializeNameTable(sourceFile);
}
Expand All @@ -7522,15 +7525,15 @@ namespace ts {
}

function initializeNameTable(sourceFile: SourceFile): void {
const nameTable: Map<string> = {};
const nameTable: Map<number> = {};

walk(sourceFile);
sourceFile.nameTable = nameTable;

function walk(node: Node) {
switch (node.kind) {
case SyntaxKind.Identifier:
nameTable[(<Identifier>node).text] = (<Identifier>node).text;
nameTable[(<Identifier>node).text] = nameTable[(<Identifier>node).text] === undefined ? node.pos : -1;
Copy link
Member

@DanielRosenwasser DanielRosenwasser Jan 29, 2016

Choose a reason for hiding this comment

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

Isn't this too order-dependent? What happens if you add code prior to another usage? For instance, I start out with:

if (Person.blah) {
    doSomething();
}

I'd want to also get completion for if I added a use of Person.blah before the if statement:

Person.blah/**/

if (Person.blah) {
    doSomething();
}

Copy link
Member Author

@RyanCavanaugh RyanCavanaugh Jan 29, 2016

Choose a reason for hiding this comment

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

If you have multiple uses, the value in the table becomes -1 and no filtering occurs

Copy link
Member

@DanielRosenwasser DanielRosenwasser Jan 29, 2016

Choose a reason for hiding this comment

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

Ah, okay. I misunderstood.

Why don't you use an enum that would make this explicit?

const name = (node as Identifier).text;
nameTable[name] = hasProperty(nameTable, name) ? NameTableState.Once : NameTableState.Many;

or at the very least, leave a nice big comment explaining what you're doing here and what -1 means.

Copy link
Member Author

@RyanCavanaugh RyanCavanaugh Jan 29, 2016

Choose a reason for hiding this comment

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

Because we need the position in the case where it's Once.

Copy link
Member

@DanielRosenwasser DanielRosenwasser Jan 30, 2016

Choose a reason for hiding this comment

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

Ah, right. You're not checking the current identifier and asking "should I actually show this", you're just grabbing up identifiers. I'm not sure why I missed that.

break;
case SyntaxKind.StringLiteral:
case SyntaxKind.NumericLiteral:
Expand All @@ -7542,7 +7545,7 @@ namespace ts {
node.parent.kind === SyntaxKind.ExternalModuleReference ||
isArgumentOfElementAccessExpression(node)) {

nameTable[(<LiteralExpression>node).text] = (<LiteralExpression>node).text;
nameTable[(<LiteralExpression>node).text] = nameTable[(<LiteralExpression>node).text] === undefined ? node.pos : -1;
}
break;
default:
Expand Down
21 changes: 21 additions & 0 deletions tests/cases/fourslash/getJavaScriptCompletions20.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/// <reference path="fourslash.ts" />

// @allowNonTsExtensions: true
// @Filename: file.js
//// /**
//// * A person
//// * @constructor
//// * @param {string} name - The name of the person.
//// * @param {number} age - The age of the person.
//// */
//// function Person(name, age) {
//// this.name = name;
//// this.age = age;
//// }
////
////
//// Person.getName = 10;
//// Person.getNa/**/ = 10;

goTo.marker();
verify.not.memberListContains('getNa');