Organize imports collation #52115

rbuckton · 2023-01-05T04:07:11Z

After some internal discussion about #51916 and #52090, I took a brief look at providing more control over the sorting behavior for "Organize imports" by adding some additional configuration options to UserPreferences:

organizeImportsCollation - Indicates whether to perform "ordinal" (binary) sorting (i.e., by the numeric value associated with each code point), or "unicode" collation (using the Unicode Collation Algorithm/Intl.Collator). Default: "ordinal"
organizeImportsLocale - Overrides the locale used for collation. Specify "auto" to use the UI locale. Only applies to organizeImportsCollation: "unicode". Default: "en"
organizeImportsNumericCollation - Indicates whether to collate decimal numeric strings by their integer value (i.e., "a1" < "a2" < "a100") instead of just their code points (i.e., "a1" < "a100" < "a2"). Only applies to organizeImportsCollation: "unicode". Default: true
organizeImportsAccentCollation - Indicates whether to compare characters with accents or other diacritic marks as unequal from the base character. Only applies to organizeImportsCollation: "unicode". Default: true.
organizeImportsCaseFirst - Either "upper" to collate upper-case characters before lower-case, "lower" to collate lower-case characters before upper-case, or false to collate using the default for the locale. Only applies to organizeImportsCollation: "unicode". Default: false.

Each of these settings correlate to options passed to Intl.Collator. The organizeImportsNumericCollation and organizeImportsCaseFirst properties correspond to the numeric and caseFirst options to Intl.Collator. The combination of organizeImportsIgnoreCase and organizeImportsAccentCollation are used to determine the sensitivity of the collator:

`organizeImportsIgnoreCase`	`organizeImportsAccentCollation`	`sensitivity`
`true`	`false`	`"base"`
`true`	`true`	`"accent"`
`false`	`false`	`"case"`
`false`	`true`	`"variant"`

The default behavior is to perform an "ordinal" comparison in a way that is compatible with eslint's sort-imports rule, which is our current behavior. These collation rules only apply to import and export specifiers. The import statements themselves are sorted only by path, which continues to use "ordinal" sorting behavior.

These rules will also apply to auto-imports and import fixes, which will use the user's collation preferences to determine if a list of import or export specifiers is already sorted before selecting an insertion point.

These settings are not yet part of the VS Code settings schema, but can still be enabled using either the typescript.unstable or javascript.unstable configuration keys in settings.json:

{
  "typescript.unstable": {
    "organizeImportsCollation": "unicode",
    "organizeImportsCaseFirst": "upper"
  }
}

typescript-bot · 2023-01-05T04:07:14Z

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @mjbvz, and @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

jakebailey · 2023-01-05T18:21:29Z

The main thing I'll want to test is that it's possible to use sort-imports or simple-import-format (paired with some organize setting) and have organize be a no-op; that to me feels like the main blocker in that I know much of the team doesn't want to have ESLint enabled in the editor, so something has to get things right enough to not require a --fix (in which case, may as well not even do anything).

I do wonder how much of this we can detect within the file; if I have a file sorted via one of these methods, it'd be cool to not have to configure anything and let it be auto. That would work in our codebase as only new/ambiguous files would conflict with a lint rule, which I think would be pretty rare.

rbuckton · 2023-01-05T18:59:35Z

The main thing I'll want to test is that it's possible to use sort-imports or simple-import-format (paired with some organize setting) and have organize be a no-op; that to me feels like the main blocker in that I know much of the team doesn't want to have ESLint enabled in the editor, so something has to get things right enough to not require a --fix (in which case, may as well not even do anything).

Ideally, the configuration options should be enough to match compatibility though I may have to extend it slightly to fall back to ordinal comparisons depending on sensitivity so as to match rules like eslint-plugin-simple-import-sorting.

The default behavior ("ordinal") matches our current behavior in that it uses "eslint compatible" ordinal comparisons (where case-insensitivity/pseudo-case-folding is performed via .toLower() rather than .toUpper()).

I do wonder how much of this we can detect within the file; if I have a file sorted via one of these methods, it'd be cool to not have to configure anything and let it be auto. That would work in our codebase as only new/ambiguous files would conflict with a lint rule, which I think would be pretty rare.

For now, I've chosen to mostly leave sort detection alone (in that it only tests for case sensitive vs. case insensitive), except that it now uses the collation settings provided. I did experiment with collation settings detection, but I felt it was a bit unreliable since there are a lot of moving parts (such as determining the collation locale).

jakebailey · 2023-01-13T19:39:19Z

I finally had a chance to test this; it looks like (probably intentionally), using your example config produces an output that matches simple-import-sort (besides separating out blocks), which is awesome!

I've also tested this PR with #52090 applied to #51590 and it also works as expected. Very cool.

jakebailey · 2023-01-13T21:21:51Z

Hm, here's one case it differs. Organize says:

