Fix triple slash completions by aozgaa · Pull Request #11328 · microsoft/TypeScript

aozgaa · 2016-10-03T18:32:28Z

Fixes a bug where some editors wouldn't offer correct completions for triple-slash-reference-path completions (TSRPC's) and TSRPC's wouldn't appear for users using backslashes in the path (eg: Windows users).

aozgaa · 2016-10-03T19:28:53Z

Any thoughts, @mhegazy @billti @paulvanbrenk @jramsay ?

billti · 2016-10-04T00:23:04Z

-        readFile(fileName: string): string {
-            return this.host.readFile(fileName);
+        readDirectory(path: string, extensions?: string[], exclude?: string[], include?: string[]): string[] {
+            return this.host.readDirectory(path, exclude, exclude, include);


This doesn't look right. Did you mean to use exclude twice, and not use extensions? If so, add a quick comment to note that it's deliberate.

billti · 2016-10-04T00:41:33Z

                }
            }

+            result = result.filter(entry => entry.name.indexOf(baseName) >= 0);


Is this deliberately fuzzy, rather than just "starts with"? i.e. if the user had typed "./dir/ab|" and was getting completions at the "|", then "xxab" would match the filter as well as "abc".

Yes.

It's worth noting that some editors, eg vscode, appear to perform filtering of results on the client side (haven't found the exact spot in the source where this occurs). I'm not sure what the general expectation from clients is on this front, but if we can quickly tell whether the sub-string occurs or not, I imagine it is more correct and (speculatively) performance is improved by not sending garbage to the client.

Are there any editors that prefer to show all possible completions to provide context, that this would then restrict?

Could we do this check in advance against foundFileName at line 363 and directoryName at line 380, rather than allocating a number of completion objects that are then thrown out?

