- Notifications
You must be signed in to change notification settings - Fork12.9k
Add option for organize imports case sensitivity#51733
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Add option for organize imports case sensitivity#51733
Conversation
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. |
andrewbranch commentedDec 6, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
| } | ||
| /** @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
:yikes: nice catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
“less than or equal to” can’t be represented in a single value the way ourComparison system works:
exportconstenumComparison{LessThan=-1,EqualTo=0,GreaterThan=1}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah - just an idle sort of "it would be nice".
src/compiler/core.ts Outdated
| if (caseSensitiveComparer(prevElement, element) === Comparison.GreaterThan) { | ||
| kind &= ~SortKind.CaseSensitive; | ||
| } | ||
| if (caseInsensitiveComparer(prevElement, element) === Comparison.GreaterThan) { | ||
| kind &= ~SortKind.CaseInsensitive; | ||
| } | ||
| if (kind === SortKind.None) { | ||
| return kind; | ||
| } |
DanielRosenwasserDec 13, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(Quiz ChatGPT about this for a good laugh)
| } | ||
| return kind; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@typescript-bot user test tsserver |
typescript-bot commentedDec 13, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@DanielRosenwasser, I've started to run the diff-based user code test suite (tsserver) on this PR at4257fe4. You can monitor the buildhere. Update:The results are in! |
typescript-bot commentedDec 13, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@DanielRosenwasser, I've started to run the diff-based top-repos suite (tsserver) on this PR at4257fe4. You can monitor the buildhere. Update:The results are in! |
typescript-bot commentedDec 13, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@DanielRosenwasser, I've started to run the perf test suite on this PR at4257fe4. You can monitor the buildhere. |
@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
|
Fixes#51616
Related:#51579
A new user preference (editor PR to follow) allows setting the case sensitivity for import sorting. By default, we detect.