Skip to content

Commit

Permalink
[analysis_server] Fix sorting of different kinds of imports in DartFi…
Browse files Browse the repository at this point in the history 
…leEditBuilder.importLibrary

Previously any pending imports added by DartFileEditBuilder would just be sorted by their text ignoring the kind of import. If there were existing imports for dart, package, relative then they might be inserted in the right places, but if there were not (or not the right kind) of existing imports, they would be just be added in alphabetical order (rather than dart, then package, then relative).

This change extracts some of the rules for sorting directives from analysis_server's ImportOrganizer into analyzer_plugin/src so that they can be reused by the DartFileEditBuilder that lives there.

Fixes #56657

Change-Id: I6dc5476add2b7b1804080ffdc8270d0bb80597db
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/384284
Commit-Queue: Brian Wilkerson <[email protected]>
Reviewed-by: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
  • Loading branch information
@DanTup
DanTup authored and Commit Queue committed Sep 10, 2024
1 parent ad94d96 commit ffeadc9
Show file tree
Hide file tree
Showing 5 changed files with 235 additions and 141 deletions.
214 changes: 77 additions & 137 deletions pkg/analysis_server/lib/src/services/correction/organize_imports.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand All @@ -168,7 +176,7 @@ class ImportOrganizer {
String directivesCode;
{
var sb = StringBuffer();
_DirectivePriority? currentPriority;
DirectiveSortPriority? currentPriority;
var previousDirectiveText = '';
for (var directiveInfo in directives) {
if (!hasUnresolvedIdentifierError) {
Expand Down Expand Up @@ -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) =>
Expand All @@ -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.
Expand All @@ -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;
}
Expand All @@ -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;
}
41 changes: 41 additions & 0 deletions pkg/analysis_server/test/src/services/refactoring/move_top_level_to_file_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> 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<void> test_imports_prefix_cascade() async {
var otherFileDeclarations = '''
final list = <int>[];
Expand Down
20 changes: 16 additions & 4 deletions pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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
Expand All @@ -2545,6 +2548,7 @@ class _LibraryImport {
List<List<String>>? hiddenNames,
}) : shownNames = shownNames ?? [],
hiddenNames = hiddenNames ?? [] {
sortPriority = DirectiveSortPriority(uriText, DirectiveSortKind.import);
prefixes.add(prefix);
}

Expand All @@ -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.
Expand Down
Loading

0 comments on commit ffeadc9

Please sign in to comment.