From 2dd06d10c9b24ca10af33692f9a36c5505f12442 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Thu, 7 Mar 2024 10:10:07 -0800 Subject: [PATCH] [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. --- dev/bots/analyze.dart | 5 ++ dev/bots/custom_rules/analyze.dart | 35 ++++++++ .../custom_rules/avoid_future_catcherror.dart | 89 +++++++++++++++++++ dev/bots/custom_rules/no_double_clamp.dart | 7 +- dev/bots/utils.dart | 7 ++ packages/flutter_tools/lib/src/daemon.dart | 4 +- 6 files changed, 139 insertions(+), 8 deletions(-) create mode 100644 dev/bots/custom_rules/avoid_future_catcherror.dart diff --git a/dev/bots/analyze.dart b/dev/bots/analyze.dart index 0527d9b563a..36470ac5435 100644 --- a/dev/bots/analyze.dart +++ b/dev/bots/analyze.dart @@ -18,6 +18,7 @@ import 'package:path/path.dart' as path; import 'allowlist.dart'; import 'custom_rules/analyze.dart'; +import 'custom_rules/avoid_future_catcherror.dart'; import 'custom_rules/no_double_clamp.dart'; import 'custom_rules/no_stop_watches.dart'; import 'run_command.dart'; @@ -186,6 +187,10 @@ Future run(List arguments) async { await analyzeWithRules(flutterRoot, testRules, includePaths: ['packages/flutter/test'], ); + final List toolRules = [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 { printProgress('Skipped performing further analysis in the framework because "flutter analyze" finished with a non-zero exit code.'); } diff --git a/dev/bots/custom_rules/analyze.dart b/dev/bots/custom_rules/analyze.dart index 2968f5d7320..0267d653df6 100644 --- a/dev/bots/custom_rules/analyze.dart +++ b/dev/bots/custom_rules/analyze.dart @@ -64,6 +64,41 @@ Future analyzeWithRules(String flutterRootDirectory, List rul } } +Future analyzeToolWithRules(String flutterRootDirectory, List rules) async { + final String libPath = path.canonicalize('$flutterRootDirectory/packages/flutter_tools/lib'); + if (!Directory(libPath).existsSync()) { + foundError(['Analyzer error: the specified $libPath does not exist.']); + } + final String testPath = path.canonicalize('$flutterRootDirectory/packages/flutter_tools/test'); + final AnalysisContextCollection collection = AnalysisContextCollection( + includedPaths: [libPath, testPath], + ); + + final List analyzerErrors = []; + for (final AnalysisContext context in collection.contexts) { + final Iterable 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 /// about code that violates the best practices in a [ResolvedUnitResult]. /// diff --git a/dev/bots/custom_rules/avoid_future_catcherror.dart b/dev/bots/custom_rules/avoid_future_catcherror.dart new file mode 100644 index 00000000000..1ff83be792f --- /dev/null +++ b/dev/bots/custom_rules/avoid_future_catcherror.dart @@ -0,0 +1,89 @@ +// 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 doSomething() { +/// return doSomethingAsync().catchError((_) => null); +/// } +/// +/// Future doSomethingAsync() { +/// return Future.error(Exception('error')); +/// } +/// ``` +/// +/// **GOOD:** +/// +/// ```dart +/// Future doSomething() { +/// return doSomethingAsync().then( +/// (Object? obj) => obj, +/// onError: (_) => null, +/// ); +/// } +/// +/// Future doSomethingAsync() { +/// return Future.error(Exception('error')); +/// } +/// ``` +class AvoidFutureCatchError extends AnalyzeRule { + final Map> _errors = >{}; + + @override + void applyTo(ResolvedUnitResult unit) { + final _Visitor visitor = _Visitor(); + unit.unit.visitChildren(visitor); + if (visitor._offendingNodes.isNotEmpty) { + _errors.putIfAbsent(unit, () => []).addAll(visitor._offendingNodes); + } + } + + @override + void reportViolations(String workingDirectory) { + if (_errors.isEmpty) { + return; + } + + foundError([ + for (final MapEntry> 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 { + _Visitor(); + + final List _offendingNodes = []; + + @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); + } +} diff --git a/dev/bots/custom_rules/no_double_clamp.dart b/dev/bots/custom_rules/no_double_clamp.dart index a9b5836f6d9..7859decc318 100644 --- a/dev/bots/custom_rules/no_double_clamp.dart +++ b/dev/bots/custom_rules/no_double_clamp.dart @@ -7,7 +7,6 @@ import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/visitor.dart'; import 'package:analyzer/dart/element/element.dart'; import 'package:analyzer/dart/element/type.dart'; -import 'package:path/path.dart' as path; import '../utils.dart'; import 'analyze.dart'; @@ -39,14 +38,10 @@ class _NoDoubleClamp implements AnalyzeRule { return; } - String locationInFile(ResolvedUnitResult unit, AstNode node) { - return '${path.relative(path.relative(unit.path, from: workingDirectory))}:${unit.lineInfo.getLocation(node.offset).lineNumber}'; - } - foundError([ for (final MapEntry> entry in _errors.entries) 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', ]); } diff --git a/dev/bots/utils.dart b/dev/bots/utils.dart index 165fc379237..f302d628537 100644 --- a/dev/bots/utils.dart +++ b/dev/bots/utils.dart @@ -8,7 +8,10 @@ import 'dart:io' as system show exit; import 'dart:io' hide exit; 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: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 @@ -253,3 +256,7 @@ Future _isPortAvailable(int port) async { 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}'; +} diff --git a/packages/flutter_tools/lib/src/daemon.dart b/packages/flutter_tools/lib/src/daemon.dart index 1ea8908d7dc..87912456a1a 100644 --- a/packages/flutter_tools/lib/src/daemon.dart +++ b/packages/flutter_tools/lib/src/daemon.dart @@ -190,10 +190,10 @@ class DaemonStreams { final Future socketFuture = Socket.connect(host, port); final StreamController> inputStreamController = StreamController>(); final StreamController> outputStreamController = StreamController>(); - socketFuture.then((Socket socket) { + socketFuture.then((Socket socket) { inputStreamController.addStream(socket); socket.addStream(outputStreamController.stream); - }).onError((Object error, StackTrace stackTrace) { + }, onError: (Object error, StackTrace stackTrace) { logger.printError('Socket error: $error'); logger.printTrace('$stackTrace'); // Propagate the error to the streams.