Handle different default export forms the same way in import code fixes by amcasey · Pull Request #21014 · microsoft/TypeScript

Andrew Casey (amcasey) · 2018-01-04T23:41:23Z

export default C and export { C as default } should be handled the same as export default class C { }.

Fixes #19115

`export default C` and `export { C as default }` should be handled the same as `export default class C { }`. Fixes microsoft#19115

The regular displayName had special handling for `as` clauses in imports and exports but the fullDisplayName did not. They should be consistent. (The impact of this change is very minor - it only affects the root node of the tree control in the rename preview, which is off by default.) Bonus: Eliminate `getDeclaredName` which is only called by rename. Note: Special handling of default exports was not preserved since `default` cannot be renamed.

Deleted user (ghost) · 2018-01-05T16:39:54Z

-                const displayName = stripQuotes(getDeclaredName(typeChecker, symbol, node));
                const kind = SymbolDisplay.getSymbolKind(typeChecker, symbol, node);
-                return kind ? getRenameInfoSuccess(displayName, typeChecker.getFullyQualifiedName(symbol), kind, SymbolDisplay.getSymbolModifiers(symbol), node, sourceFile) : undefined;
+                if (!kind) {


This change could use a test.

We don't have a good way to test that it shows up in the right place in the UI, but I can try to add a lower-level test of the rename info.

Just to clarify, what is the expected change to rename behavior due to this PR?

Different strings are displayed in VS dialogs. No impact on renaming.

There must be some function that returns a different value than before in some situation?

Yes, of course. And I believe I've found a fourslash helper for testing it. I just meant that the same user actions would result in the same result (i.e. renaming) after the change.

Deleted user (ghost) · 2018-01-05T16:40:26Z

@@ -0,0 +1,21 @@
+/// <reference path="fourslash.ts" />
+
+// @allowJs: true


Is it necessary that this test be JS? Should work exactly the same in TS?

This test matches the repro steps in the bug. If a TS test would be preferable, I can change it (it should be equivalent).

If it's exactly the same with these lines removed I would do that.

Assuming you also meant to update the file extensions (so that diagnostics are produced at all), it would still not be exactly the same since the suppression fixes would not be offered (though that distinction is not important to the test/change).

Yeah, I agree it's OK to remove the JS-specific suppression codefixes from this test since that's not what the point of the test is.

Deleted user (ghost) · 2018-01-05T16:41:40Z

        createSearch(location: Node, symbol: Symbol, comingFrom: ImportExport | undefined, searchOptions: { text?: string, allSearchSymbols?: Symbol[] } = {}): Search {
            // Note: if this is an external module symbol, the name doesn't include quotes.
+            // Note: getLocalSymbolForExportDefault handles `export default class C {}`, but not `export default C` or `export { C as default }`.
+            // The other two forms seem to be handled downstream (i.e. special-casing the first form here appears to be intentional).


This may be because we don't get a default export alias symbol here. See for example skipPastExportOrImportSpecifier.

Supplemented the comment with that information.

Deleted user (ghost) · 2018-01-05T16:42:53Z

+
+            function getEscapedNameForExportDefault(symbol: Symbol): __String | undefined {
+                const declarations = symbol.declarations;
+                if (length(declarations) > 0) {


Might was well return firstDefined(symbol.declarations, declaration => ...

That's not equivalent, is it? It'll check subsequent declarations if the lambda returns undefined for the first one?

You're right, it's not exactly equivalent, but I don't think it hurts to loop through declarations (and there is usually only one).

And the advantage is removing one bounds check?

If you like it this way that's fine, I just find it suspicious when we depend on declarations[0] being something particular, although that should be work in this case since the symbol for an export assignment or export specifier should always have exactly one declaration.

I can check that the length is one if that would be more robust.

I don't think that's necessary, it shouldn't really matter whether it has other declarations or not.

Deleted user (ghost) · 2018-01-08T16:12:03Z

@@ -0,0 +1,21 @@
+/// <reference path="fourslash.ts" />
+
+// @allowJs: true


This doesn't really need to be a JS test, right?

While I can no longer recall my original motivation, it appears to be a JS version of the TS test in importNameCodeFixDefaultExport2.ts.

Well, I don't think we need the same test twice -- the same services code will be tested either way.

Andrew Casey (amcasey) added 4 commits January 4, 2018 15:34

Handle different default export forms the same way in import code fixes

ff58f1f

`export default C` and `export { C as default }` should be handled the same as `export default class C { }`. Fixes microsoft#19115

Add regression tests

1e93967

Add explanatory comment to FAR call-site

f0d9cb3

Andrew Casey (amcasey) requested review from a user and Ryan Cavanaugh (RyanCavanaugh) January 4, 2018 23:41

Andrew Casey (amcasey) closed this Jan 5, 2018

Andrew Casey (amcasey) reopened this Jan 5, 2018

Deleted user (ghost) reviewed Jan 5, 2018

View reviewed changes

Andrew Casey (amcasey) added 4 commits January 5, 2018 10:53

Improve comment

a0495a6

Add regression tests for rename display name

13c26fe

Convert test from JS to TS

b07797c

Stop limiting getEscapedNameForExportDefault to the first declaration

18f9d69

Deleted user (ghost) approved these changes Jan 8, 2018

View reviewed changes

Deleted user (ghost) reviewed Jan 8, 2018

View reviewed changes

Mohamed Hegazy (mhegazy) approved these changes Jan 8, 2018

View reviewed changes

Andrew Casey (amcasey) merged commit ef5a289 into microsoft:master Jan 9, 2018

Andrew Casey (amcasey) deleted the GH19115 branch January 9, 2018 03:07

Andrew Casey (amcasey) mentioned this pull request Jan 9, 2018

Delete redundant test #21085

Merged

Microsoft (microsoft) locked and limited conversation to collaborators Jul 3, 2018

		@@ -0,0 +1,21 @@
		/// <reference path="fourslash.ts" />

		// @allowJs: true

Uh oh!

Conversation

Andrew Casey (amcasey) commented Jan 4, 2018

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants