Unverified Commit 2dd06d10 authored by Christopher Fujino's avatar Christopher Fujino Committed by GitHub

[flutter_tools] add custom tool analysis to analyze.dart, lint Future.catchError (#140122)

Ensure tool code does not use Future.catchError or Future.onError, because it is not statically safe: https://github.com/dart-lang/sdk/issues/51248.

This was proposed upstream in dart-lang/linter in https://github.com/dart-lang/linter/issues/4071 and https://github.com/dart-lang/linter/pull/4068, but not accepted.
parent 2ea5ca03
...@@ -18,6 +18,7 @@ import 'package:path/path.dart' as path; ...@@ -18,6 +18,7 @@ import 'package:path/path.dart' as path;
import 'allowlist.dart'; import 'allowlist.dart';
import 'custom_rules/analyze.dart'; import 'custom_rules/analyze.dart';
import 'custom_rules/avoid_future_catcherror.dart';
import 'custom_rules/no_double_clamp.dart'; import 'custom_rules/no_double_clamp.dart';
import 'custom_rules/no_stop_watches.dart'; import 'custom_rules/no_stop_watches.dart';
import 'run_command.dart'; import 'run_command.dart';
...@@ -186,6 +187,10 @@ Future<void> run(List<String> arguments) async { ...@@ -186,6 +187,10 @@ Future<void> run(List<String> arguments) async {
await analyzeWithRules(flutterRoot, testRules, await analyzeWithRules(flutterRoot, testRules,
includePaths: <String>['packages/flutter/test'], includePaths: <String>['packages/flutter/test'],
); );
final List<AnalyzeRule> toolRules = <AnalyzeRule>[AvoidFutureCatchError()];
final String toolRuleNames = toolRules.map((AnalyzeRule rule) => '\n * $rule').join();
printProgress('Analyzing code in the tool with the following rules:$toolRuleNames');
await analyzeToolWithRules(flutterRoot, toolRules);
} else { } else {
printProgress('Skipped performing further analysis in the framework because "flutter analyze" finished with a non-zero exit code.'); printProgress('Skipped performing further analysis in the framework because "flutter analyze" finished with a non-zero exit code.');
} }
......
...@@ -64,6 +64,41 @@ Future<void> analyzeWithRules(String flutterRootDirectory, List<AnalyzeRule> rul ...@@ -64,6 +64,41 @@ Future<void> analyzeWithRules(String flutterRootDirectory, List<AnalyzeRule> rul
} }
} }
Future<void> analyzeToolWithRules(String flutterRootDirectory, List<AnalyzeRule> rules) async {
final String libPath = path.canonicalize('$flutterRootDirectory/packages/flutter_tools/lib');
if (!Directory(libPath).existsSync()) {
foundError(<String>['Analyzer error: the specified $libPath does not exist.']);
}
final String testPath = path.canonicalize('$flutterRootDirectory/packages/flutter_tools/test');
final AnalysisContextCollection collection = AnalysisContextCollection(
includedPaths: <String>[libPath, testPath],
);
final List<String> analyzerErrors = <String>[];
for (final AnalysisContext context in collection.contexts) {
final Iterable<String> analyzedFilePaths = context.contextRoot.analyzedFiles();
final AnalysisSession session = context.currentSession;
for (final String filePath in analyzedFilePaths) {
final SomeResolvedUnitResult unit = await session.getResolvedUnit(filePath);
if (unit is ResolvedUnitResult) {
for (final AnalyzeRule rule in rules) {
rule.applyTo(unit);
}
} else {
analyzerErrors.add('Analyzer error: file $unit could not be resolved. Expected "ResolvedUnitResult", got ${unit.runtimeType}.');
}
}
}
if (analyzerErrors.isNotEmpty) {
foundError(analyzerErrors);
}
for (final AnalyzeRule verifier in rules) {
verifier.reportViolations(flutterRootDirectory);
}
}
/// An interface that defines a set of best practices, and collects information /// An interface that defines a set of best practices, and collects information
/// about code that violates the best practices in a [ResolvedUnitResult]. /// about code that violates the best practices in a [ResolvedUnitResult].
/// ///
......
// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:analyzer/dart/element/type.dart';
import '../utils.dart';
import 'analyze.dart';
/// Don't use Future.catchError or Future.onError.
///
/// See https://github.com/flutter/flutter/pull/130662 for more context.
///
/// **BAD:**
///
/// ```dart
/// Future<Object?> doSomething() {
/// return doSomethingAsync().catchError((_) => null);
/// }
///
/// Future<Object?> doSomethingAsync() {
/// return Future<Object>.error(Exception('error'));
/// }
/// ```
///
/// **GOOD:**
///
/// ```dart
/// Future<Object?> doSomething() {
/// return doSomethingAsync().then(
/// (Object? obj) => obj,
/// onError: (_) => null,
/// );
/// }
///
/// Future<Object?> doSomethingAsync() {
/// return Future<Object>.error(Exception('error'));
/// }
/// ```
class AvoidFutureCatchError extends AnalyzeRule {
final Map<ResolvedUnitResult, List<AstNode>> _errors = <ResolvedUnitResult, List<AstNode>>{};
@override
void applyTo(ResolvedUnitResult unit) {
final _Visitor visitor = _Visitor();
unit.unit.visitChildren(visitor);
if (visitor._offendingNodes.isNotEmpty) {
_errors.putIfAbsent(unit, () => <AstNode>[]).addAll(visitor._offendingNodes);
}
}
@override
void reportViolations(String workingDirectory) {
if (_errors.isEmpty) {
return;
}
foundError(<String>[
for (final MapEntry<ResolvedUnitResult, List<AstNode>> entry in _errors.entries)
for (final AstNode node in entry.value)
'${locationInFile(entry.key, node, workingDirectory)}: ${node.parent}',
'\n${bold}Future.catchError and Future.onError are not type safe--instead use Future.then: https://github.com/dart-lang/sdk/issues/51248$reset',
]);
}
@override
String toString() => 'Avoid "Future.catchError" and "Future.onError"';
}
class _Visitor extends RecursiveAstVisitor<void> {
_Visitor();
final List<AstNode> _offendingNodes = <AstNode>[];
@override
void visitMethodInvocation(MethodInvocation node) {
if (node.methodName.name != 'onError' && node.methodName.name != 'catchError') {
return;
}
final DartType? targetType = node.realTarget?.staticType;
if (targetType == null || !targetType.isDartAsyncFuture) {
return;
}
_offendingNodes.add(node);
}
}
...@@ -7,7 +7,6 @@ import 'package:analyzer/dart/ast/ast.dart'; ...@@ -7,7 +7,6 @@ import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart'; import 'package:analyzer/dart/ast/visitor.dart';
import 'package:analyzer/dart/element/element.dart'; import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/dart/element/type.dart'; import 'package:analyzer/dart/element/type.dart';
import 'package:path/path.dart' as path;
import '../utils.dart'; import '../utils.dart';
import 'analyze.dart'; import 'analyze.dart';
...@@ -39,14 +38,10 @@ class _NoDoubleClamp implements AnalyzeRule { ...@@ -39,14 +38,10 @@ class _NoDoubleClamp implements AnalyzeRule {
return; return;
} }
String locationInFile(ResolvedUnitResult unit, AstNode node) {
return '${path.relative(path.relative(unit.path, from: workingDirectory))}:${unit.lineInfo.getLocation(node.offset).lineNumber}';
}
foundError(<String>[ foundError(<String>[
for (final MapEntry<ResolvedUnitResult, List<AstNode>> entry in _errors.entries) for (final MapEntry<ResolvedUnitResult, List<AstNode>> entry in _errors.entries)
for (final AstNode node in entry.value) for (final AstNode node in entry.value)
'${locationInFile(entry.key, node)}: ${node.parent}', '${locationInFile(entry.key, node, workingDirectory)}: ${node.parent}',
'\n${bold}For performance reasons, we use a custom "clampDouble" function instead of using "double.clamp".$reset', '\n${bold}For performance reasons, we use a custom "clampDouble" function instead of using "double.clamp".$reset',
]); ]);
} }
......
...@@ -8,7 +8,10 @@ import 'dart:io' as system show exit; ...@@ -8,7 +8,10 @@ import 'dart:io' as system show exit;
import 'dart:io' hide exit; import 'dart:io' hide exit;
import 'dart:math' as math; import 'dart:math' as math;
import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:meta/meta.dart'; import 'package:meta/meta.dart';
import 'package:path/path.dart' as path;
const Duration _quietTimeout = Duration(minutes: 10); // how long the output should be hidden between calls to printProgress before just being verbose const Duration _quietTimeout = Duration(minutes: 10); // how long the output should be hidden between calls to printProgress before just being verbose
...@@ -253,3 +256,7 @@ Future<bool> _isPortAvailable(int port) async { ...@@ -253,3 +256,7 @@ Future<bool> _isPortAvailable(int port) async {
return true; return true;
} }
} }
String locationInFile(ResolvedUnitResult unit, AstNode node, String workingDirectory) {
return '${path.relative(path.relative(unit.path, from: workingDirectory))}:${unit.lineInfo.getLocation(node.offset).lineNumber}';
}
...@@ -190,10 +190,10 @@ class DaemonStreams { ...@@ -190,10 +190,10 @@ class DaemonStreams {
final Future<Socket> socketFuture = Socket.connect(host, port); final Future<Socket> socketFuture = Socket.connect(host, port);
final StreamController<List<int>> inputStreamController = StreamController<List<int>>(); final StreamController<List<int>> inputStreamController = StreamController<List<int>>();
final StreamController<List<int>> outputStreamController = StreamController<List<int>>(); final StreamController<List<int>> outputStreamController = StreamController<List<int>>();
socketFuture.then((Socket socket) { socketFuture.then<void>((Socket socket) {
inputStreamController.addStream(socket); inputStreamController.addStream(socket);
socket.addStream(outputStreamController.stream); socket.addStream(outputStreamController.stream);
}).onError((Object error, StackTrace stackTrace) { }, onError: (Object error, StackTrace stackTrace) {
logger.printError('Socket error: $error'); logger.printError('Socket error: $error');
logger.printTrace('$stackTrace'); logger.printTrace('$stackTrace');
// Propagate the error to the streams. // Propagate the error to the streams.
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment