Skip to content

Commit fc5b4e3

Browse files
danrubelcommit-bot@chromium.org
danrubel
authored and
[email protected]
committed
Rework dartfix lint fixes to remove race condition
This refactors the way that dartfix finds lints to fix to be based on error codes rather than manually running the lints. This prevents a race condition that occurs between calling setAnalysisRoots and requesting fixes. Fix #37558 Change-Id: I35817a9b152a28db9e78662a5ea5096f275f68bb Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/111700 Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Dan Rubel <[email protected]>
1 parent d08c203 commit fc5b4e3

File tree

7 files changed

+133
-317
lines changed

7 files changed

+133
-317
lines changed

‎pkg/analysis_server/lib/src/edit/edit_dartfix.dart‎

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import 'package:analysis_server/src/edit/fix/fix_lint_task.dart';
1414
import 'package:analyzer/dart/analysis/results.dart';
1515
import 'package:analyzer/dart/analysis/session.dart';
1616
import 'package:analyzer/file_system/file_system.dart';
17+
import 'package:analyzer/src/generated/engine.dart' show AnalysisOptionsImpl;
1718

1819
class EditDartFix
1920
with FixCodeProcessor, FixErrorProcessor, FixLintProcessor
@@ -71,6 +72,13 @@ class EditDartFix
7172
// Validate each included file and directory.
7273
final resourceProvider = server.resourceProvider;
7374
final contextManager = server.contextManager;
75+
76+
// Discard any existing analysis so that the linters set below will be
77+
// used to generate errors that can then be fixed.
78+
// TODO(danrubel): Rework to use a different approach if this command
79+
// will be used from within the IDE.
80+
contextManager.refresh(null);
81+
7482
for (String filePath in params.included) {
7583
if (!server.isValidFilePath(filePath)) {
7684
return new Response.invalidFilePathFormat(request, filePath);
@@ -81,8 +89,18 @@ class EditDartFix
8189
contextManager.isInAnalysisRoot(filePath))) {
8290
return new Response.fileNotAnalyzed(request, filePath);
8391
}
84-
var pkgFolder =
85-
findPkgFolder(contextManager.getContextFolderFor(filePath));
92+
93+
// Set the linters used during analysis. If this command is used from
94+
// within an IDE, then this will cause the lint results to change.
95+
// TODO(danrubel): Rework to use a different approach if this command
96+
// will be used from within the IDE.
97+
var driver = contextManager.getDriverFor(filePath);
98+
var analysisOptions = driver.analysisOptions as AnalysisOptionsImpl;
99+
analysisOptions.lint = true;
100+
analysisOptions.lintRules = linters;
101+
102+
var contextFolder = contextManager.getContextFolderFor(filePath);
103+
var pkgFolder = findPkgFolder(contextFolder);
86104
if (pkgFolder != null && !pkgFolders.contains(pkgFolder)) {
87105
pkgFolders.add(pkgFolder);
88106
}
@@ -98,21 +116,22 @@ class EditDartFix
98116
await processPackage(pkgFolder);
99117
}
100118

101-
// Process each source file.
102119
bool hasErrors = false;
103120
String changedPath;
104-
server.contextManager.driverMap.values
105-
.forEach((d) => d.onCurrentSessionAboutToBeDiscarded = (String path) {
106-
// Remember the resource that changed during analysis
107-
changedPath = path;
108-
});
121+
contextManager.driverMap.values.forEach((driver) {
122+
// Setup a listener to remember the resource that changed during analysis
123+
// so it can be reported if there is an InconsistentAnalysisException.
124+
driver.onCurrentSessionAboutToBeDiscarded = (String path) {
125+
changedPath = path;
126+
};
127+
});
109128

