Skip to content

Commit

Permalink
Add option for organize imports case sensitivity (#51733)
Browse files Browse the repository at this point in the history
* Add ignore case option to organizeImports

* Adopt in auto-imports, use same case-insensitive comparison as eslint

* Fix build/lint

* Mark functions internal

* Update affected auto import test

* Update API baseline

* Update protocol

* Update API baseline

* Short-circuit comparisons that have already failed
  • Loading branch information
andrewbranch authored Dec 13, 2022
1 parent a5dde88 commit 4076ff8
Show file tree
Hide file tree
Showing 19 changed files with 431 additions and 110 deletions.
83 changes: 79 additions & 4 deletions src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -944,16 +944,53 @@ 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++) {
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 (kind & SortKind.CaseSensitive && caseSensitiveComparer(prevElement, element) === Comparison.GreaterThan) {
kind &= ~SortKind.CaseSensitive;
}
if (kind & SortKind.CaseInsensitive && caseInsensitiveComparer(prevElement, element) === Comparison.GreaterThan) {
kind &= ~SortKind.CaseInsensitive;
}
if (kind === SortKind.None) {
return kind;
}
}
return kind;
}

/** @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 +2181,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 +2347,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
Original file line number Diff line number Diff line change
Expand Up @@ -9740,6 +9740,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
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
Original file line number Diff line number Diff line change
Expand Up @@ -630,8 +630,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
Original file line number Diff line number Diff line change
Expand Up @@ -3516,6 +3516,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
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 @@ -1176,14 +1178,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 @@ -1199,7 +1202,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 @@ -1240,10 +1244,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 @@ -1274,7 +1279,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 @@ -1300,6 +1305,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 @@ -1327,21 +1333,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

0 comments on commit 4076ff8

Please sign in to comment.