I've only checked vs and vs code. It appears vs doesn't do any additional filtering, but vs code does (namely, it repeats exactly the same fuzzy matching I've added).

In order to offer a consistent experience, and so typing additional characters refines completions, I added the filtering.

Changed the checking to occur earlier, but I worry slightly that we might be violating DRY.

Removing the filtering altogether.

Per our offline conversation, I found that VS and VS Code provide more detailed completions such as camel-case based ones (for VS Code, viewable at src/vs/base/common/filters.ts) and so I'm deferring the work of filtering to the client, as appears to be the norm in the status quo.

billti · 2016-10-04T00:54:27Z

-            return undefined;
+            return {
+                isMemberCompletion: false,
+                isNewIdentifierLocation: false,


Why is "isNewIdentifierLocation" true if there are results, but false if not? What difference does this make? (Leave a comment to explain if there's a reason for it).

The flag should be true in both cases.

isNewIdentifierLocation is used to tell the editor that the user may be typing in an identifier that doesn't yet exist anywhere in the language service's context. So if you are typing in a new variable, eg var fo| (cursor at |), the LS has no idea how you might finish the statement.

Editors use this to change their behavior in terms of how they encourage you to choose a completion or not. In VS, for example, the behavior of pressing Enter changes depending on whether the flag is set (if true, pressing enter inserts a newline and doesn't take a highlighted completion. If false, pressing enter inserts the selected completion. Behavior when pressing Tab is unaffected by the flag -- the completion is always inserted).

In our case, someone writing a triple slash reference path may want to point to a file that doesn't yet exist, so we should allow them to create what is potentially a "new identifier" in this case.

I've refactored the code to reflect this observation, and added a comment.

billti · 2016-10-04T01:04:25Z

+    verify.completionListContains("f7");
+    verify.completionListContains("d1");
+    verify.completionListContains("d2");
+    verify.not.completionListItemsCountIsGreaterThan(6);


Isn't there just a "verify it is equal to 6" check? You already checked for 6 entries above, so... :-)

The interface (viewable in stc/fourslash/fourslash.ts) doesn't contain such a method, but I can add it if you think it's a good idea. If so, should I do that in a separate PR?

billti · 2016-10-04T01:10:43Z

@@ -0,0 +1,31 @@
+/// <reference path='fourslash.ts' />
+
+// Should give completions for relative references to ts files when allowJs is false


I see lots of tests where allowJs is false. Is there sufficient coverage for checking that when it's true the .js[x] files do appear in references and imports?

For references, we have:

tripleSlashRefPathCompletionAllowJSFalse.ts

tripleSlashRefPathCompletionAllowJSTrue.ts

For imports, we have:

completionForStringLiteralRelativeImportAllowJSFalse.ts

completionForStringLiteralRelativeImportAllowJSTrue.ts

(latter two renamed in the latest commit).

rbuckton · 2016-10-04T23:25:45Z

                }
            }

+            result = result.filter(entry => entry.name.indexOf(baseName) >= 0);


Are there any editors that prefer to show all possible completions to provide context, that this would then restrict?

Could we do this check in advance against foundFileName at line 363 and directoryName at line 380, rather than allocating a number of completion objects that are then thrown out?

rbuckton · 2016-10-04T23:30:15Z

-                fragment = ensureTrailingDirectorySeparator(fragment);
+
+            if (fragment === "") {
+                fragment = "." + directorySeparator;


Isn't this doing the same thing as line 330?

This function really should never get undefined as an argument for fragment, but I don't know of a way to assert that. So the check on line 330 is really a matter of null-checking. In particular, normalizeSlashes() on line 332 dereferences its argument without null-checking.

Regarding line 340, consider the case when fragment is "foo". Then getDirectoryPath(fragment) === "" because it removes the basename part of the path. So we need to check again.

Really, fragment no longer refers to a path fragment after the assignment on line 337. Perhaps a new variable called relativeOrAbsolutePath should be initialized on the LHS of line 337. I didn't make the change because I was trying to minimize the changes I was making to the codebase to get a fix in (with mixed success).

Should I add a new declaration?

No, the comment was specifically about the fact that line 330 uses "./" while line 340 uses "." + directorySeparator", though they do the same thing. Perhaps you should initialize fragment to "" on 330 rather than repeat yourself on 340.

Arthur Ozga added 25 commits September 23, 2016 17:37

Convert to Doc Comment

2035db1

Annotate directorySeparator

b38d7ac

comments

d423aad

Merge branch 'master' into FixAbsoluteTripleSlashCompletions

8f883b9

More comments

1f7b6e6

new test

769d248

Merge branch 'master' into FixAbsoluteTripleSlashCompletions

8a479e8

remove Comment

6dd5482

Merge branch 'master' into FixAbsoluteTripleSlashCompletions

bfb2185

Factored out hidden path test

629e126

factor and simplify rel path test

f6ff6bd

Fix tests

49e0de9

Rename Tests

2195f17

Fix fragment handling

4b07377

Rename Tests

8573631

Pass linting

cb13ae0

New Test

6553413

Question added

a2cbc63

Merge branch 'master' into FixAbsoluteTripleSlashCompletions

d96d098

Merge branch 'master' into FixAbsoluteTripleSlashCompletions

b9e5dbc

add readDirectory to LSHost

3e15394

Rename labels in tests

3ce97af

Merge branch 'master' into FixAbsoluteTripleSlashCompletions

728252f

Removed comments and fixed indentation

179ceda

Remove trailing comment

0871628

msftclas added the cla-not-required label Oct 3, 2016

billti reviewed Oct 4, 2016

View reviewed changes

Arthur Ozga added 5 commits October 4, 2016 13:07

Responding To billti's comments

d737dc1

Fixed a comment

55d4e12

Actually fixed comments

2e589f1

Merge branch 'master' into FixTripleSlashCompletions

eb362fe

merge master and add isGlobalCompletion flags to CompletionInfo

1baf496

rbuckton reviewed Oct 4, 2016

View reviewed changes

Arthur Ozga added 6 commits October 4, 2016 17:44

Eliminate allocation of filtered completions

a12fe2e

Change fragment initialization

617daa9

Remove filtering functionality

24938a7

Merge branch 'master' into FixTripleSlashCompletions

77a2d0e

update tests to reflect no completion filtering

4a497fb

provide completions only in the correct bounds

af833aa

rbuckton approved these changes Oct 6, 2016

View reviewed changes

aozgaa merged commit 94d8955 into master Oct 6, 2016

aozgaa deleted the FixTripleSlashCompletions branch February 3, 2017 23:23

microsoft locked and limited conversation to collaborators Jun 19, 2018

		@@ -0,0 +1,31 @@
		/// <reference path='fourslash.ts' />

		// Should give completions for relative references to ts files when allowJs is false

Conversation

aozgaa commented Oct 3, 2016

Uh oh!

aozgaa commented Oct 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rbuckton Oct 4, 2016 •

edited

Loading

rbuckton Oct 4, 2016 •

edited

Loading