diff --git a/pkg/analysis_server/lib/src/services/correction/organize_imports.dart b/pkg/analysis_server/lib/src/services/correction/organize_imports.dart index 72d40dd1e835..78bb80279fbf 100644 --- a/pkg/analysis_server/lib/src/services/correction/organize_imports.dart +++ b/pkg/analysis_server/lib/src/services/correction/organize_imports.dart @@ -14,9 +14,11 @@ import 'package:analyzer/src/error/codes.dart'; import 'package:analyzer/src/ignore_comments/ignore_info.dart'; import 'package:analyzer_plugin/protocol/protocol_common.dart' hide AnalysisError, Element; +import 'package:analyzer_plugin/src/utilities/directive_sort.dart'; import 'package:meta/meta_meta.dart'; -/// Organizer of imports (and other directives) in the [unit]. +/// Organizes imports (and other directives) in the [unit], using sorting +/// rules from [DirectiveSorter]. class ImportOrganizer { final String initialCode; @@ -84,75 +86,81 @@ class ImportOrganizer { // text for the computed DirectiveInfo and also its range for replacement // in the document. int? libraryDocsAndAnnotationsEndOffset; - var priority = _getDirectivePriority(directive); - if (priority != null) { - var offset = directive.offset; - var end = directive.end; - - var isPseudoLibraryDirective = - !hasLibraryDirective && directive == unit.directives.first; - Annotation? lastLibraryAnnotation; - if (isPseudoLibraryDirective) { - // Find the last library-level annotation that does not come - // after any non-library annotation. If there are already - // non-library annotations before library annotations, we will not - // try to correct those. - lastLibraryAnnotation = directive.metadata - .takeWhile(_isLibraryTargetAnnotation) - .lastOrNull; - - // If there is no annotation, use the end of the doc text (since the - // doc text is considered library-level here). - libraryDocsAndAnnotationsEndOffset = lastLibraryAnnotation?.end ?? - directive.documentationComment?.end; - - // Fix up the offset to be after the line end. - if (libraryDocsAndAnnotationsEndOffset != null) { - libraryDocsAndAnnotationsEndOffset = lineInfo - .getOffsetOfLineAfter(libraryDocsAndAnnotationsEndOffset); - // In the case of a blank line after the annotation/doc text - // we should include that in the library part. Otherwise it will - // be included in the top of the following directive and may - // result in an extra blank line in the annotation block if it - // is moved. - var nextLineOffset = lineInfo - .getOffsetOfLineAfter(libraryDocsAndAnnotationsEndOffset); - if (code - .substring(libraryDocsAndAnnotationsEndOffset, nextLineOffset) - .trim() - .isEmpty) { - libraryDocsAndAnnotationsEndOffset = nextLineOffset; - } + var uriContent = directive.uri.stringValue ?? ''; + var priority = switch (directive) { + ImportDirective() => + DirectiveSortPriority(uriContent, DirectiveSortKind.import), + ExportDirective() => + DirectiveSortPriority(uriContent, DirectiveSortKind.export), + PartDirective() => + DirectiveSortPriority(uriContent, DirectiveSortKind.part), + }; + + var offset = directive.offset; + var end = directive.end; + + var isPseudoLibraryDirective = + !hasLibraryDirective && directive == unit.directives.first; + Annotation? lastLibraryAnnotation; + if (isPseudoLibraryDirective) { + // Find the last library-level annotation that does not come + // after any non-library annotation. If there are already + // non-library annotations before library annotations, we will not + // try to correct those. + lastLibraryAnnotation = directive.metadata + .takeWhile(_isLibraryTargetAnnotation) + .lastOrNull; + + // If there is no annotation, use the end of the doc text (since the + // doc text is considered library-level here). + libraryDocsAndAnnotationsEndOffset = + lastLibraryAnnotation?.end ?? directive.documentationComment?.end; + + // Fix up the offset to be after the line end. + if (libraryDocsAndAnnotationsEndOffset != null) { + libraryDocsAndAnnotationsEndOffset = lineInfo + .getOffsetOfLineAfter(libraryDocsAndAnnotationsEndOffset); + // In the case of a blank line after the annotation/doc text + // we should include that in the library part. Otherwise it will + // be included in the top of the following directive and may + // result in an extra blank line in the annotation block if it + // is moved. + var nextLineOffset = lineInfo + .getOffsetOfLineAfter(libraryDocsAndAnnotationsEndOffset); + if (code + .substring(libraryDocsAndAnnotationsEndOffset, nextLineOffset) + .trim() + .isEmpty) { + libraryDocsAndAnnotationsEndOffset = nextLineOffset; } } + } - // Usually we look for leading comments on the directive. However if - // some library annotations were trimmed off, those comments are part - // of that and should not also be included here. - var leadingToken = - lastLibraryAnnotation == null ? directive.beginToken : null; - var leadingComment = leadingToken != null - ? getLeadingComment(unit, leadingToken, lineInfo, - isPseudoLibraryDirective: isPseudoLibraryDirective) - : null; - var trailingComment = getTrailingComment(unit, directive, lineInfo); - - if (leadingComment != null && leadingToken != null) { - offset = libraryDocsAndAnnotationsEndOffset != null - ? math.max( - libraryDocsAndAnnotationsEndOffset, leadingComment.offset) - : leadingComment.offset; - } - if (trailingComment != null) { - end = trailingComment.end; - } - offset = libraryDocsAndAnnotationsEndOffset ?? offset; - var text = code.substring(offset, end); - var uriContent = directive.uri.stringValue ?? ''; - directives.add( - _DirectiveInfo(directive, priority, uriContent, offset, end, text), - ); + // Usually we look for leading comments on the directive. However if + // some library annotations were trimmed off, those comments are part + // of that and should not also be included here. + var leadingToken = + lastLibraryAnnotation == null ? directive.beginToken : null; + var leadingComment = leadingToken != null + ? getLeadingComment(unit, leadingToken, lineInfo, + isPseudoLibraryDirective: isPseudoLibraryDirective) + : null; + var trailingComment = getTrailingComment(unit, directive, lineInfo); + + if (leadingComment != null && leadingToken != null) { + offset = libraryDocsAndAnnotationsEndOffset != null + ? math.max( + libraryDocsAndAnnotationsEndOffset, leadingComment.offset) + : leadingComment.offset; } + if (trailingComment != null) { + end = trailingComment.end; + } + offset = libraryDocsAndAnnotationsEndOffset ?? offset; + var text = code.substring(offset, end); + directives.add( + _DirectiveInfo(directive, priority, uriContent, offset, end, text), + ); } } // nothing to do @@ -168,7 +176,7 @@ class ImportOrganizer { String directivesCode; { var sb = StringBuffer(); - _DirectivePriority? currentPriority; + DirectiveSortPriority? currentPriority; var previousDirectiveText = ''; for (var directiveInfo in directives) { if (!hasUnresolvedIdentifierError) { @@ -293,37 +301,6 @@ class ImportOrganizer { return null; } - static _DirectivePriority? _getDirectivePriority( - UriBasedDirective directive) { - var uriContent = directive.uri.stringValue ?? ''; - if (directive is ImportDirective) { - if (uriContent.startsWith('dart:')) { - return _DirectivePriority.IMPORT_SDK; - } else if (uriContent.startsWith('package:')) { - return _DirectivePriority.IMPORT_PKG; - } else if (uriContent.contains('://')) { - return _DirectivePriority.IMPORT_OTHER; - } else { - return _DirectivePriority.IMPORT_REL; - } - } - if (directive is ExportDirective) { - if (uriContent.startsWith('dart:')) { - return _DirectivePriority.EXPORT_SDK; - } else if (uriContent.startsWith('package:')) { - return _DirectivePriority.EXPORT_PKG; - } else if (uriContent.contains('://')) { - return _DirectivePriority.EXPORT_OTHER; - } else { - return _DirectivePriority.EXPORT_REL; - } - } - if (directive is PartDirective) { - return _DirectivePriority.PART; - } - return null; - } - /// Returns whether this token is a '// ignore:' comment (but not an /// '// ignore_for_file:' comment). static bool _isIgnoreComment(Token token) => @@ -336,7 +313,7 @@ class ImportOrganizer { class _DirectiveInfo implements Comparable<_DirectiveInfo> { final UriBasedDirective directive; - final _DirectivePriority priority; + final DirectiveSortPriority priority; final String uri; /// The offset of the first token, usually the keyword but may include leading comments. @@ -354,7 +331,7 @@ class _DirectiveInfo implements Comparable<_DirectiveInfo> { @override int compareTo(_DirectiveInfo other) { if (priority == other.priority) { - var compare = _compareUri(uri, other.uri); + var compare = compareDirectiveUri(uri, other.uri); if (compare != 0) { return compare; } @@ -365,41 +342,4 @@ class _DirectiveInfo implements Comparable<_DirectiveInfo> { @override String toString() => '(priority=$priority; text=$text)'; - - /// Should keep these in sync! Copied from - /// https://github.com/dart-lang/linter/blob/658f497eef/lib/src/rules/directives_ordering.dart#L380-L387 - /// Consider finding a way to share this code! - static int _compareUri(String a, String b) { - if (!a.startsWith('package:') || !b.startsWith('package:')) { - if (!a.startsWith('/') && !b.startsWith('/')) { - return a.compareTo(b); - } - } - var indexA = a.indexOf('/'); - var indexB = b.indexOf('/'); - if (indexA == -1 || indexB == -1) return a.compareTo(b); - var result = a.substring(0, indexA).compareTo(b.substring(0, indexB)); - if (result != 0) return result; - return a.substring(indexA + 1).compareTo(b.substring(indexB + 1)); - } -} - -class _DirectivePriority { - static const IMPORT_SDK = _DirectivePriority('IMPORT_SDK', 0); - static const IMPORT_PKG = _DirectivePriority('IMPORT_PKG', 1); - static const IMPORT_OTHER = _DirectivePriority('IMPORT_OTHER', 2); - static const IMPORT_REL = _DirectivePriority('IMPORT_REL', 3); - static const EXPORT_SDK = _DirectivePriority('EXPORT_SDK', 4); - static const EXPORT_PKG = _DirectivePriority('EXPORT_PKG', 5); - static const EXPORT_OTHER = _DirectivePriority('EXPORT_OTHER', 6); - static const EXPORT_REL = _DirectivePriority('EXPORT_REL', 7); - static const PART = _DirectivePriority('PART', 8); - - final String name; - final int ordinal; - - const _DirectivePriority(this.name, this.ordinal); - - @override - String toString() => name; } diff --git a/pkg/analysis_server/test/src/services/refactoring/move_top_level_to_file_test.dart b/pkg/analysis_server/test/src/services/refactoring/move_top_level_to_file_test.dart index 1676fa457eee..3c1f1eabae77 100644 --- a/pkg/analysis_server/test/src/services/refactoring/move_top_level_to_file_test.dart +++ b/pkg/analysis_server/test/src/services/refactoring/move_top_level_to_file_test.dart @@ -306,6 +306,47 @@ class A {} otherFileContent: otherFileContent); } + /// Test that if the destination file gets both relative and package imports, + /// they are added in the correct order. + /// + /// https://github.com/dart-lang/sdk/issues/56657 + Future test_imports_ordering() async { + var libFilePath = join(projectFolderPath, 'lib', 'mixin.dart'); + + // Put the file in tool/ so we can use a package: import for the file + // above but get a relative import back to src. + mainFilePath = join(projectFolderPath, 'tool', 'main.dart'); + // TODO(dantup): Make these URIs getters to avoid setting these twice in + // each test. + mainFileUri = pathContext.toUri(mainFilePath); + + newFile(libFilePath, 'mixin PackageMixin {};'); + var originalSource = ''' +import 'package:test/mixin.dart'; + +class Staying {} +class Mov^ing extends Staying with PackageMixin {} +'''; + var declarationName = 'Moving'; + + var expected = ''' +>>>>>>>>>> tool/main.dart +import 'package:test/mixin.dart'; + +class Staying {} +>>>>>>>>>> tool/moving.dart created +import 'package:test/mixin.dart'; +import 'main.dart'; + +class Moving extends Staying with PackageMixin {} +'''; + await _singleDeclaration( + originalSource: originalSource, + expected: expected, + declarationName: declarationName, + ); + } + Future test_imports_prefix_cascade() async { var otherFileDeclarations = ''' final list = []; diff --git a/pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart b/pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart index fed3d05a8c94..62dc3d3c06fd 100644 --- a/pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart +++ b/pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart @@ -20,6 +20,7 @@ import 'package:analyzer_plugin/protocol/protocol_common.dart' hide Element, ElementKind; import 'package:analyzer_plugin/src/utilities/change_builder/change_builder_core.dart'; import 'package:analyzer_plugin/src/utilities/charcodes.dart'; +import 'package:analyzer_plugin/src/utilities/directive_sort.dart'; import 'package:analyzer_plugin/src/utilities/extensions/resolved_unit_result.dart'; import 'package:analyzer_plugin/src/utilities/library.dart'; import 'package:analyzer_plugin/src/utilities/string_utilities.dart'; @@ -1793,9 +1794,9 @@ class DartFileEditBuilderImpl extends FileEditBuilderImpl } } - // Sort imports by URIs. - var importList = imports.toList(); - importList.sort((a, b) => a.uriText.compareTo(b.uriText)); + // Sort the new imports so dart, package, and relative imports appear in the + // correct order. + var importList = imports.toList()..sort(); var sortCombinators = resolvedUnit.session.analysisContext .getAnalysisOptionsForFile(resolvedUnit.file) .isLintEnabled('combinators_ordering'); @@ -2523,9 +2524,11 @@ class _InsertionPreparer { } /// Information about a library import. -class _LibraryImport { +class _LibraryImport implements Comparable<_LibraryImport> { final String uriText; + late final DirectiveSortPriority sortPriority; + /// Prefixes that this library is/will be imported using. /// /// An empty string means the import is unprefixed. This can be included along @@ -2545,6 +2548,7 @@ class _LibraryImport { List>? hiddenNames, }) : shownNames = shownNames ?? [], hiddenNames = hiddenNames ?? [] { + sortPriority = DirectiveSortPriority(uriText, DirectiveSortKind.import); prefixes.add(prefix); } @@ -2569,6 +2573,14 @@ class _LibraryImport { !const SetEquality().equals(other.prefixes, prefixes); } + @override + int compareTo(_LibraryImport other) { + if (sortPriority == other.sortPriority) { + return compareDirectiveUri(uriText, other.uriText); + } + return sortPriority.ordinal - other.sortPriority.ordinal; + } + /// Ensures [name] is visible for this import. /// /// If the import already has a show combinator, this name will be added. diff --git a/pkg/analyzer_plugin/lib/src/utilities/directive_sort.dart b/pkg/analyzer_plugin/lib/src/utilities/directive_sort.dart new file mode 100644 index 000000000000..81d68af2c415 --- /dev/null +++ b/pkg/analyzer_plugin/lib/src/utilities/directive_sort.dart @@ -0,0 +1,73 @@ +// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +/// Compares to URI strings for a directive to produce the desired sort order. +/// +/// Should keep these in sync! Copied from +/// https://github.com/dart-lang/linter/blob/658f497eef/lib/src/rules/directives_ordering.dart#L380-L387 +/// Consider finding a way to share this code! +int compareDirectiveUri(String a, String b) { + if (!a.startsWith('package:') || !b.startsWith('package:')) { + if (!a.startsWith('/') && !b.startsWith('/')) { + return a.compareTo(b); + } + } + var indexA = a.indexOf('/'); + var indexB = b.indexOf('/'); + if (indexA == -1 || indexB == -1) return a.compareTo(b); + var result = a.substring(0, indexA).compareTo(b.substring(0, indexB)); + if (result != 0) return result; + return a.substring(indexA + 1).compareTo(b.substring(indexB + 1)); +} + +/// The kind of directive for sorting purposes. +enum DirectiveSortKind { import, export, part } + +/// The priority used for grouping directives when sorting. +class DirectiveSortPriority { + static const IMPORT_SDK = DirectiveSortPriority._('IMPORT_SDK', 0); + static const IMPORT_PKG = DirectiveSortPriority._('IMPORT_PKG', 1); + static const IMPORT_OTHER = DirectiveSortPriority._('IMPORT_OTHER', 2); + static const IMPORT_REL = DirectiveSortPriority._('IMPORT_REL', 3); + static const EXPORT_SDK = DirectiveSortPriority._('EXPORT_SDK', 4); + static const EXPORT_PKG = DirectiveSortPriority._('EXPORT_PKG', 5); + static const EXPORT_OTHER = DirectiveSortPriority._('EXPORT_OTHER', 6); + static const EXPORT_REL = DirectiveSortPriority._('EXPORT_REL', 7); + static const PART = DirectiveSortPriority._('PART', 8); + + final String name; + final int ordinal; + + factory DirectiveSortPriority(String uri, DirectiveSortKind kind) { + switch (kind) { + case DirectiveSortKind.import: + if (uri.startsWith('dart:')) { + return DirectiveSortPriority.IMPORT_SDK; + } else if (uri.startsWith('package:')) { + return DirectiveSortPriority.IMPORT_PKG; + } else if (uri.contains('://')) { + return DirectiveSortPriority.IMPORT_OTHER; + } else { + return DirectiveSortPriority.IMPORT_REL; + } + case DirectiveSortKind.export: + if (uri.startsWith('dart:')) { + return DirectiveSortPriority.EXPORT_SDK; + } else if (uri.startsWith('package:')) { + return DirectiveSortPriority.EXPORT_PKG; + } else if (uri.contains('://')) { + return DirectiveSortPriority.EXPORT_OTHER; + } else { + return DirectiveSortPriority.EXPORT_REL; + } + case DirectiveSortKind.part: + return DirectiveSortPriority.PART; + } + } + + const DirectiveSortPriority._(this.name, this.ordinal); + + @override + String toString() => name; +} diff --git a/pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_dart_test.dart b/pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_dart_test.dart index d82a3b5d782f..c77143ccf646 100644 --- a/pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_dart_test.dart +++ b/pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_dart_test.dart @@ -2639,6 +2639,34 @@ class B {} "import 'package:test/test_a.dart' show A, Other; A")); } + Future test_importLibrary_sorted() async { + var resolvedUnit = await resolveContent('/home/test/lib/test.dart', ''); + var builder = await newBuilder(); + await builder.addDartFileEdit(resolvedUnit.path, (builder) { + builder.importLibrary(Uri.parse('z.dart')); + builder.importLibrary(Uri.parse('a.dart')); + builder.importLibrary(Uri.parse('../z.dart')); + builder.importLibrary(Uri.parse('../a.dart')); + builder.importLibrary(Uri.parse('package:foo/x.dart')); + builder.importLibrary(Uri.parse('package:foo/a.dart')); + builder.importLibrary(Uri.parse('dart:x')); + builder.importLibrary(Uri.parse('dart:a')); + }); + + var edits = getEdits(builder); + expect(edits, hasLength(1)); + expect(edits[0].replacement, equalsIgnoringWhitespace(''' +import 'dart:a'; +import 'dart:x'; +import 'package:foo/a.dart'; +import 'package:foo/x.dart'; +import '../a.dart'; +import '../z.dart'; +import 'a.dart'; +import 'z.dart'; +''')); + } + Future test_multipleEdits_concurrently() async { var initialCode = '00'; var path = convertPath('/home/test/lib/test.dart');