import {
    Baseline,
    Compiler,
    FileBasedTest,
    FileBasedTestConfiguration,
    getFileBasedTestConfigurationDescription,
    getFileBasedTestConfigurations,
    IO,
    RunnerBase,
    TestCaseParser,
    TestRunnerKind,
} from "./_namespaces/Harness";
import * as Utils from "./_namespaces/Utils";
import * as compiler from "./_namespaces/compiler";
import * as ts from "./_namespaces/ts";
import * as vpath from "./_namespaces/vpath";

But simple-import-sort says:

import * as compiler from "./_namespaces/compiler";
import {
    Baseline,
    Compiler,
    FileBasedTest,
    FileBasedTestConfiguration,
    getFileBasedTestConfigurationDescription,
    getFileBasedTestConfigurations,
    IO,
    RunnerBase,
    TestCaseParser,
    TestRunnerKind,
} from "./_namespaces/Harness";
import * as ts from "./_namespaces/ts";
import * as Utils from "./_namespaces/Utils";
import * as vpath from "./_namespaces/vpath";

which seems correct. It doesn't seem like this PR is applying the rules to the paths?

rbuckton · 2023-01-18T19:00:00Z

[...] It doesn't seem like this PR is applying the rules to the paths?

No, it isn't. I can, but wasn't sure if these rules made sense for paths as opposed to identifiers, so I left path comparisons as-is.

rbuckton · 2023-01-19T16:53:38Z

I've updated the PR to apply collation settings to paths as well.

andrewbranch

One option for exposing these settings in VS Code could be to make a single dropdown of “presets” that match popular formatters / lint rules 🤷

tests/baselines/reference/api/tsserverlibrary.d.ts

rbuckton · 2023-01-19T20:16:20Z

I started looking at detection of collation settings, in the same vein as detectSortCaseSensitivity, but I may wait on applying that.

rbuckton · 2023-01-19T20:17:21Z

Hm, here's one case it differs. Organize says: [...]

@jakebailey, can you check if the current version now aligns with simple-import-sort?

jakebailey · 2023-01-19T20:36:57Z

That example appears to be fixed, yeah, thanks! One thing I did notice is that in order to get numeric sorting to match (sort transformES5 before transformES2015), I also had to explicitly set:

"organizeImportsNumericCollation": true

But the doc comment seems to imply that it should be the default? (Maybe false is the default and the comment is outdated, or vice versa?)

src/services/organizeImports.ts

jakebailey · 2023-01-19T22:02:13Z

Hm, just retested with:

    "typescript.unstable": {
        "organizeImportsCollation": "unicode",
        "organizeImportsCaseFirst": "upper",
        "organizeImportsNumericCollation": true,
    },

And I'm having trouble getting this to organize:

import {
    hasDecorators,
    HasDecorators,
} ...

HasDecorators should come first (to match simple-import-sort), but organize seems to accept it as sorted. Am I missing some option?

jakebailey · 2023-01-19T22:33:44Z

Just to finish this off, these are the settings now to make organize match simple-import-format:

    "typescript.unstable": {
        "organizeImportsCollation": "unicode",
        "organizeImportsCaseFirst": "upper",
        "organizeImportsIgnoreCase": false,
        "organizeImportsNumericCollation": true,
    },

rbuckton · 2023-01-19T22:35:25Z

[...] I'm having trouble getting this to organize [...]

Discussed offline and the issue was the default setting for organizeImportsIgnoreCase is "auto", which auto-detects case sensitivity from context. Since hasDecorators, HasDecorators is a valid case-insensitive sort order, it acted as if organizeImportsIgnoreCase was true. The organizeImportsCaseFirst setting is only respected when perform a case-sensitive sort.

rubiesonthesky · 2023-02-03T20:42:01Z

Just to finish this off, these are the settings now to make organize match simple-import-format:

    "typescript.unstable": {
        "organizeImportsCollation": "unicode",
        "organizeImportsCaseFirst": "upper",
        "organizeImportsIgnoreCase": false,
        "organizeImportsNumericCollation": true,
    },

There is still small inconsistent issue between Typescript's Organize imports and simple-import-sort plugin. Since this repository does not use (usually) dots in filenames, I don't think it will be problem for this repository, though.

Using VS Code Insiders with Typescript nightly and Typescript set to version 5.0.0-dev.20230202.

Organize imports

import {TreeHelperUtil} from '../utils/tree-helper.util';
import {TreeUtil} from '../utils/tree.util';

Eslint simple-import-sort

import {TreeUtil} from '../utils/tree.util';
import {TreeHelperUtil} from '../utils/tree-helper.util';

jakebailey · 2023-02-03T20:46:45Z

I believe that you're describing lydell/eslint-plugin-simple-import-sort#127.

Our general goal is to get the configurability (and behavior of various tools) to the point that we can offer up some presets that do "the right thing" for that plugin, dprint, etc, since this impacts auto-imports and it's nice to not have to immediately quick fix an eslint error.

elboletaire · 2023-11-14T14:32:36Z

I was about to open a bug as a regression of #23366, but after searching quite a lot I've found out these pull requests with these secret settings, and setting organizeImportsIgnoreCase to true seems to do what I want (note: using auto doesn't seem to be working well... since leaving it to "auto" was changing my imports from ignoring case to not ignoring it).

