Add option for organize imports case sensitivity #51733
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
the
For Milestone Bug
|
I'm finding it hard to deduce from the tests, but what is the behavior when you have both This is something that frustrates me currently about the eslint plugin; it doesn't care which order you have, which leaves a hole where we can end up with conflicts when the result should not depend on the input ordering at all. |
sheetalkamat
approved these changes
Dec 6, 2022
|
Currently, case-insensitive is truly case-insensitive like eslint, which means any order between specifiers that differ only in case is allowed, and a reshuffle will preserve the pre-existing relative order (eslint doesn’t use a stable sort, which means pre-existing order between case-different specifiers is not preserved, resulting in sorts that look totally random). This is something we can definitely change or improve on over the eslint behavior, but if we change back to case sensitive sorting, I wasn’t sure if anyone would end up caring about it. |
jakebailey
reviewed
Dec 6, 2022
| } | ||
|
|
||
| /** @internal */ | ||
| export function getImportDeclarationInsertionIndex(sortedImports: SortedReadonlyArray<AnyImportOrRequireStatement>, newImport: AnyImportOrRequireStatement) { | ||
| const index = binarySearch(sortedImports, newImport, identity, compareImportsOrRequireStatements); | ||
| export const detectImportSpecifierSorting = memoizeWeak((specifiers: readonly ImportSpecifier[]): SortKind => { |
There was a problem hiding this comment.
Is there a particular reason why this one is cached but others aren't? Would everything benefit from being cached like this?
There was a problem hiding this comment.
As it stands now, this is the only one that benefits from being cached. It gets called several times from auto-imports in ways that are cumbersome to cache locally.
jakebailey
approved these changes
Dec 6, 2022
sandersn
reviewed
Dec 12, 2022
| let prevElement = array[0]; | ||
| for (const element of array.slice(1)) { | ||
| if (comparer(prevElement, element) === Comparison.GreaterThan) { | ||
| for (let i = 1, len = array.length; i < len; i++) { |
There was a problem hiding this comment.
@DanielRosenwasser noticed this while I was screensharing working on this. Turns out I wrote it years ago, before I was terrified of allocations 🙈
src/compiler/core.ts
Outdated
| for (let i = 1, len = array.length; i < len; i++) { | ||
| const prevElement = array[i - 1]; | ||
| const element = array[i]; | ||
| if (caseSensitiveComparer(prevElement, element) === Comparison.GreaterThan) { |
There was a problem hiding this comment.
It would be more obvious to me what was going on if this was something like !== Comparison.LessThanOrEqualTo
There was a problem hiding this comment.
“less than or equal to” can’t be represented in a single value the way our Comparison system works:
export const enum Comparison {
LessThan = -1,
EqualTo = 0,
GreaterThan = 1
}There was a problem hiding this comment.
Yeah - just an idle sort of "it would be nice".
src/compiler/core.ts
Outdated
Comment on lines
981
to
989
| if (caseSensitiveComparer(prevElement, element) === Comparison.GreaterThan) { | ||
| kind &= ~SortKind.CaseSensitive; | ||
| } | ||
| if (caseInsensitiveComparer(prevElement, element) === Comparison.GreaterThan) { | ||
| kind &= ~SortKind.CaseInsensitive; | ||
| } | ||
| if (kind === SortKind.None) { | ||
| return kind; | ||
| } |
There was a problem hiding this comment.
I think that you can optimize this.
- If you know that you're not case-sensitively sorted, you never have to check again; you can just always check the case-insensitive branch.
- If you are not case-insensitively sorted, you can just bail out at that point. You won't be case-sensitively sorted.
That means the only times you ever have to do two checks for a pair of elements is the first pair that forces you to transition to only checking for case-insensitive comparison.
There was a problem hiding this comment.
Good point on (1), but (2) is not true. ['B', 'a'] is not case-insensitively sorted but is case-sensitively sorted. (That’s the entire reason this is bit flags.)
It sure sounds like the “case-insensitively sorted lists” ought to be a superset of “case-sensitively sorted lists,” doesn’t it? They’re actually a classic Venn diagram shape where there is a non-empty intersection but neither is a subset of the other. When Jake was first trying to explain the problem to me verbally, I made the same mistake.
There was a problem hiding this comment.
Ugh, why was I so convinced this was true? But yes, I see that. At least you can avoid each kind of comparison after the first failure.
There was a problem hiding this comment.
(Quiz ChatGPT about this for a good laugh)
| } | ||
| return kind; | ||
| } | ||
|
|
|
@typescript-bot user test tsserver |
|
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite (tsserver) on this PR at 4257fe4. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite (tsserver) on this PR at 4257fe4. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 4257fe4. You can monitor the build here. |
|
@DanielRosenwasser Here are the results of running the user test suite comparing Everything looks good! |
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailsbackstage/backstage
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #51616
Related: #51579
A new user preference (editor PR to follow) allows setting the case sensitivity for import sorting. By default, we detect.