Skip to content

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Dec 13, 2022
Merged

Add option for organize imports case sensitivity #51733

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6f21600
Add ignore case option to organizeImports
andrewbranch Nov 30, 2022
7ae6020
Adopt in auto-imports, use same case-insensitive comparison as eslint
andrewbranch Dec 2, 2022
d4d0844
Fix build/lint
andrewbranch Dec 5, 2022
a9eb9cf
Mark functions internal
andrewbranch Dec 5, 2022
289276b
Update affected auto import test
andrewbranch Dec 5, 2022
005c08d
Update API baseline
andrewbranch Dec 5, 2022
a3a6cd4
Update protocol
andrewbranch Dec 5, 2022
3c34aab
Update API baseline
andrewbranch Dec 6, 2022
54d350f
Merge branch 'main' into bug/organize-imports-case-sensitivity
andrewbranch Dec 13, 2022
4257fe4
Short-circuit comparisons that have already failed
andrewbranch Dec 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 80 additions & 4 deletions src/compiler/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -944,16 +944,54 @@ export function sortAndDeduplicate<T>(array: readonly T[], comparer?: Comparer<T
/** @internal */
export function arrayIsSorted<T>(array: readonly T[], comparer: Comparer<T>) {
if (array.length < 2) return true;
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++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:yikes: nice catch

Copy link
Member Author

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 🙈

if (comparer(array[i - 1], array[i]) === Comparison.GreaterThan) {
return false;
}
prevElement = element;
}
return true;
}

/** @internal */
export const enum SortKind {
None = 0,
CaseSensitive = 1 << 0,
CaseInsensitive = 1 << 1,
Both = CaseSensitive | CaseInsensitive,
}

/** @internal */
export function detectSortCaseSensitivity(array: readonly string[], useEslintOrdering?: boolean): SortKind;
/** @internal */
export function detectSortCaseSensitivity<T>(array: readonly T[], useEslintOrdering: boolean, getString: (element: T) => string): SortKind;
/** @internal */
export function detectSortCaseSensitivity<T>(array: readonly T[], useEslintOrdering: boolean, getString?: (element: T) => string): SortKind {
let kind = SortKind.Both;
if (array.length < 2) return kind;
const caseSensitiveComparer = getString
? (a: T, b: T) => compareStringsCaseSensitive(getString(a), getString(b))
: compareStringsCaseSensitive as (a: T | undefined, b: T | undefined) => Comparison;
const compareCaseInsensitive = useEslintOrdering ? compareStringsCaseInsensitiveEslintCompatible : compareStringsCaseInsensitive;
const caseInsensitiveComparer = getString
? (a: T, b: T) => compareCaseInsensitive(getString(a), getString(b))
: compareCaseInsensitive as (a: T | undefined, b: T | undefined) => Comparison;
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) {
Copy link
Member

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

Copy link
Member Author

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 our Comparison system works:

export const enum Comparison {
    LessThan    = -1,
    EqualTo     = 0,
    GreaterThan = 1
}

Copy link
Member

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".

kind &= ~SortKind.CaseSensitive;
}
if (caseInsensitiveComparer(prevElement, element) === Comparison.GreaterThan) {
kind &= ~SortKind.CaseInsensitive;
}
if (kind === SortKind.None) {
return kind;
}
Copy link
Member

@DanielRosenwasser DanielRosenwasser Dec 13, 2022 •
edited
Loading

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.

  1. 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.
  2. 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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change


/** @internal */
export function arrayIsEqualTo<T>(array1: readonly T[] | undefined, array2: readonly T[] | undefined, equalityComparer: (a: T, b: T, index: number) => boolean = equateValues): boolean {
if (!array1 || !array2) {
Expand Down Expand Up @@ -2144,6 +2182,23 @@ export function memoizeOne<A extends string | number | boolean | undefined, T>(c
};
}

/**
* A version of `memoize` that supports a single non-primitive argument, stored as keys of a WeakMap.
*
* @internal
*/
export function memoizeWeak<A extends object, T>(callback: (arg: A) => T): (arg: A) => T {
const map = new WeakMap<A, T>();
return (arg: A) => {
let value = map.get(arg);
if (value === undefined && !map.has(arg)) {
value = callback(arg);
map.set(arg, value);
}
return value!;
};
}

/**
* High-order function, composes functions. Note that functions are composed inside-out;
* for example, `compose(a, b)` is the equivalent of `x => b(a(x))`.
Expand Down Expand Up @@ -2293,6 +2348,27 @@ export function compareStringsCaseInsensitive(a: string, b: string) {
return a < b ? Comparison.LessThan : a > b ? Comparison.GreaterThan : Comparison.EqualTo;
}

/**
* `compareStringsCaseInsensitive` transforms letters to uppercase for unicode reasons,
* while eslint's `sort-imports` rule transforms letters to lowercase. Which one you choose
* affects the relative order of letters and ASCII characters 91-96, of which `_` is a
* valid character in an identifier. So if we used `compareStringsCaseInsensitive` for
* import sorting, TypeScript and eslint would disagree about the correct case-insensitive
* sort order for `__String` and `Foo`. Since eslint's whole job is to create consistency
* by enforcing nitpicky details like this, it makes way more sense for us to just adopt
* their convention so users can have auto-imports without making eslint angry.
*
* @internal
*/
export function compareStringsCaseInsensitiveEslintCompatible(a: string, b: string) {
if (a === b) return Comparison.EqualTo;
if (a === undefined) return Comparison.LessThan;
if (b === undefined) return Comparison.GreaterThan;
a = a.toLowerCase();
b = b.toLowerCase();
return a < b ? Comparison.LessThan : a > b ? Comparison.GreaterThan : Comparison.EqualTo;
}

/**
* Compare two strings using a case-sensitive ordinal comparison.
*
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9426,6 +9426,7 @@ export interface UserPreferences {
readonly includeInlayEnumMemberValueHints?: boolean;
readonly allowRenameOfImportPath?: boolean;
readonly autoImportFileExcludePatterns?: string[];
readonly organizeImportsIgnoreCase?: "auto" | boolean;
}

/** Represents a bigint literal value without requiring bigint support */
Expand Down
4 changes: 2 additions & 2 deletions src/harness/fourslashImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,8 +535,8 @@ export class TestState {
}
}

public verifyOrganizeImports(newContent: string, mode?: ts.OrganizeImportsMode) {
const changes = this.languageService.organizeImports({ fileName: this.activeFile.fileName, type: "file", mode }, this.formatCodeSettings, ts.emptyOptions);
public verifyOrganizeImports(newContent: string, mode?: ts.OrganizeImportsMode, preferences?: ts.UserPreferences) {
const changes = this.languageService.organizeImports({ fileName: this.activeFile.fileName, type: "file", mode }, this.formatCodeSettings, preferences);
this.applyChanges(changes);
this.verifyFileContent(this.activeFile.fileName, newContent);
}
Expand Down
4 changes: 2 additions & 2 deletions src/harness/fourslashInterfaceImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,8 +626,8 @@ export class Verify extends VerifyNegatable {
this.state.noMoveToNewFile();
}

public organizeImports(newContent: string, mode?: ts.OrganizeImportsMode): void {
this.state.verifyOrganizeImports(newContent, mode);
public organizeImports(newContent: string, mode?: ts.OrganizeImportsMode, preferences?: ts.UserPreferences): void {
this.state.verifyOrganizeImports(newContent, mode, preferences);
}
}

Expand Down
1 change: 1 addition & 0 deletions src/server/protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3515,6 +3515,7 @@ export interface UserPreferences {
readonly includeInlayFunctionLikeReturnTypeHints?: boolean;
readonly includeInlayEnumMemberValueHints?: boolean;
readonly autoImportFileExcludePatterns?: string[];
readonly organizeImportsIgnoreCase?: "auto" | boolean;

/**
* Indicates whether {@link ReferencesResponseItem.lineText} is supported.
Expand Down
64 changes: 47 additions & 17 deletions src/services/codefixes/importFixes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ import {
skipAlias,
some,
sort,
SortKind,
SourceFile,
stableSort,
startsWith,
Expand Down Expand Up @@ -164,15 +165,14 @@ registerCodeFix({
const { errorCode, preferences, sourceFile, span, program } = context;
const info = getFixInfos(context, errorCode, span.start, /*useAutoImportProvider*/ true);
if (!info) return undefined;
const quotePreference = getQuotePreference(sourceFile, preferences);
return info.map(({ fix, symbolName, errorIdentifierText }) => codeActionForFix(
context,
sourceFile,
symbolName,
fix,
/*includeSymbolNameInDescription*/ symbolName !== errorIdentifierText,
quotePreference,
program.getCompilerOptions()));
program.getCompilerOptions(),
preferences));
},
fixIds: [importFixId],
getAllCodeActions: context => {
Expand Down Expand Up @@ -358,7 +358,8 @@ function createImportAdderWorker(sourceFile: SourceFile, program: Program, useAu
importClauseOrBindingPattern,
defaultImport,
arrayFrom(namedImports.entries(), ([name, addAsTypeOnly]) => ({ addAsTypeOnly, name })),
compilerOptions);
compilerOptions,
preferences);
});

let newDeclarations: AnyImportOrRequireStatement | readonly AnyImportOrRequireStatement[] | undefined;
Expand Down Expand Up @@ -516,7 +517,8 @@ export function getImportCompletionAction(
symbolName,
fix,
/*includeSymbolNameInDescription*/ false,
getQuotePreference(sourceFile, preferences), compilerOptions))
compilerOptions,
preferences))
};
}

Expand All @@ -526,7 +528,7 @@ export function getPromoteTypeOnlyCompletionAction(sourceFile: SourceFile, symbo
const symbolName = single(getSymbolNamesToImport(sourceFile, program.getTypeChecker(), symbolToken, compilerOptions));
const fix = getTypeOnlyPromotionFix(sourceFile, symbolToken, symbolName, program);
const includeSymbolNameInDescription = symbolName !== symbolToken.text;
return fix && codeFixActionToCodeAction(codeActionForFix({ host, formatContext, preferences }, sourceFile, symbolName, fix, includeSymbolNameInDescription, QuotePreference.Double, compilerOptions));
return fix && codeFixActionToCodeAction(codeActionForFix({ host, formatContext, preferences }, sourceFile, symbolName, fix, includeSymbolNameInDescription, compilerOptions, preferences));
}

function getImportFixForSymbol(sourceFile: SourceFile, exportInfos: readonly SymbolExportInfo[], moduleSymbol: Symbol, program: Program, useNamespaceInfo: { position: number, symbolName: string } | undefined, isValidTypeOnlyUseSite: boolean, useRequire: boolean, host: LanguageServiceHost, preferences: UserPreferences) {
Expand Down Expand Up @@ -1175,14 +1177,15 @@ function getExportEqualsImportKind(importingFile: SourceFile, compilerOptions: C
return allowSyntheticDefaults ? ImportKind.Default : ImportKind.CommonJS;
}

function codeActionForFix(context: textChanges.TextChangesContext, sourceFile: SourceFile, symbolName: string, fix: ImportFix, includeSymbolNameInDescription: boolean, quotePreference: QuotePreference, compilerOptions: CompilerOptions): CodeFixAction {
function codeActionForFix(context: textChanges.TextChangesContext, sourceFile: SourceFile, symbolName: string, fix: ImportFix, includeSymbolNameInDescription: boolean, compilerOptions: CompilerOptions, preferences: UserPreferences): CodeFixAction {
let diag!: DiagnosticAndArguments;
const changes = textChanges.ChangeTracker.with(context, tracker => {
diag = codeActionForFixWorker(tracker, sourceFile, symbolName, fix, includeSymbolNameInDescription, quotePreference, compilerOptions);
diag = codeActionForFixWorker(tracker, sourceFile, symbolName, fix, includeSymbolNameInDescription, compilerOptions, preferences);
});
return createCodeFixAction(importFixName, changes, diag, importFixId, Diagnostics.Add_all_missing_imports);
}
function codeActionForFixWorker(changes: textChanges.ChangeTracker, sourceFile: SourceFile, symbolName: string, fix: ImportFix, includeSymbolNameInDescription: boolean, quotePreference: QuotePreference, compilerOptions: CompilerOptions): DiagnosticAndArguments {
function codeActionForFixWorker(changes: textChanges.ChangeTracker, sourceFile: SourceFile, symbolName: string, fix: ImportFix, includeSymbolNameInDescription: boolean, compilerOptions: CompilerOptions, preferences: UserPreferences): DiagnosticAndArguments {
const quotePreference = getQuotePreference(sourceFile, preferences);
switch (fix.kind) {
case ImportFixKind.UseNamespace:
addNamespaceQualifier(changes, sourceFile, fix);
Expand All @@ -1198,7 +1201,8 @@ function codeActionForFixWorker(changes: textChanges.ChangeTracker, sourceFile:
importClauseOrBindingPattern,
importKind === ImportKind.Default ? { name: symbolName, addAsTypeOnly } : undefined,
importKind === ImportKind.Named ? [{ name: symbolName, addAsTypeOnly }] : emptyArray,
compilerOptions);
compilerOptions,
preferences);
const moduleSpecifierWithoutQuotes = stripQuotes(moduleSpecifier);
return includeSymbolNameInDescription
? [Diagnostics.Import_0_from_1, symbolName, moduleSpecifierWithoutQuotes]
Expand Down Expand Up @@ -1239,10 +1243,11 @@ function promoteFromTypeOnly(changes: textChanges.ChangeTracker, aliasDeclaratio
switch (aliasDeclaration.kind) {
case SyntaxKind.ImportSpecifier:
if (aliasDeclaration.isTypeOnly) {
if (aliasDeclaration.parent.elements.length > 1 && OrganizeImports.importSpecifiersAreSorted(aliasDeclaration.parent.elements)) {
const sortKind = OrganizeImports.detectImportSpecifierSorting(aliasDeclaration.parent.elements);
if (aliasDeclaration.parent.elements.length > 1 && sortKind) {
changes.delete(sourceFile, aliasDeclaration);
const newSpecifier = factory.updateImportSpecifier(aliasDeclaration, /*isTypeOnly*/ false, aliasDeclaration.propertyName, aliasDeclaration.name);
const insertionIndex = OrganizeImports.getImportSpecifierInsertionIndex(aliasDeclaration.parent.elements, newSpecifier);
const insertionIndex = OrganizeImports.getImportSpecifierInsertionIndex(aliasDeclaration.parent.elements, newSpecifier, sortKind === SortKind.CaseInsensitive);
changes.insertImportSpecifierAtIndex(sourceFile, newSpecifier, aliasDeclaration.parent, insertionIndex);
}
else {
Expand Down Expand Up @@ -1273,7 +1278,7 @@ function promoteFromTypeOnly(changes: textChanges.ChangeTracker, aliasDeclaratio
if (convertExistingToTypeOnly) {
const namedImports = tryCast(importClause.namedBindings, isNamedImports);
if (namedImports && namedImports.elements.length > 1) {
if (OrganizeImports.importSpecifiersAreSorted(namedImports.elements) &&
if (OrganizeImports.detectImportSpecifierSorting(namedImports.elements) &&
aliasDeclaration.kind === SyntaxKind.ImportSpecifier &&
namedImports.elements.indexOf(aliasDeclaration) !== 0
) {
Expand All @@ -1299,6 +1304,7 @@ function doAddExistingFix(
defaultImport: Import | undefined,
namedImports: readonly Import[],
compilerOptions: CompilerOptions,
preferences: UserPreferences,
): void {
if (clause.kind === SyntaxKind.ObjectBindingPattern) {
if (defaultImport) {
Expand Down Expand Up @@ -1326,21 +1332,45 @@ function doAddExistingFix(
}

if (namedImports.length) {
// sort case sensitivity:
// - if the user preference is explicit, use that
// - otherwise, if there are enough existing import specifiers in this import to detect unambiguously, use that
// - otherwise, detect from other imports in the file
let ignoreCaseForSorting: boolean | undefined;
if (typeof preferences.organizeImportsIgnoreCase === "boolean") {
ignoreCaseForSorting = preferences.organizeImportsIgnoreCase;
}
else if (existingSpecifiers) {
const targetImportSorting = OrganizeImports.detectImportSpecifierSorting(existingSpecifiers);
if (targetImportSorting !== SortKind.Both) {
ignoreCaseForSorting = targetImportSorting === SortKind.CaseInsensitive;
}
}
if (ignoreCaseForSorting === undefined) {
ignoreCaseForSorting = OrganizeImports.detectSorting(sourceFile) === SortKind.CaseInsensitive;
}

const newSpecifiers = stableSort(
namedImports.map(namedImport => factory.createImportSpecifier(
(!clause.isTypeOnly || promoteFromTypeOnly) && needsTypeOnly(namedImport),
/*propertyName*/ undefined,
factory.createIdentifier(namedImport.name))),
OrganizeImports.compareImportOrExportSpecifiers);

if (existingSpecifiers?.length && OrganizeImports.importSpecifiersAreSorted(existingSpecifiers)) {
(s1, s2) => OrganizeImports.compareImportOrExportSpecifiers(s1, s2, ignoreCaseForSorting));

// The sorting preference computed earlier may or may not have validated that these particular
// import specifiers are sorted. If they aren't, `getImportSpecifierInsertionIndex` will return
// nonsense. So if there are existing specifiers, even if we know the sorting preference, we
// need to ensure that the existing specifiers are sorted according to the preference in order
// to do a sorted insertion.
const specifierSort = existingSpecifiers?.length && OrganizeImports.detectImportSpecifierSorting(existingSpecifiers);
if (specifierSort && !(ignoreCaseForSorting && specifierSort === SortKind.CaseSensitive)) {
for (const spec of newSpecifiers) {
// Organize imports puts type-only import specifiers last, so if we're
// adding a non-type-only specifier and converting all the other ones to
// type-only, there's no need to ask for the insertion index - it's 0.
const insertionIndex = convertExistingToTypeOnly && !spec.isTypeOnly
? 0
: OrganizeImports.getImportSpecifierInsertionIndex(existingSpecifiers, spec);
: OrganizeImports.getImportSpecifierInsertionIndex(existingSpecifiers, spec, ignoreCaseForSorting);
changes.insertImportSpecifierAtIndex(sourceFile, spec, clause.namedBindings as NamedImports, insertionIndex);
}
}
Expand Down
Loading