mbest · 2023-11-30T02:32:36Z

I'm seeing similar behavior. The detection is very inconsistent and seems to default to case sensitive. In version 4.x, organize imports always used case-insensitive sorting (see #24048).

rbuckton added 3 commits January 4, 2023 18:08

Add configurable collation settings for 'organize imports'

6100acd

Auto-imports should use collation settings

8aaaba4

Fix failing tests, update baselines

b083508

rbuckton requested review from andrewbranch, jakebailey and RyanCavanaugh January 5, 2023 04:07

typescript-bot assigned rbuckton Jan 5, 2023

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 5, 2023

rbuckton added 2 commits January 4, 2023 23:07

Use 'unicode' instead of 'natural' for 'organizeImportsCollation'

d022b95

Address lint errors

7244b3f

Apply collation to paths as well

72678a3

rbuckton added 3 commits January 19, 2023 11:58

Merge branch 'main' into organize-imports-collation

c6c7ace

Fix lint errors

e60cb49

Reintroduce early exit shortcut to detectSortCaseSensitivity

12cca65

andrewbranch approved these changes Jan 19, 2023

View reviewed changes

jakebailey reviewed Jan 19, 2023

View reviewed changes

tests/baselines/reference/api/tsserverlibrary.d.ts Outdated Show resolved Hide resolved

jakebailey reviewed Jan 19, 2023

View reviewed changes

tests/baselines/reference/api/tsserverlibrary.d.ts Outdated Show resolved Hide resolved

Fix comments

d7de752

jakebailey reviewed Jan 19, 2023

View reviewed changes

src/services/organizeImports.ts Show resolved Hide resolved

rbuckton force-pushed the organize-imports-collation branch from eeca0b7 to a068f34 Compare January 19, 2023 22:31

Fix comment

1d54f35

rbuckton force-pushed the organize-imports-collation branch from a068f34 to 1d54f35 Compare January 19, 2023 22:32

jakebailey approved these changes Jan 19, 2023

View reviewed changes

rbuckton merged commit 20182cf into main Jan 19, 2023

rbuckton deleted the organize-imports-collation branch January 19, 2023 22:53

This was referenced Jan 20, 2023

Use eslint-plugin-simple-import-sort #52090

Merged

Sorting of "type FooX" and "Foo" imports disagrees with typescript, dprint lydell/eslint-plugin-simple-import-sort#124

Closed

DanielRosenwasser mentioned this pull request Jan 27, 2023

Design Meeting Notes, 1/25/2023 #52469

Closed

jakebailey mentioned this pull request Feb 17, 2023

Support customization of where type imports are placed in organize/sort imports #52820

Closed

iisaduan mentioned this pull request Jan 9, 2024

Organize type imports #55269

Merged

iisaduan mentioned this pull request Mar 28, 2024

Change detection in organizeImports #57267

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Organize imports collation #52115

Organize imports collation #52115

rbuckton commented Jan 5, 2023

typescript-bot commented Jan 5, 2023

jakebailey commented Jan 5, 2023

rbuckton commented Jan 5, 2023

jakebailey commented Jan 13, 2023

jakebailey commented Jan 13, 2023

rbuckton commented Jan 18, 2023 •

edited

Loading

rbuckton commented Jan 19, 2023

andrewbranch left a comment

rbuckton commented Jan 19, 2023

rbuckton commented Jan 19, 2023

jakebailey commented Jan 19, 2023

jakebailey commented Jan 19, 2023 •

edited

Loading

jakebailey commented Jan 19, 2023

rbuckton commented Jan 19, 2023

rubiesonthesky commented Feb 3, 2023

jakebailey commented Feb 3, 2023 •

edited

Loading

elboletaire commented Nov 14, 2023

mbest commented Nov 30, 2023 •

edited

Loading

Organize imports collation #52115

Organize imports collation #52115

Conversation

rbuckton commented Jan 5, 2023

typescript-bot commented Jan 5, 2023

jakebailey commented Jan 5, 2023

rbuckton commented Jan 5, 2023

jakebailey commented Jan 13, 2023

jakebailey commented Jan 13, 2023

rbuckton commented Jan 18, 2023 • edited Loading

rbuckton commented Jan 19, 2023

andrewbranch left a comment

Choose a reason for hiding this comment

rbuckton commented Jan 19, 2023

rbuckton commented Jan 19, 2023

jakebailey commented Jan 19, 2023

jakebailey commented Jan 19, 2023 • edited Loading

jakebailey commented Jan 19, 2023

rbuckton commented Jan 19, 2023

rubiesonthesky commented Feb 3, 2023

jakebailey commented Feb 3, 2023 • edited Loading

elboletaire commented Nov 14, 2023

mbest commented Nov 30, 2023 • edited Loading

rbuckton commented Jan 18, 2023 •

edited

Loading

jakebailey commented Jan 19, 2023 •

edited

Loading

jakebailey commented Feb 3, 2023 •

edited

Loading

mbest commented Nov 30, 2023 •

edited

Loading