129+
// Process each source file.
110130
try {
111131
await processResources((ResolvedUnitResult result) async {
112132
if (await processErrors(result)) {
113133
hasErrors = true;
114134
}
115-
await processLints(result);
116135
if (numPhases > 0) {
117136
await processCodeTasks(0, result);
118137
}
@@ -122,7 +141,6 @@ class EditDartFix
122141
await processCodeTasks(phase, result);
123142
});
124143
}
125-
await finishLints();
126144
await finishCodeTasks();
127145
} on InconsistentAnalysisException catch (_) {
128146
// If a resource changed, report the problem without suggesting fixes

‎pkg/analysis_server/lib/src/edit/fix/basic_fix_lint_assist_task.dart‎

Lines changed: 21 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -10,60 +10,40 @@ import 'package:analysis_server/src/services/correction/assist.dart';
1010
import 'package:analysis_server/src/services/correction/assist_internal.dart';
1111
import 'package:analysis_server/src/services/correction/change_workspace.dart';
1212
import 'package:analyzer/dart/analysis/results.dart';
13-
import 'package:analyzer/dart/ast/ast.dart';
1413
import 'package:analyzer/error/error.dart';
14+
import 'package:analyzer/src/dart/ast/utilities.dart';
1515
import 'package:analyzer/src/lint/registry.dart';
1616
import 'package:analyzer_plugin/utilities/assist/assist.dart';
1717

1818
class BasicFixLintAssistTask extends FixLintTask {
1919
final AssistKind assistKind;
20-
final nodes = <AstNode>[];
2120

2221
BasicFixLintAssistTask(this.assistKind, DartFixListener listener)
2322
: super(listener);
2423

2524
@override
26-
Future<void> applyLocalFixes(ResolvedUnitResult result) async {
27-
while (nodes.isNotEmpty) {
28-
AstNode node = nodes.removeLast();
29-
AssistProcessor processor = new AssistProcessor(
30-
new DartAssistContextImpl(
31-
DartChangeWorkspace(listener.server.currentSessions),
32-
result,
33-
node.offset,
34-
node.length,
35-
),
36-
);
37-
List<Assist> assists = await processor.computeAssist(assistKind);
25+
Future<void> fixError(ResolvedUnitResult result, AnalysisError error) async {
26+
var node = new NodeLocator(error.offset).searchWithin(result.unit);
27+
AssistProcessor processor = new AssistProcessor(
28+
new DartAssistContextImpl(
29+
DartChangeWorkspace(listener.server.currentSessions),
30+
result,
31+
node.offset,
32+
node.length,
33+
),
34+
);
35+
List<Assist> assists = await processor.computeAssist(assistKind);
3836

39-
final location = listener.locationFor(result, node.offset, node.length);
40-
if (assists.isNotEmpty) {
41-
for (Assist assist in assists) {
42-
listener.addSourceChange(
43-
assist.kind.message, location, assist.change);
44-
}
45-
} else {
46-
// TODO(danrubel): If assists is empty, then determine why
47-
// assist could not be performed and report that in the description.
48-
listener.addRecommendation(
49-
'Fix not found: ${assistKind.message}', location);
37+
final location = listener.locationFor(result, node.offset, node.length);
38+
if (assists.isNotEmpty) {
39+
for (Assist assist in assists) {
40+
listener.addSourceChange(assist.kind.message, location, assist.change);
5041
}
51-
}
52-
53-
return null;
54-
}
55-
56-
@override
57-
Future<void> applyRemainingFixes() {
58-
// All fixes applied in [applyLocalFixes]
59-
return null;
60-
}
61-
62-
@override
63-
void reportErrorForNode(ErrorCode errorCode, AstNode node,
64-
[List<Object> arguments]) {
65-
if (source.fullName != null) {
66-
nodes.add(node);
42+
} else {
43+
// TODO(danrubel): If assists is empty, then determine why
44+
// assist could not be performed and report that in the description.
45+
listener.addRecommendation(
46+
'Fix not found: ${assistKind.message}', location);
6747
}
6848
}
6949

‎pkg/analysis_server/lib/src/edit/fix/basic_fix_lint_error_task.dart‎

Lines changed: 0 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -2,76 +2,20 @@
22
// for details. All rights reserved. Use of this source code is governed by a
33
// BSD-style license that can be found in the LICENSE file.
44

5-
import 'package:analysis_server/plugin/edit/fix/fix_core.dart';
65
import 'package:analysis_server/src/edit/fix/dartfix_listener.dart';
76
import 'package:analysis_server/src/edit/fix/dartfix_registrar.dart';
87
import 'package:analysis_server/src/edit/fix/fix_lint_task.dart';
9-
import 'package:analysis_server/src/services/correction/change_workspace.dart';
108
import 'package:analysis_server/src/services/correction/fix.dart';
11-
import 'package:analysis_server/src/services/correction/fix_internal.dart';
12-
import 'package:analyzer/dart/analysis/results.dart';
13-
import 'package:analyzer/dart/ast/ast.dart';
149
import 'package:analyzer/error/error.dart';
1510
import 'package:analyzer/src/lint/registry.dart';
1611
import 'package:analyzer_plugin/utilities/fixes/fixes.dart';
17-
import 'package:front_end/src/scanner/token.dart';
1812

1913
class BasicFixLintErrorTask extends FixLintTask {
2014
final FixKind fixKind;
21-
final errors = <AnalysisError>[];
2215

2316
BasicFixLintErrorTask(this.fixKind, DartFixListener listener)
2417
: super(listener);
2518

26-
@override
27-
Future<void> applyLocalFixes(ResolvedUnitResult result) async {
28-
while (errors.isNotEmpty) {
29-
AnalysisError error = errors.removeLast();
30-
final workspace = DartChangeWorkspace(listener.server.currentSessions);
31-
final dartContext = new DartFixContextImpl(
32-
workspace,
33-
result,
34-
error,
35-
(name) => [],
36-
);
37-
final processor = new FixProcessor(dartContext);
38-
Fix fix = await processor.computeFix();
39-
final location = listener.locationFor(result, error.offset, error.length);
40-
if (fix != null) {
41-
listener.addSourceChange(fix.change.message, location, fix.change);
42-
} else {
43-
// TODO(danrubel): Determine why the fix could not be applied
44-
// and report that in the description.
45-
listener.addRecommendation(
46-
'Could not fix "${error.message}"', location);
47-
}
48-
}
49-
50-
return null;
51-
}
52-
53-
@override
54-
Future<void> applyRemainingFixes() {
55-
// All fixes applied in [applyLocalFixes]
56-
return null;
57-
}
58-
59-
@override
60-
void reportErrorForNode(ErrorCode code, AstNode node,
61-
[List<Object> arguments]) {
62-
if (source.fullName != null) {
63-
errors.add(new AnalysisError(source, node.offset, node.length, code));
64-
}
65-
}
66-
67-
@override
68-
void reportErrorForToken(ErrorCode code, Token token,
69-
[List<Object> arguments]) {
70-
if (source.fullName != null) {
71-
errors.add(new AnalysisError(source, token.offset, token.length, code));
72-
}
73-
}
74-
7519
static void nullClosures(
7620
DartFixRegistrar registrar, DartFixListener listener) {
7721
registrar.registerLintTask(

‎pkg/analysis_server/lib/src/edit/fix/fix_error_task.dart‎

Lines changed: 28 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,31 @@ import 'package:analyzer/dart/analysis/results.dart';
1313
import 'package:analyzer/error/error.dart';
1414
import 'package:analyzer/src/error/codes.dart';
1515

16-
/// A task for fixing a particular error
17-
class FixErrorTask {
18-
static void fixNamedConstructorTypeArgs(
19-
DartFixRegistrar registrar, DartFixListener listener) {
20-
registrar.registerErrorTask(
21-
StaticTypeWarningCode.WRONG_NUMBER_OF_TYPE_ARGUMENTS_CONSTRUCTOR,
22-
new FixErrorTask(listener));
16+
/// A processor used by [EditDartFix] to manage [FixErrorTask]s.
17+
mixin FixErrorProcessor {
18+
/// A mapping from [ErrorCode] to the fix that should be applied.
19+
final errorTaskMap = <ErrorCode, FixErrorTask>{};
20+
21+
Future<bool> processErrors(ResolvedUnitResult result) async {
22+
bool foundError = false;
23+
for (AnalysisError error in result.errors) {
24+
final task = errorTaskMap[error.errorCode];
25+
if (task != null) {
26+
await task.fixError(result, error);
27+
} else if (error.errorCode.type == ErrorType.SYNTACTIC_ERROR) {
28+
foundError = true;
29+
}
30+
}
31+
return foundError;
2332
}
2433

34+
void registerErrorTask(ErrorCode errorCode, FixErrorTask task) {
35+
errorTaskMap[errorCode] = task;
36+
}
37+
}
38+
39+
/// A task for fixing a particular error
40+
class FixErrorTask {
2541
final DartFixListener listener;
2642

2743
FixErrorTask(this.listener);
@@ -45,31 +61,11 @@ class FixErrorTask {
4561
listener.addRecommendation('Could not fix "${error.message}"', location);
4662
}
4763
}
48-
}
49-
50-
/// A processor used by [EditDartFix] to manage [FixErrorTask]s.
51-
mixin FixErrorProcessor {
52-
/// A mapping from [ErrorCode] to the fix that should be applied.
53-
final errorTaskMap = <ErrorCode, FixErrorTask>{};
5464

55-
Future<bool> processErrors(ResolvedUnitResult result) async {
56-
bool foundError = false;
57-
for (AnalysisError error in result.errors) {
58-
final task = errorTaskMap[error.errorCode];
59-
if (task != null) {
60-
await task.fixError(result, error);
61-
} else if (error.errorCode.type == ErrorType.SYNTACTIC_ERROR) {
62-
foundError = true;
63-
}
64-
}
65-
return foundError;
66-
}
67-
68-
Future<bool> fixError(ResolvedUnitResult result, AnalysisError error) async {
69-
return true;
70-
}
71-
72-
void registerErrorTask(ErrorCode errorCode, FixErrorTask task) {
73-
errorTaskMap[errorCode] = task;
65+
static void fixNamedConstructorTypeArgs(
66+
DartFixRegistrar registrar, DartFixListener listener) {
67+
registrar.registerErrorTask(
68+
StaticTypeWarningCode.WRONG_NUMBER_OF_TYPE_ARGUMENTS_CONSTRUCTOR,
69+
new FixErrorTask(listener));
7470
}
7571
}

0 commit comments

Comments
 (0)