Code fix to remove unused named import should preserve default import by minestarks · Pull Request #17078 · microsoft/TypeScript

minestarks · 2017-07-10T21:24:27Z

amcasey

Mostly open questions - no actual requests.

amcasey · 2017-07-10T21:35:48Z

            if (index === undefined) {
                if (!(actions && actions.length === 1)) {
-                    this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found.`);
+                    this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found. ${actions ? actions.map(a => `"${a.description}"`).join(", ") : "" }`);


Yeah, I had it that way at first, but it looks silly when actions.length is zero. Instead of adding fancy formatting logic I went with something that looks sort of okay in either case :)

amcasey · 2017-07-10T21:37:16Z

            }

+            if (this.currentCaretPosition < 0) {
+                this.currentCaretPosition = 0;


Why is this okay? Shouldn't it be an error to make the position go negative?

It's an effect of this code to adjust caret position as we apply edits:

if (offsetStart <= this.currentCaretPosition) { this.currentCaretPosition += editDelta; }

editDelta can be negative. If you delete a span that contains the current caret position (perfectly valid), the caret position may end up going negative.

There are alternative ways of fixing this (e.g. make sure the caret position remains positive inside the for loop instead of fixing it afterwards) but I didn't see a compelling reason to prefer one over the other.

If the caret is in the middle of some range that is deleted, should the caret go to the beginning of the range? Or be invalidated?

I'm unsure this can happen in a real editing scenario, but it can make sense for sequencing checks in tests.

If the caret is in the middle of some range that is deleted, should the caret go to the beginning of the range?

@aozgaa yes. This was also the conclusion we arrived with @amcasey after discussing offline. Will make that change.
Perfectly valid scenario - this happens in remove unused quickfix, for instance (which is I guess why I was hitting the invalid caret situation)

amcasey · 2017-07-10T21:39:05Z

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


What's "13FS"?

It's the 13th in a series of files that have that exact naming convention.... I think FS stands for fourslash.

amcasey · 2017-07-10T21:46:48Z

+                        const startPosition = textChanges.getAdjustedStartPosition(sourceFile, previousToken, {}, textChanges.Position.FullStart);
+                        return [deleteRange({ pos: startPosition, end: namedBindings.end })];
+                    }
+                    return undefined;


The caller declares that it can return undefined, but it's not clear that that ever happened or that it would work if it did.

There also appear to be cases where it returns [undefined], which seems even weirder.

I had my questions about this too.

My understanding is that ultimately this undefined will be returned from getCodeActions which is treated by the codeFixProvider to mean no code fixes are available. So I'm pretty confident returning undefined is valid and will be handled gracefully.

However I am confused about how to handle seemingly impossible cases (like this one). I don't expect to ever reach this branch and I don't have any test coverage for it. I returned undefined here to avoid deleting code in an unexpected case. I considered throwing too. But the previous version of the code would have returned return [deleteRange(namespaceImport)]; and I'm not sure why. Asking @aozgaa ...

I'm not sure about [undefined] -- I have a feeling VS would not take that too well.

I wasn't aware we would return [undefined] in any cases before. Regardless of whether or not the VS extension throws an exception when that happens today (I'd be mildly surprised if it didn't), we definitely should not return an arrays with null values.

We should standardize and agree to only return undefined, even though ts.codefix.getFixes doesn't care.

amcasey · 2017-07-10T21:50:14Z

+                    const previousToken = getTokenAtPosition(sourceFile, namedBindings.pos - 1, /*includeJsDocComment*/ false);
+                    if (previousToken && previousToken.kind === SyntaxKind.CommaToken) {
+                        const startPosition = textChanges.getAdjustedStartPosition(sourceFile, previousToken, {}, textChanges.Position.FullStart);
+                        return [deleteRange({ pos: startPosition, end: namedBindings.end })];


What happens if there are comments in and around the names?

import /* foo */ A, { x } from './a' => import /* foo */ A from './a'
import A, { /* foo */ x } from './a' => import A from './a'
import A, /* foo */ { x } from './a' => import A from './a'
import A /* foo */, { x } from './a' => import A from './a'

I'm not sure I agree with the last one. The others look good.

Using Position.Start instead of Position.FullStart on the comma token should change the behavior to agree with @amcasey's thoughts on the last case while preserving the other cases.

Please add a fourslash test documenting this behavior.

Great, agreed on the desired behavior - I just wasn't sure how to achieve it. Thanks.

aozgaa

a couple points of confusion with the moved code, comment-range handling, nits, and a request to add a test.

aozgaa · 2017-07-10T23:10:36Z

            }

