Skip to content

[release-2.4] Filter completion list according to meaning of the location #16529

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 14, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion Gulpfile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,7 @@ declare module "convert-source-map" {
export function fromSource(source: string, largeSource?: boolean): SourceMapConverter;
}

gulp.task("browserify", "Runs browserify on run.js to produce a file suitable for running tests in the browser", [servicesFile], (done) => {
gulp.task("browserify", "Runs browserify on run.js to produce a file suitable for running tests in the browser", [servicesFile, run], (done) => {
const testProject = tsc.createProject("src/harness/tsconfig.json", getCompilerSettings({ outFile: "../../built/local/bundle.js" }, /*useBuiltCompiler*/ true));
return testProject.src()
.pipe(newer("built/local/bundle.js"))
Expand Down
2 changes: 1 addition & 1 deletion Jakefile.js
Original file line number Diff line number Diff line change
Expand Up @@ -959,7 +959,7 @@ var nodeServerInFile = "tests/webTestServer.ts";
compileFile(nodeServerOutFile, [nodeServerInFile], [builtLocalDirectory, tscFile], [], /*useBuiltCompiler:*/ true, { noOutFile: true, lib: "es6" });

desc("Runs browserify on run.js to produce a file suitable for running tests in the browser");
task("browserify", ["tests", builtLocalDirectory, nodeServerOutFile], function() {
task("browserify", ["tests", run, builtLocalDirectory, nodeServerOutFile], function() {
var cmd = 'browserify built/local/run.js -t ./scripts/browserify-optional -d -o built/local/bundle.js';
exec(cmd);
}, { async: true });
Expand Down
23 changes: 21 additions & 2 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14590,12 +14590,31 @@ namespace ts {
? (<PropertyAccessExpression>node).expression
: (<QualifiedName>node).left;

const type = checkExpression(left);
return isValidPropertyAccessWithType(node, left, propertyName, getWidenedType(checkExpression(left)));
}

function isValidPropertyAccessWithType(
node: PropertyAccessExpression | QualifiedName,
left: LeftHandSideExpression | QualifiedName,
propertyName: string,
type: Type): boolean {

if (type !== unknownType && !isTypeAny(type)) {
const prop = getPropertyOfType(getWidenedType(type), propertyName);
const prop = getPropertyOfType(type, propertyName);
if (prop) {
return checkPropertyAccessibility(node, left, type, prop);
}

// In js files properties of unions are allowed in completion
if (isInJavaScriptFile(left) && (type.flags & TypeFlags.Union)) {
for (const elementType of (<UnionType>type).types) {
if (isValidPropertyAccessWithType(node, left, propertyName, elementType)) {
return true;
}
}
}

return false;
}
return true;
}
Expand Down
7 changes: 5 additions & 2 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -814,8 +814,8 @@ namespace FourSlash {

function filterByTextOrDocumentation(entry: ts.CompletionEntry) {
const details = that.getCompletionEntryDetails(entry.name);
const documentation = ts.displayPartsToString(details.documentation);
const text = ts.displayPartsToString(details.displayParts);
const documentation = details && ts.displayPartsToString(details.documentation);
const text = details && ts.displayPartsToString(details.displayParts);

// If any of the expected values are undefined, assume that users don't
// care about them.
Expand Down Expand Up @@ -852,6 +852,9 @@ namespace FourSlash {
if (expectedKind) {
error += "Expected kind: " + expectedKind + " to equal: " + filterCompletions[0].kind + ".";
}
else {
error += "kind: " + filterCompletions[0].kind + ".";
}
if (replacementSpan) {
const spanText = filterCompletions[0].replacementSpan ? stringify(filterCompletions[0].replacementSpan) : undefined;
error += "Expected replacement span: " + stringify(replacementSpan) + " to equal: " + spanText + ".";
Expand Down
172 changes: 129 additions & 43 deletions src/services/completions.ts

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions src/services/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ namespace ts {
else if (node.parent.kind === SyntaxKind.ExportAssignment) {
return SemanticMeaning.All;
}
else if (isInRightSideOfImport(node)) {
else if (isInRightSideOfInternalImportEqualsDeclaration(node)) {
return getMeaningFromRightHandSideOfImportEquals(node);
}
else if (isDeclarationName(node)) {
Expand Down Expand Up @@ -118,7 +118,7 @@ namespace ts {
return SemanticMeaning.Namespace;
}

function isInRightSideOfImport(node: Node) {
export function isInRightSideOfInternalImportEqualsDeclaration(node: Node) {
while (node.parent.kind === SyntaxKind.QualifiedName) {
node = node.parent;
}
Expand Down Expand Up @@ -745,7 +745,7 @@ namespace ts {
// NOTE: JsxText is a weird kind of node that can contain only whitespaces (since they are not counted as trivia).
// if this is the case - then we should assume that token in question is located in previous child.
if (position < child.end && (nodeHasTokens(child) || child.kind === SyntaxKind.JsxText)) {
const start = (includeJsDoc && child.jsDoc ? child.jsDoc[0] : child).getStart(sourceFile);
const start = child.getStart(sourceFile, includeJsDoc);
const lookInPreviousChild =
(start >= position) || // cursor in the leading trivia
(child.kind === SyntaxKind.JsxText && start === child.end); // whitespace only JsxText
Expand Down
6 changes: 3 additions & 3 deletions tests/cases/fourslash/commentsInheritance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ verify.quickInfos({
});

goTo.marker('16');
verify.completionListContains("i1", "interface i1", "i1 is interface with properties");
verify.not.completionListContains("i1", "interface i1", "i1 is interface with properties");
verify.completionListContains("i1_i", "var i1_i: i1", "");
verify.completionListContains("c1", "class c1", "");
verify.completionListContains("c1_i", "var c1_i: c1", "");
Expand Down Expand Up @@ -603,9 +603,9 @@ verify.quickInfos({
});

goTo.marker('51');
verify.completionListContains("i2", "interface i2", "");
verify.not.completionListContains("i2", "interface i2", "");
verify.completionListContains("i2_i", "var i2_i: i2", "");
verify.completionListContains("i3", "interface i3", "");
verify.not.completionListContains("i3", "interface i3", "");
verify.completionListContains("i3_i", "var i3_i: i3", "");

goTo.marker('51i');
Expand Down
8 changes: 4 additions & 4 deletions tests/cases/fourslash/commentsInterface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,11 +163,11 @@ verify.currentParameterHelpArgumentDocCommentIs("");
verify.quickInfoAt("33q", "(method) i2.nc_fnfoo(b: number): string");

goTo.marker('34');
verify.completionListContains("i1", "interface i1", "this is interface 1");
verify.not.completionListContains("i1", "interface i1", "this is interface 1");
verify.completionListContains("i1_i", "var i1_i: i1", "");
verify.completionListContains("nc_i1", "interface nc_i1", "");
verify.not.completionListContains("nc_i1", "interface nc_i1", "");
verify.completionListContains("nc_i1_i", "var nc_i1_i: nc_i1", "");
verify.completionListContains("i2", "interface i2", "this is interface 2 with memebers");
verify.not.completionListContains("i2", "interface i2", "this is interface 2 with memebers");
verify.completionListContains("i2_i", "var i2_i: i2", "");
verify.completionListContains("i2_i_x", "var i2_i_x: number", "");
verify.completionListContains("i2_i_foo", "var i2_i_foo: (b: number) => string", "");
Expand All @@ -194,7 +194,7 @@ verify.completionListContains("a", "(parameter) a: number", "i3_i a");

verify.quickInfoAt("40q", "var i3_i: i3");
goTo.marker('40');
verify.completionListContains("i3", "interface i3", "");
verify.not.completionListContains("i3", "interface i3", "");
verify.completionListContains("i3_i", "var i3_i: i3", "");

goTo.marker('41');
Expand Down
8 changes: 4 additions & 4 deletions tests/cases/fourslash/commentsOverloads.ts
Original file line number Diff line number Diff line change
Expand Up @@ -295,13 +295,13 @@ verify.completionListContains('f3', 'function f3(a: number): number (+1 overload
verify.completionListContains('f4', 'function f4(a: number): number (+1 overload)', 'this is signature 4 - with number parameter');

goTo.marker('18');
verify.completionListContains('i1', 'interface i1', '');
verify.not.completionListContains('i1', 'interface i1', '');
verify.completionListContains('i1_i', 'var i1_i: new i1(b: number) => any (+1 overload)', '');
verify.completionListContains('i2', 'interface i2', '');
verify.not.completionListContains('i2', 'interface i2', '');
verify.completionListContains('i2_i', 'var i2_i: new i2(a: string) => any (+1 overload)', '');
verify.completionListContains('i3', 'interface i3', '');
verify.not.completionListContains('i3', 'interface i3', '');
verify.completionListContains('i3_i', 'var i3_i: new i3(a: string) => any (+1 overload)', 'new 1');
verify.completionListContains('i4', 'interface i4', '');
verify.not.completionListContains('i4', 'interface i4', '');
verify.completionListContains('i4_i', 'var i4_i: new i4(a: string) => any (+1 overload)', '');

goTo.marker('19');
Expand Down
3 changes: 2 additions & 1 deletion tests/cases/fourslash/completionListAfterFunction2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,6 @@ goTo.marker("1");
verify.not.completionListContains("a");

goTo.marker("2");
verify.not.completionListContains("b");
edit.insert("typeof ");
verify.completionListContains("b");

6 changes: 2 additions & 4 deletions tests/cases/fourslash/completionListCladule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
//// export var x: number;
////}
////Foo/*c1*/; // should get "x", "prototype"
////var s: Foo/*c2*/; // should get "x" and "prototype"
////var s: Foo/*c2*/; // no types, in Foo, so shouldnt have anything
////var f = new Foo();
////f/*c3*/;

Expand All @@ -20,9 +20,7 @@ verify.completionListContains("staticMethod");

goTo.marker("c2");
edit.insert(".");
verify.completionListContains("x");
verify.completionListContains("staticMethod");
verify.completionListContains("prototype");
verify.completionListIsEmpty();

goTo.marker("c3");
edit.insert(".");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
///<reference path="fourslash.ts" />
///<reference path="fourslash.ts" />

//// var x = class myClass <TypeParam> {
//// getClassName (){
//// /*0*/
//// var tmp: /*0Type*/;
//// }
//// prop: Ty/*1*/
//// }

goTo.marker("0");
verify.not.completionListContains("TypeParam", "(type parameter) TypeParam in myClass<TypeParam>", /*documentation*/ undefined, "type parameter");
goTo.marker("0Type");
verify.completionListContains("TypeParam", "(type parameter) TypeParam in myClass<TypeParam>", /*documentation*/ undefined, "type parameter");

goTo.marker("1");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@ verify.completionListContains("TString");
verify.completionListContains("TNumber");

// Ideally the following shouldn't show up since they're not types.
verify.completionListContains("foo");
verify.completionListContains("obj");
verify.not.completionListContains("foo");
verify.not.completionListContains("obj");
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@ verify.completionListContains("TString");
verify.completionListContains("TNumber"); // REVIEW: Is this intended behavior?

// Ideally the following shouldn't show up since they're not types.
verify.completionListContains("foo");
verify.completionListContains("obj");
verify.not.completionListContains("foo");
verify.not.completionListContains("obj");
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@ verify.completionListContains("TString");
verify.completionListContains("TNumber"); // REVIEW: Is this intended behavior?

// Ideally the following shouldn't show up since they're not types.
verify.completionListContains("foo");
verify.completionListContains("obj");
verify.not.completionListContains("foo");
verify.not.completionListContains("obj");
2 changes: 1 addition & 1 deletion tests/cases/fourslash/completionListInExtendsClause.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,4 @@ goTo.marker("3");
verify.completionListIsEmpty();

goTo.marker("4");
verify.not.completionListIsEmpty();
verify.completionListIsEmpty();
6 changes: 3 additions & 3 deletions tests/cases/fourslash/completionListInScope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,11 @@ verify.completionListContains("exportedInterface");
verify.completionListContains("localClass");
verify.completionListContains("exportedClass");

verify.completionListContains("localModule");
verify.completionListContains("exportedModule");
verify.not.completionListContains("localModule");
verify.not.completionListContains("exportedModule");

verify.completionListContains("exportedClass2");
verify.completionListContains("exportedModule2");
verify.not.completionListContains("exportedModule2");

goTo.marker("insideMethod");
verify.not.completionListContains("property");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/// <reference path='fourslash.ts'/>
/// <reference path='fourslash.ts'/>

////type List1</*0*/
////type List2</*1*/T> = T[];
Expand All @@ -8,7 +8,7 @@
goTo.marker("0");
verify.completionListIsEmpty();
goTo.marker("1");
verify.not.completionListIsEmpty();
verify.completionListIsEmpty();
goTo.marker("2");
verify.completionListContains("T");
goTo.marker("3");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/// <reference path='fourslash.ts'/>
/// <reference path='fourslash.ts'/>

////type Map1<K, /*0*/
////type Map1<K, /*1*/V> = [];
Expand All @@ -8,12 +8,12 @@
goTo.marker("0");
verify.completionListIsEmpty();
goTo.marker("1");
verify.completionListContains("V");
verify.completionListIsEmpty();
goTo.marker("2");
verify.completionListContains("K");
verify.completionListContains("V");
goTo.marker("3");
verify.not.completionListContains("K");
verify.not.completionListContains("V");
verify.completionListContains("K1");
verify.completionListContains("V1");
verify.not.completionListContains("K1");
verify.not.completionListContains("V1");
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
/// <reference path='fourslash.ts'/>
/// <reference path='fourslash.ts'/>

//// type constructorType<T1, T2> = new <T/*1*/, /*2*/

goTo.marker("1");
verify.completionListContains("T");
verify.completionListContains("T1");
verify.completionListContains("T2");
verify.not.completionListContains("T");
verify.not.completionListContains("T1");
verify.not.completionListContains("T2");

goTo.marker("2");
verify.completionListContains("T");
verify.completionListContains("T1");
verify.completionListContains("T2");
verify.not.completionListContains("T");
verify.not.completionListContains("T1");
verify.not.completionListContains("T2");
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

goTo.marker("1");
verify.completionListContains("C");
verify.completionListContains("foo"); // ideally this shouldn't show up for a type
verify.not.completionListContains("foo"); // ideally this shouldn't show up for a type
edit.insert("typeof ");
verify.completionListContains("C");
verify.completionListContains("foo");
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@ verify.completionListContains("TString");
verify.completionListContains("TNumber");

// Ideally the following shouldn't show up since they're not types.
verify.completionListContains("foo");
verify.completionListContains("obj");
verify.not.completionListContains("foo");
verify.not.completionListContains("obj");
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@ verify.completionListContains("TString");
verify.completionListContains("TNumber"); // REVIEW: Is this intended behavior?

// Ideally the following shouldn't show up since they're not types.
verify.completionListContains("foo");
verify.completionListContains("obj");
verify.not.completionListContains("foo");
verify.not.completionListContains("obj");
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@ verify.completionListContains("TString");
verify.completionListContains("TNumber"); // REVIEW: Is this intended behavior?

// Ideally the following shouldn't show up since they're not types.
verify.completionListContains("foo");
verify.completionListContains("obj");
verify.not.completionListContains("foo");
verify.not.completionListContains("obj");
Loading