DAS: Fix producers that can apply in bulk, but not really across file
This introduces CorrectionApplicability.automaticallyButOncePerFile,
used in just 4 producers:
* AddNullCheck
* MoveDocCommentToLibraryDirective
* RemoveLibraryName
* RemoveUnnecessaryLibraryDirective
Also fixes FixProcessorMapTest to actually test registered fixes.
Change-Id: I2ed2d8d6a030f04ea3fa969988719bb9d4c2f8fb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/367661
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
diff --git a/pkg/analysis_server/lib/src/services/correction/dart/abstract_producer.dart b/pkg/analysis_server/lib/src/services/correction/dart/abstract_producer.dart
index 595f694..19a9ff2 100644
--- a/pkg/analysis_server/lib/src/services/correction/dart/abstract_producer.dart
+++ b/pkg/analysis_server/lib/src/services/correction/dart/abstract_producer.dart
@@ -30,9 +30,10 @@
/// How broadly a [CorrectionProducer] can be applied.
///
-/// Each value of this enum is cumulative, as the index increases. When a
-/// correctiopn producer has a given applicability, it also can be applied with
-/// each lower-indexed value. For example, a correction producer with an
+/// Each value of this enum is cumulative, as the index increases, except for
+/// [CorrectionApplicability.automaticallyButOncePerFile]. When a correctiopn
+/// producer has a given applicability, it also can be applied with each
+/// lower-indexed value. For example, a correction producer with an
/// applicability of [CorrectionApplicability.acrossFiles] can also be used to
/// apply corrections across a single file
/// ([CorrectionApplicability.singleLocation]) and at a single location
@@ -40,6 +41,9 @@
/// reflect this property: [CorrectionProducer.canBeAppliedAcrossSingleFile],
/// [CorrectionProducer.canBeAppliedAcrossFiles],
/// [CorrectionProducer.canBeAppliedAutomatically].
+///
+/// Note that [CorrectionApplicability.automaticallyButOncePerFile] is the one
+/// value that does not have this cumulative property.
enum CorrectionApplicability {
/// Indicates a correction can be applied only at a specific location.
///
@@ -78,11 +82,27 @@
/// A correction with this applicability is also applicable at a specific
/// location, and across a file, and across multiple files.
automatically,
+
+ /// Indicates a correction can be applied in multiple files, except only one
+ /// location per file; the correction can be applied even if not chosen
+ /// explicitly as a tool action, and can be applied to potentially incomplete
+ /// code.
+ ///
+ /// A correction with this applicability is also applicable at a specific
+ /// location, and across multiple files.
+ automaticallyButOncePerFile,
}
/// An object that can compute a correction (fix or assist) in a Dart file.
abstract class CorrectionProducer<T extends ParsedUnitResult>
extends _AbstractCorrectionProducer<T> {
+ /// The applicability of this producer.
+ ///
+ /// This property is to be implemented by each subclass, but outside code must
+ /// use other properties to determine a producer's applicability:
+ /// [canBeAppliedAcrossSingleFile], [canBeAppliedAcrossFiles], and
+ /// [canBeAppliedAutomatically].
+ @protected
CorrectionApplicability get applicability;
/// The arguments that should be used when composing the message for an
@@ -99,7 +119,8 @@
/// time as applying corrections from other producers.
bool get canBeAppliedAcrossFiles =>
applicability == CorrectionApplicability.acrossFiles ||
- applicability == CorrectionApplicability.automatically;
+ applicability == CorrectionApplicability.automatically ||
+ applicability == CorrectionApplicability.automaticallyButOncePerFile;
/// Whether this producer can be used to apply a correction in multiple
/// positions simultaneously across a file.
@@ -113,7 +134,8 @@
/// in bulk across multiple files and/or at the same time as applying
/// corrections from other producers.
bool get canBeAppliedAutomatically =>
- applicability == CorrectionApplicability.automatically;
+ applicability == CorrectionApplicability.automatically ||
+ applicability == CorrectionApplicability.automaticallyButOncePerFile;
/// The length of the source range associated with the error message being
/// fixed, or `null` if there is no diagnostic.
diff --git a/pkg/analysis_server/lib/src/services/correction/dart/add_null_check.dart b/pkg/analysis_server/lib/src/services/correction/dart/add_null_check.dart
index d195b53..7f8d3d5 100644
--- a/pkg/analysis_server/lib/src/services/correction/dart/add_null_check.dart
+++ b/pkg/analysis_server/lib/src/services/correction/dart/add_null_check.dart
@@ -21,6 +21,12 @@
final bool skipAssignabilityCheck;
@override
+ // This is a mutable field so it can be changed in `_replaceWithNullCheck`.
+ // TODO(srawlins): This seems to violate a few assert statements around the
+ // package that read:
+ //
+ // > Producers used in bulk fixes must not modify the FixKind during
+ // > computation.
FixKind fixKind = DartFixKind.ADD_NULL_CHECK;
@override
@@ -32,7 +38,7 @@
AddNullCheck.withoutAssignabilityCheck()
: skipAssignabilityCheck = true,
- applicability = CorrectionApplicability.automatically;
+ applicability = CorrectionApplicability.automaticallyButOncePerFile;
@override
Future<void> compute(ChangeBuilder builder) async {
@@ -221,7 +227,7 @@
return false;
}
- /// Replace the null aware [token] with the null check operator.
+ /// Replaces the null aware [token] with the null check operator.
Future<void> _replaceWithNullCheck(ChangeBuilder builder, Token token) async {
fixKind = DartFixKind.REPLACE_WITH_NULL_AWARE;
var lexeme = token.lexeme;
diff --git a/pkg/analysis_server/lib/src/services/correction/dart/move_doc_comment_to_library_directive.dart b/pkg/analysis_server/lib/src/services/correction/dart/move_doc_comment_to_library_directive.dart
index e4ca5f8..02e3b0c 100644
--- a/pkg/analysis_server/lib/src/services/correction/dart/move_doc_comment_to_library_directive.dart
+++ b/pkg/analysis_server/lib/src/services/correction/dart/move_doc_comment_to_library_directive.dart
@@ -16,7 +16,7 @@
class MoveDocCommentToLibraryDirective extends ResolvedCorrectionProducer {
@override
CorrectionApplicability get applicability =>
- CorrectionApplicability.automatically;
+ CorrectionApplicability.automaticallyButOncePerFile;
@override
FixKind get fixKind => DartFixKind.MOVE_DOC_COMMENT_TO_LIBRARY_DIRECTIVE;
diff --git a/pkg/analysis_server/lib/src/services/correction/dart/remove_library_name.dart b/pkg/analysis_server/lib/src/services/correction/dart/remove_library_name.dart
index cf019ac..293284d 100644
--- a/pkg/analysis_server/lib/src/services/correction/dart/remove_library_name.dart
+++ b/pkg/analysis_server/lib/src/services/correction/dart/remove_library_name.dart
@@ -12,7 +12,7 @@
class RemoveLibraryName extends ResolvedCorrectionProducer {
@override
CorrectionApplicability get applicability =>
- CorrectionApplicability.automatically;
+ CorrectionApplicability.automaticallyButOncePerFile;
@override
FixKind get fixKind => DartFixKind.REMOVE_LIBRARY_NAME;
diff --git a/pkg/analysis_server/lib/src/services/correction/dart/remove_unnecessary_library_directive.dart b/pkg/analysis_server/lib/src/services/correction/dart/remove_unnecessary_library_directive.dart
index 54b4e99..c240e07 100644
--- a/pkg/analysis_server/lib/src/services/correction/dart/remove_unnecessary_library_directive.dart
+++ b/pkg/analysis_server/lib/src/services/correction/dart/remove_unnecessary_library_directive.dart
@@ -12,7 +12,7 @@
class RemoveUnnecessaryLibraryDirective extends ResolvedCorrectionProducer {
@override
CorrectionApplicability get applicability =>
- CorrectionApplicability.automatically;
+ CorrectionApplicability.automaticallyButOncePerFile;
@override
FixKind get fixKind => DartFixKind.REMOVE_UNNECESSARY_LIBRARY_DIRECTIVE;
diff --git a/pkg/analysis_server/lib/src/services/correction/dart/replace_with_part_of_uri.dart b/pkg/analysis_server/lib/src/services/correction/dart/replace_with_part_of_uri.dart
index 1549523..ec25bdc 100644
--- a/pkg/analysis_server/lib/src/services/correction/dart/replace_with_part_of_uri.dart
+++ b/pkg/analysis_server/lib/src/services/correction/dart/replace_with_part_of_uri.dart
@@ -14,9 +14,6 @@
String _uriStr = '';
@override
- FixKind fixKind = DartFixKind.REPLACE_WITH_PART_OF_URI;
-
- @override
CorrectionApplicability get applicability =>
// TODO(applicability): comment on why.
CorrectionApplicability.singleLocation;
@@ -25,6 +22,9 @@
List<String> get fixArguments => [_uriStr];
@override
+ FixKind get fixKind => DartFixKind.REPLACE_WITH_PART_OF_URI;
+
+ @override
Future<void> compute(ChangeBuilder builder) async {
var partOfDirective = node;
if (partOfDirective is! PartOfDirective) {
diff --git a/pkg/analysis_server/lib/src/services/correction/fix.dart b/pkg/analysis_server/lib/src/services/correction/fix.dart
index b3830f0..6d98b87 100644
--- a/pkg/analysis_server/lib/src/services/correction/fix.dart
+++ b/pkg/analysis_server/lib/src/services/correction/fix.dart
@@ -1337,26 +1337,6 @@
DartFixKindPriority.IN_FILE,
"Remove unnecessary 'const' keywords everywhere in file",
);
- static const REMOVE_UNNECESSARY_LATE = FixKind(
- 'dart.fix.remove.unnecessaryLate',
- DartFixKindPriority.DEFAULT,
- "Remove unnecessary 'late' keyword",
- );
- static const REMOVE_UNNECESSARY_LATE_MULTI = FixKind(
- 'dart.fix.remove.unnecessaryLate.multi',
- DartFixKindPriority.IN_FILE,
- "Remove unnecessary 'late' keywords everywhere in file",
- );
- static const REMOVE_UNNECESSARY_NEW = FixKind(
- 'dart.fix.remove.unnecessaryNew',
- DartFixKindPriority.DEFAULT,
- "Remove unnecessary 'new' keyword",
- );
- static const REMOVE_UNNECESSARY_NEW_MULTI = FixKind(
- 'dart.fix.remove.unnecessaryNew.multi',
- DartFixKindPriority.IN_FILE,
- "Remove unnecessary 'new' keywords everywhere in file",
- );
static const REMOVE_UNNECESSARY_CONTAINER = FixKind(
'dart.fix.remove.unnecessaryContainer',
DartFixKindPriority.DEFAULT,
@@ -1367,11 +1347,31 @@
DartFixKindPriority.IN_FILE,
"Remove unnecessary 'Container's in file",
);
+ static const REMOVE_UNNECESSARY_LATE = FixKind(
+ 'dart.fix.remove.unnecessaryLate',
+ DartFixKindPriority.DEFAULT,
+ "Remove unnecessary 'late' keyword",
+ );
+ static const REMOVE_UNNECESSARY_LATE_MULTI = FixKind(
+ 'dart.fix.remove.unnecessaryLate.multi',
+ DartFixKindPriority.IN_FILE,
+ "Remove unnecessary 'late' keywords everywhere in file",
+ );
static const REMOVE_UNNECESSARY_LIBRARY_DIRECTIVE = FixKind(
'dart.fix.remove.unnecessaryLibraryDirective',
DartFixKindPriority.DEFAULT,
'Remove unnecessary library directive',
);
+ static const REMOVE_UNNECESSARY_NEW = FixKind(
+ 'dart.fix.remove.unnecessaryNew',
+ DartFixKindPriority.DEFAULT,
+ "Remove unnecessary 'new' keyword",
+ );
+ static const REMOVE_UNNECESSARY_NEW_MULTI = FixKind(
+ 'dart.fix.remove.unnecessaryNew.multi',
+ DartFixKindPriority.IN_FILE,
+ "Remove unnecessary 'new' keywords everywhere in file",
+ );
static const REMOVE_UNNECESSARY_PARENTHESES = FixKind(
'dart.fix.remove.unnecessaryParentheses',
DartFixKindPriority.DEFAULT,
diff --git a/pkg/analysis_server/test/src/services/correction/fix/fix_processor_map_test.dart b/pkg/analysis_server/test/src/services/correction/fix/fix_processor_map_test.dart
index 328da67..400b8de 100644
--- a/pkg/analysis_server/test/src/services/correction/fix/fix_processor_map_test.dart
+++ b/pkg/analysis_server/test/src/services/correction/fix/fix_processor_map_test.dart
@@ -3,6 +3,7 @@
// BSD-style license that can be found in the LICENSE file.
import 'package:analysis_server/src/services/correction/dart/abstract_producer.dart';
+import 'package:analysis_server/src/services/correction/fix_internal.dart';
import 'package:analysis_server/src/services/correction/fix_processor.dart';
import 'package:test/test.dart';
import 'package:test_reflective_loader/test_reflective_loader.dart';
@@ -23,13 +24,17 @@
'prefer_inlined_adds',
];
+ void setUp() {
+ registerBuiltInProducers();
+ }
+
void test_lintProducerMap() {
- _assertMap(FixProcessor.lintProducerMap.entries,
- lintsAllowedToHaveMultipleBulkFixes);
+ _assertMap(
+ FixProcessor.lintProducerMap, lintsAllowedToHaveMultipleBulkFixes);
}
void test_nonLintProducerMap() {
- _assertMap(FixProcessor.nonLintProducerMap.entries);
+ _assertMap(FixProcessor.nonLintProducerMap);
}
void test_registerFixForLint() {
@@ -43,12 +48,12 @@
FixProcessor.lintProducerMap.remove(lintName);
}
- void _assertMap<K>(Iterable<MapEntry<K, List<ProducerGenerator>>> entries,
- [List<String> keysAllowedToHaveMultipleBulkFixes = const []]) {
- var list = <String>[];
- for (var entry in entries) {
+ void _assertMap(Map<Object, List<ProducerGenerator>> producerMap,
+ [List<String> codesAllowedToHaveMultipleBulkFixes = const []]) {
+ var unexpectedBulkCodes = <String>[];
+ for (var MapEntry(:key, value: generators) in producerMap.entries) {
var bulkCount = 0;
- for (var generator in entry.value) {
+ for (var generator in generators) {
var producer = generator();
_assertValidProducer(producer);
if (producer.canBeAppliedAcrossFiles) {
@@ -56,16 +61,16 @@
}
}
if (bulkCount > 1) {
- var key = entry.key.toString();
- if (!keysAllowedToHaveMultipleBulkFixes.contains(key)) {
- list.add(key);
+ var name = key.toString();
+ if (!codesAllowedToHaveMultipleBulkFixes.contains(name)) {
+ unexpectedBulkCodes.add(name);
}
}
}
- if (list.isNotEmpty) {
+ if (unexpectedBulkCodes.isNotEmpty) {
var buffer = StringBuffer();
- buffer.writeln('Multiple bulk fixes for');
- for (var code in list) {
+ buffer.writeln('Unexpected multiple bulk fixes for');
+ for (var code in unexpectedBulkCodes) {
buffer.writeln('- $code');
}
fail(buffer.toString());
@@ -77,7 +82,7 @@
expect(producer.fixKind, isNotNull, reason: '$className.fixKind');
if (producer.canBeAppliedAcrossSingleFile) {
expect(producer.multiFixKind, isNotNull,
- reason: '$className.multiFixKind');
+ reason: '$className.multiFixKind should be non-null');
}
}
}