+            if (this.currentCaretPosition < 0) {
+                this.currentCaretPosition = 0;


If the caret is in the middle of some range that is deleted, should the caret go to the beginning of the range? Or be invalidated?

I'm unsure this can happen in a real editing scenario, but it can make sense for sequencing checks in tests.

aozgaa · 2017-07-10T23:14:08Z

@@ -100,7 +98,7 @@ namespace ts.codefix {
                    // or "'import {a, b as ns} from './file'"


Can you change this to use the | delimiters like the other comments in this file?

actually this comment is wrong. I'm removing it. The correct cases are already articulated in inline comments just below.

aozgaa · 2017-07-10T23:32:38Z

+                    // import d|, { a }| from './file'
+                    const previousToken = getTokenAtPosition(sourceFile, namedBindings.pos - 1, /*includeJsDocComment*/ false);
+                    if (previousToken && previousToken.kind === SyntaxKind.CommaToken) {
+                        const startPosition = textChanges.getAdjustedStartPosition(sourceFile, previousToken, {}, textChanges.Position.FullStart);


this doesn't appear to be called anywhere outside of textChanges.ts. Can you explain why it is used here/what happens if it we just use previousToken.getStart(sourceFile)?

Good question. It's just carried over from the method I was refactoring, so I'm not sure why it was used specifically. I'll investigate.

OK actually previousToken.getStart() is the one to use here, to avoid the comment problem called out below. Using fullStart would wipe out comments leading up to the comma.

aozgaa · 2017-07-10T23:34:40Z

+                    const previousToken = getTokenAtPosition(sourceFile, namedBindings.pos - 1, /*includeJsDocComment*/ false);
+                    if (previousToken && previousToken.kind === SyntaxKind.CommaToken) {
+                        const startPosition = textChanges.getAdjustedStartPosition(sourceFile, previousToken, {}, textChanges.Position.FullStart);
+                        return [deleteRange({ pos: startPosition, end: namedBindings.end })];


Using Position.Start instead of Position.FullStart on the comma token should change the behavior to agree with @amcasey's thoughts on the last case while preserving the other cases.

Please add a fourslash test documenting this behavior.

aozgaa · 2017-07-10T23:47:10Z

+                        const startPosition = textChanges.getAdjustedStartPosition(sourceFile, previousToken, {}, textChanges.Position.FullStart);
+                        return [deleteRange({ pos: startPosition, end: namedBindings.end })];
+                    }
+                    return undefined;


I wasn't aware we would return [undefined] in any cases before. Regardless of whether or not the VS extension throws an exception when that happens today (I'd be mildly surprised if it didn't), we definitely should not return an arrays with null values.

We should standardize and agree to only return undefined, even though ts.codefix.getFixes doesn't care.

aozgaa · 2017-07-10T23:49:17Z


                    case SyntaxKind.NamespaceImport:
-                        const namespaceImport = <NamespaceImport>parent;
-                        if (namespaceImport.name === identifier && !(<ImportClause>namespaceImport.parent).name) {


It looks like the first check was removed. Why?

I believed namespaceImport.name !== identifier is an unreachable condition. So it seemed like an arbitrary extra check. If we have an identifier whose parent is a NamespaceImport, then by definition namespaceImport.name has to be that identifier. Is that not right?

Yes, according to types.ts, it is a redundant check

aozgaa · 2017-07-10T23:53:38Z

            if (index === undefined) {
                if (!(actions && actions.length === 1)) {
-                    this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found.`);
+                    this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found. ${actions ? actions.map(a => `"${a.description}"`).join(", ") : "" }`);


This is great!

maybe ...but actually got ${actions ? actions.length : 0} actions, namely\n${...?

Also, would splitting with a newline be more readable?

Okay, since I got two comments on this, I'll rearrange this into a better string

amcasey

Thanks for making the extra fixes!

amcasey · 2017-07-11T22:16:20Z

+                    }
+                    else {
+                        // The span being replaced includes the caret position, place the caret at the beginning of the span
+                        this.currentCaretPosition = offsetStart;


Would it make sense to put the caret at the end of the new text? It should have the same behavior as this code when the new text is empty.

In practice, the test cases only hit this when the caret is already at the start of the span (i.e. this.currentCaretPosition === offsetStart already). So it makes sense not to move it in that case. When the caret is in the middle, it's kind of arbitrary which way to go. I chose the route with the least logic. (Note: I checked VS it does something different altogether. When the caret is in the middle, it only repositions the caret if it would end up outside the range of the new text. Which makes sense. But we literally never hit this case in tests, so I'm not going to spend any more time on it).

amcasey · 2017-07-11T22:17:29Z

            if (index === undefined) {
                if (!(actions && actions.length === 1)) {
-                    this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found. ${actions ? actions.map(a => `"${a.description}"`).join(", ") : "" }`);
+                    this.raiseError(`Should find exactly one codefix, but ${actions ? actions.length : "none"} found. ${actions ? actions.map(a => `\r\n "${a.description}"`) : "" }`);


Is there a helper for retrieving the platform-specific newline? (i.e. does this work on Linux?)

amcasey · 2017-07-11T22:19:21Z


                default:
-                    return [deleteDefault()];
+                    return deleteDefault();


Thanks for fixing this!

amcasey · 2017-07-11T22:20:32Z

+
+// @noUnusedLocals: true
+// @Filename: file2.ts
+//// [| import /* 1 */ A /* 2 */, /* 3 */ { x } from './a'; |]


{ /* 4 */ x /* 5 */ } /* 6 */?

Ha, I guess I got tired of typing out comments. I didn't think it was interesting to test since we're removing the { x } node altogether. But might as well.

The other thing is, I can't decide if that /* 6 */ should stay or go. I think either way is fine, so I don't want to necessarily codify the existing behavior in tests. (Currently, /* 6 */ stays.)

My preference, in such cases, is to capture the current behavior but add a comment in the test (which will fail if it changes) explaining that the test is descriptive, rather than normative (i.e. the failure can be addressed by updating the baseline).

minestarks · 2017-07-12T18:15:10Z

@aozgaa do I have your approval?

aozgaa

Looks good, thanks for the clarifications and changes!

aozgaa · 2017-07-13T17:43:56Z


                    case SyntaxKind.NamespaceImport:
-                        const namespaceImport = <NamespaceImport>parent;
-                        if (namespaceImport.name === identifier && !(<ImportClause>namespaceImport.parent).name) {


Yes, according to types.ts, it is a redundant check

minestarks added 3 commits July 10, 2017 12:49

Applying edits in Fourslash can cause caret to move off-range

0c40c18

More detailed error logging in Fourslash

b6b6d05

Code fix to remove unused import should preserve default import

39e4b1f

minestarks requested review from amcasey and aozgaa July 10, 2017 21:24

msftclas added the cla-not-required label Jul 10, 2017

minestarks changed the title ~~Code fix to remove unused import should preserve default import~~ Code fix to remove unused named import should preserve default import Jul 10, 2017

amcasey suggested changes Jul 10, 2017

View reviewed changes

aozgaa suggested changes Jul 11, 2017

View reviewed changes

minestarks added 5 commits July 11, 2017 14:52

Fix caret update logic in fourslash tests

003c28f

format error message with newlines

5fd16ca

Remove incorrect comment

bb063f1

Fix comment behavior in remove unused named bindings

80b64de

Fix case where we can return [undefined]

3915d46

amcasey approved these changes Jul 11, 2017

View reviewed changes

minestarks added 2 commits July 11, 2017 16:05

Use platform agnostic newline

0694a38

Add a bit more validation around comments

abb229e

aozgaa approved these changes Jul 13, 2017

View reviewed changes

minestarks merged commit fd2dd2e into microsoft:master Jul 13, 2017

minestarks mentioned this pull request Aug 22, 2017

'index' shouldn't be included as part of module path in quick fix #14138

Closed

microsoft locked and limited conversation to collaborators Jun 14, 2018

		@@ -100,7 +98,7 @@ namespace ts.codefix {
		// or "'import {a, b as ns} from './file'"

Conversation

minestarks commented Jul 10, 2017

Uh oh!

amcasey left a comment

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!

Choose a reason for hiding this comment

Uh oh!

minestarks Jul 10, 2017 • 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!

minestarks Jul 10, 2017 • 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!

aozgaa left a comment

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!

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!

amcasey left a comment

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!

minestarks Jul 10, 2017 •

edited

Loading

minestarks Jul 10, 2017 •

edited

Loading

minestarks Jul 11, 2017 •

edited

Loading