Organize imports moving imports before file header

[mjbvz]

TypeScript Version: 2.8.0-dev20180320

Search Terms:

• Organize imports
• vscode

Repo Steps

1. In the vscode source repo.
2. Open webview.ts
3. Run organize imports

Bug:

Imports split around file header comment:

import { addClass, addDisposableListener } from 'vs/base/browser/dom';
import { Emitter, Event } from 'vs/base/common/event';
import { IDisposable, dispose } from 'vs/base/common/lifecycle';
import { nativeSep } from 'vs/base/common/paths';
import { startsWith } from 'vs/base/common/strings';
/*---------------------------------------------------------------------------------------------
 *  Copyright (c) Microsoft Corporation. All rights reserved.
 *  Licensed under the MIT License. See License.txt in the project root for license information.
 *--------------------------------------------------------------------------------------------*/
import URI from 'vs/base/common/uri';
import { IContextKey } from 'vs/platform/contextkey/common/contextkey';
import { IContextViewService } from 'vs/platform/contextview/browser/contextView';
import { IEnvironmentService } from 'vs/platform/environment/common/environment';
import { editorBackground, editorForeground, textLinkForeground } from 'vs/platform/theme/common/colorRegistry';
import { DARK, ITheme, IThemeService, LIGHT } from 'vs/platform/theme/common/themeService';
import { WebviewFindWidget } from './webviewFindWidget';

[Trace  - 13:44:19] Response received: organizeImports (5142). Request took 14 ms. Success: true 
Result: [
    {
        "fileName": "/Users/matb/projects/vscode/src/vs/workbench/parts/html/electron-browser/webview.ts",
        "textChanges": [
            {
                "start": {
                    "line": 1,
                    "offset": 1
                },
                "end": {
                    "line": 7,
                    "offset": 1
                },
                "newText": "import { addClass, addDisposableListener } from 'vs/base/browser/dom';\nimport { Emitter, Event } from 'vs/base/common/event';\nimport { IDisposable, dispose } from 'vs/base/common/lifecycle';\nimport { nativeSep } from 'vs/base/common/paths';\nimport { startsWith } from 'vs/base/common/strings';\n/*---------------------------------------------------------------------------------------------\n *  Copyright (c) Microsoft Corporation. All rights reserved.\n *  Licensed under the MIT License. See License.txt in the project root for license information.\n *--------------------------------------------------------------------------------------------*/\nimport URI from 'vs/base/common/uri';\nimport { IContextKey } from 'vs/platform/contextkey/common/contextkey';\nimport { IContextViewService } from 'vs/platform/contextview/browser/contextView';\nimport { IEnvironmentService } from 'vs/platform/environment/common/environment';\nimport { editorBackground, editorForeground, textLinkForeground } from 'vs/platform/theme/common/colorRegistry';\nimport { DARK, ITheme, IThemeService, LIGHT } from 'vs/platform/theme/common/themeService';\nimport { WebviewFindWidget } from './webviewFindWidget';\n"
            },
            {
                "start": {
                    "line": 7,
                    "offset": 1
                },
                "end": {
                    "line": 8,
                    "offset": 1
                },
                "newText": ""
            },
            {
                "start": {
                    "line": 8,
                    "offset": 1
                },
                "end": {
                    "line": 9,
                    "offset": 1
                },
                "newText": ""
            },
            {
                "start": {
                    "line": 9,
                    "offset": 1
                },
                "end": {
                    "line": 10,
                    "offset": 1
                },
                "newText": ""
            },
            {
                "start": {
                    "line": 10,
                    "offset": 1
                },
                "end": {
                    "line": 11,
                    "offset": 1
                },
                "newText": ""
            },
            {
                "start": {
                    "line": 11,
                    "offset": 1
                },
                "end": {
                    "line": 12,
                    "offset": 1
                },
                "newText": ""
            },
            {
                "start": {
                    "line": 12,
                    "offset": 1
                },
                "end": {
                    "line": 13,
                    "offset": 1
                },
                "newText": ""
            },
            {
                "start": {
                    "line": 13,
                    "offset": 1
                },
                "end": {
                    "line": 14,
                    "offset": 1
                },
                "newText": ""
            },
            {
                "start": {
                    "line": 14,
                    "offset": 1
                },
                "end": {
                    "line": 15,
                    "offset": 1
                },
                "newText": ""
            },
            {
                "start": {
                    "line": 15,
                    "offset": 1
                },
                "end": {
                    "line": 16,
                    "offset": 1
                },
                "newText": ""
            },
            {
                "start": {
                    "line": 16,
                    "offset": 1
                },
                "end": {
                    "line": 17,
                    "offset": 1
                },
                "newText": ""
            },
            {
                "start": {
                    "line": 17,
                    "offset": 1
                },
                "end": {
                    "line": 18,
                    "offset": 1
                },
                "newText": ""
            }
        ]
    }
]

Playground Link:

Related Issues:

[amcasey]

Technically, they're following the import they were attached to as it moves down the list of imports. Either way, the behavior is undesirable.

[mjbvz]

This bug makes organize imports pretty unusable in the vscode code base. Not sure how many people will hit it as well. Can we please look into for 2.8.2

[amcasey]

It seems like it should be sufficient to suppress leading trivia on the (old) first import declaration and then leave the surrounding trivia in place when replacing or deleting it. However, that doesn't seem to be working. It looks like emitNodeWithNestedComments emits the comment regardless of whether it's suppressed, which I find surprising (obviously, not based on any documentation of its intended behavior). If that behavior is correct, then checking for the suppression flag in the caller is largely meaningless.

[amcasey]

@andy-ms @mhegazy if this rings any bells, please let me know. Otherwise, I'll keep stepping through the emitter until I figure out why those comments aren't being suppressed.

If this is urgent, we do have a stopgap solution - we can suppress all comments in the import declarations and leave the header where it is. Then, organizing imports will delete interior and trailing comments, but not touch the header. This may be an improvement, in practice.

[amcasey]

@weswigham agrees that this is probably a known deficiency in the emitter - the leading comments on the import keyword should not be emitted, but there's no system in place for checking the EmitFlags at that level. And, for future reference, emitNodeWithNestedComments should not emit leading or trailing comments - that's emitNodeWithComments's job.