diff --git a/dev/bots/analyze.dart b/dev/bots/analyze.dart index e43fdda7bc..0527d9b563 100644 --- a/dev/bots/analyze.dart +++ b/dev/bots/analyze.dart @@ -19,6 +19,7 @@ import 'package:path/path.dart' as path; import 'allowlist.dart'; import 'custom_rules/analyze.dart'; import 'custom_rules/no_double_clamp.dart'; +import 'custom_rules/no_stop_watches.dart'; import 'run_command.dart'; import 'utils.dart'; @@ -123,9 +124,6 @@ Future run(List arguments) async { printProgress('Goldens...'); await verifyGoldenTags(flutterPackages); - printProgress('Prevent flakes from Stopwatches...'); - await verifyNoStopwatches(flutterPackages); - printProgress('Skip test comments...'); await verifySkipTestComments(flutterRoot); @@ -175,10 +173,19 @@ Future run(List arguments) async { // Only run the private lints when the code is free of type errors. The // lints are easier to write when they can assume, for example, there is no // inheritance cycles. - final List rules = [noDoubleClamp]; + final List rules = [noDoubleClamp, noStopwatches]; final String ruleNames = rules.map((AnalyzeRule rule) => '\n * $rule').join(); printProgress('Analyzing code in the framework with the following rules:$ruleNames'); - await analyzeFrameworkWithRules(flutterRoot, rules); + await analyzeWithRules(flutterRoot, rules, + includePaths: ['packages/flutter/lib'], + excludePaths: ['packages/flutter/lib/fix_data'], + ); + final List testRules = [noStopwatches]; + final String testRuleNames = testRules.map((AnalyzeRule rule) => '\n * $rule').join(); + printProgress('Analyzing code in the test folder with the following rules:$testRuleNames'); + await analyzeWithRules(flutterRoot, testRules, + includePaths: ['packages/flutter/test'], + ); } else { printProgress('Skipped performing further analysis in the framework because "flutter analyze" finished with a non-zero exit code.'); } @@ -535,48 +542,6 @@ Future verifyGoldenTags(String workingDirectory, { int minimumMatches = 20 } } -/// Use of Stopwatches can introduce test flakes as the logical time of a -/// stopwatch can fall out of sync with the mocked time of FakeAsync in testing. -/// The Clock object provides a safe stopwatch instead, which is paired with -/// FakeAsync as part of the test binding. -final RegExp _findStopwatchPattern = RegExp(r'Stopwatch\(\)'); -const String _ignoreStopwatch = '// flutter_ignore: stopwatch (see analyze.dart)'; -const String _ignoreStopwatchForFile = '// flutter_ignore_for_file: stopwatch (see analyze.dart)'; - -Future verifyNoStopwatches(String workingDirectory, { int minimumMatches = 2000 }) async { - final List errors = []; - await for (final File file in _allFiles(workingDirectory, 'dart', minimumMatches: minimumMatches)) { - if (file.path.contains('flutter_tool')) { - // Skip flutter_tool package. - continue; - } - int lineNumber = 1; - final List lines = file.readAsLinesSync(); - for (final String line in lines) { - // If the file is being ignored, skip parsing the rest of the lines. - if (line.contains(_ignoreStopwatchForFile)) { - break; - } - - if (line.contains(_findStopwatchPattern) - && !line.contains(_leadingComment) - && !line.contains(_ignoreStopwatch)) { - // Stopwatch found - errors.add('\t${file.path}:$lineNumber'); - } - lineNumber++; - } - } - if (errors.isNotEmpty) { - foundError([ - 'Stopwatch use was found in the following files:', - ...errors, - '${bold}Stopwatches introduce flakes by falling out of sync with the FakeAsync used in testing.$reset', - 'A Stopwatch that stays in sync with FakeAsync is available through the Gesture or Test bindings, through samplingClock.' - ]); - } -} - final RegExp _findDeprecationPattern = RegExp(r'@[Dd]eprecated'); final RegExp _deprecationStartPattern = RegExp(r'^(? *)@Deprecated\($'); // flutter_ignore: deprecation_syntax (see analyze.dart) final RegExp _deprecationMessagePattern = RegExp(r"^ *'(?.+) '$"); diff --git a/dev/bots/custom_rules/analyze.dart b/dev/bots/custom_rules/analyze.dart index 95a811956f..2968f5d732 100644 --- a/dev/bots/custom_rules/analyze.dart +++ b/dev/bots/custom_rules/analyze.dart @@ -12,19 +12,31 @@ import 'package:path/path.dart' as path; import '../utils.dart'; -/// Analyzes the given `flutterRootDirectory` containing the flutter framework -/// source files, with the given [AnalyzeRule]s. +/// Analyzes the dart source files in the given `flutterRootDirectory` with the +/// given [AnalyzeRule]s. +/// +/// The `includePath` parameter takes a collection of paths relative to the given +/// `flutterRootDirectory`. It specifies the files or directory this function +/// should analyze. Defaults to null in which case this function analyzes the +/// all dart source files in `flutterRootDirectory`. +/// +/// The `excludePath` parameter takes a collection of paths relative to the given +/// `flutterRootDirectory` that this function should skip analyzing. /// /// If a compilation unit can not be resolved, this function ignores the /// corresponding dart source file and logs an error using [foundError]. -Future analyzeFrameworkWithRules(String flutterRootDirectory, List rules) async { - final String flutterLibPath = path.canonicalize('$flutterRootDirectory/packages/flutter/lib'); - if (!Directory(flutterLibPath).existsSync()) { - foundError(['Analyzer error: the specified $flutterLibPath does not exist.']); +Future analyzeWithRules(String flutterRootDirectory, List rules, { + Iterable? includePaths, + Iterable? excludePaths, +}) async { + if (!Directory(flutterRootDirectory).existsSync()) { + foundError(['Analyzer error: the specified $flutterRootDirectory does not exist.']); } + final Iterable includes = includePaths?.map((String relativePath) => path.canonicalize('$flutterRootDirectory/$relativePath')) + ?? [path.canonicalize(flutterRootDirectory)]; final AnalysisContextCollection collection = AnalysisContextCollection( - includedPaths: [flutterLibPath], - excludedPaths: [path.canonicalize('$flutterLibPath/fix_data')], + includedPaths: includes.toList(), + excludedPaths: excludePaths?.map((String relativePath) => path.canonicalize('$flutterRootDirectory/$relativePath')).toList(), ); final List analyzerErrors = []; @@ -55,7 +67,7 @@ Future analyzeFrameworkWithRules(String flutterRootDirectory, List> _errors = >{}; + + @override + void applyTo(ResolvedUnitResult unit) { + final _StopwatchVisitor visitor = _StopwatchVisitor(unit); + unit.unit.visitChildren(visitor); + final List violationsInUnit = visitor.stopwatchAccessNodes; + if (violationsInUnit.isNotEmpty) { + _errors.putIfAbsent(unit, () => []).addAll(violationsInUnit); + } + } + + @override + void reportViolations(String workingDirectory) { + if (_errors.isEmpty) { + 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}', + '\n${bold}Stopwatches introduce flakes by falling out of sync with the FakeAsync used in testing.$reset', + 'A Stopwatch that stays in sync with FakeAsync is available through the Gesture or Test bindings, through samplingClock.' + ]); + } + + @override + String toString() => 'No "Stopwatch"'; +} + +// This visitor finds invocation sites of Stopwatch (and subclasses) constructors +// and references to "external" functions that return a Stopwatch (and subclasses), +// including constructors, and put them in the stopwatchAccessNodes list. +class _StopwatchVisitor extends RecursiveAstVisitor { + _StopwatchVisitor(this.compilationUnit); + + final ResolvedUnitResult compilationUnit; + + final List stopwatchAccessNodes = []; + + final Map _isStopwatchClassElementCache = {}; + + bool _checkIfImplementsStopwatchRecurively(ClassElement classElement) { + if (classElement.library.isDartCore) { + return classElement.name == 'Stopwatch'; + } + return classElement.allSupertypes.any((InterfaceType interface) { + final InterfaceElement interfaceElement = interface.element; + return interfaceElement is ClassElement && _implementsStopwatch(interfaceElement); + }); + } + + // The cached version, call this method instead of _checkIfImplementsStopwatchRecurively. + bool _implementsStopwatch(ClassElement classElement) { + return classElement.library.isDartCore + ? classElement.name == 'Stopwatch' + :_isStopwatchClassElementCache.putIfAbsent(classElement, () => _checkIfImplementsStopwatchRecurively(classElement)); + } + + bool _isInternal(LibraryElement libraryElement) { + return path.isWithin( + compilationUnit.session.analysisContext.contextRoot.root.path, + libraryElement.source.fullName, + ); + } + + bool _hasTrailingFlutterIgnore(AstNode node) { + return compilationUnit.content + .substring(node.offset + node.length, compilationUnit.lineInfo.getOffsetOfLineAfter(node.offset + node.length)) + .contains(_ignoreStopwatch); + } + + // We don't care about directives or comments, skip them. + @override + void visitImportDirective(ImportDirective node) { } + + @override + void visitExportDirective(ExportDirective node) { } + + @override + void visitComment(Comment node) { } + + @override + void visitConstructorName(ConstructorName node) { + final Element? element = node.staticElement; + if (element is! ConstructorElement) { + assert(false, '$element of $node is not a ConstructorElement.'); + return; + } + final bool isAllowed = switch (element.returnType) { + InterfaceType(element: final ClassElement classElement) => !_implementsStopwatch(classElement), + InterfaceType(element: InterfaceElement()) => true, + }; + if (isAllowed || _hasTrailingFlutterIgnore(node)) { + return; + } + stopwatchAccessNodes.add(node); + } + + @override + void visitSimpleIdentifier(SimpleIdentifier node) { + final bool isAllowed = switch (node.staticElement) { + ExecutableElement( + returnType: DartType(element: final ClassElement classElement), + library: final LibraryElement libraryElement + ) => _isInternal(libraryElement) || !_implementsStopwatch(classElement), + Element() || null => true, + }; + if (isAllowed || _hasTrailingFlutterIgnore(node)) { + return; + } + stopwatchAccessNodes.add(node); + } +} diff --git a/dev/bots/test/analyze-test-input/root/packages/flutter/lib/stopwatch.dart b/dev/bots/test/analyze-test-input/root/packages/flutter/lib/stopwatch.dart new file mode 100644 index 0000000000..2440d849b6 --- /dev/null +++ b/dev/bots/test/analyze-test-input/root/packages/flutter/lib/stopwatch.dart @@ -0,0 +1,56 @@ +// 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 '../../foo/stopwatch_external_lib.dart' as externallib; + +typedef ExternalStopwatchConstructor = externallib.MyStopwatch Function(); + +class StopwatchAtHome extends Stopwatch { + StopwatchAtHome(); + StopwatchAtHome.create(): this(); + + Stopwatch get stopwatch => this; +} + +void testNoStopwatches(Stopwatch stopwatch) { + stopwatch.runtimeType; // OK for now, but we probably want to catch public APIs that take a Stopwatch? + final Stopwatch localVariable = Stopwatch(); // Bad: introducing Stopwatch from dart:core. + Stopwatch().runtimeType; // Bad: introducing Stopwatch from dart:core. + + (localVariable..runtimeType) // OK: not directly introducing Stopwatch. + .runtimeType; + + StopwatchAtHome().runtimeType; // Bad: introducing a Stopwatch subclass. + + Stopwatch anothorStopwatch = stopwatch; // OK: not directly introducing Stopwatch. + StopwatchAtHome Function() constructor = StopwatchAtHome.new; // Bad: introducing a Stopwatch constructor. + assert(() { + anothorStopwatch = constructor()..runtimeType; + constructor = StopwatchAtHome.create; // Bad: introducing a Stopwatch constructor. + anothorStopwatch = constructor()..runtimeType; + return true; + }()); + anothorStopwatch.runtimeType; + + externallib.MyStopwatch.create(); // Bad: introducing an external Stopwatch constructor. + ExternalStopwatchConstructor? externalConstructor; + + assert(() { + externalConstructor = externallib.MyStopwatch.new; // Bad: introducing an external Stopwatch constructor. + return true; + }()); + externalConstructor?.call(); + + externallib.stopwatch.runtimeType; // Bad: introducing an external Stopwatch. + externallib.createMyStopwatch().runtimeType; // Bad: calling an external function that returns a Stopwatch. + externallib.createStopwatch().runtimeType; // Bad: calling an external function that returns a Stopwatch. + externalConstructor = externallib.createMyStopwatch; // Bad: introducing the tear-off form of an external function that returns a Stopwatch. + + constructor.call().stopwatch; // OK: existing instance. +} + +void testStopwatchIgnore(Stopwatch stopwatch) { + Stopwatch().runtimeType; // flutter_ignore: stopwatch (see analyze.dart) + Stopwatch().runtimeType; // flutter_ignore: some_other_ignores, stopwatch (see analyze.dart) +} diff --git a/dev/bots/test/analyze-test-input/root/packages/foo/stopwatch_doc.dart b/dev/bots/test/analyze-test-input/root/packages/foo/stopwatch_doc.dart deleted file mode 100644 index 238699f92e..0000000000 --- a/dev/bots/test/analyze-test-input/root/packages/foo/stopwatch_doc.dart +++ /dev/null @@ -1,20 +0,0 @@ -// 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. - -/// Sample Code -/// -/// No analysis failures should be found. -/// -/// {@tool snippet} -/// Sample invocations of [Stopwatch]. -/// -/// ```dart -/// Stopwatch(); -/// ``` -/// {@end-tool} -String? foo; -// Other comments -// Stopwatch(); - -String literal = 'Stopwatch()'; // flutter_ignore: stopwatch (see analyze.dart) diff --git a/dev/bots/test/analyze-test-input/root/packages/foo/stopwatch_external_lib.dart b/dev/bots/test/analyze-test-input/root/packages/foo/stopwatch_external_lib.dart new file mode 100644 index 0000000000..7c4cbefb2f --- /dev/null +++ b/dev/bots/test/analyze-test-input/root/packages/foo/stopwatch_external_lib.dart @@ -0,0 +1,43 @@ +// 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. + +// External Library that creates Stopwatches. This file will not be analyzed but +// its symbols will be imported by tests. + +class MyStopwatch implements Stopwatch { + MyStopwatch(); + MyStopwatch.create(): this(); + + @override + Duration get elapsed => throw UnimplementedError(); + + @override + int get elapsedMicroseconds => throw UnimplementedError(); + + @override + int get elapsedMilliseconds => throw UnimplementedError(); + + @override + int get elapsedTicks => throw UnimplementedError(); + + @override + int get frequency => throw UnimplementedError(); + + @override + bool get isRunning => throw UnimplementedError(); + + @override + void reset() { } + + @override + void start() { } + + @override + void stop() { } +} + +final MyStopwatch stopwatch = MyStopwatch.create(); + +MyStopwatch createMyStopwatch() => MyStopwatch(); +Stopwatch createStopwatch() => Stopwatch(); diff --git a/dev/bots/test/analyze-test-input/root/packages/foo/stopwatch_fail.dart b/dev/bots/test/analyze-test-input/root/packages/foo/stopwatch_fail.dart deleted file mode 100644 index 0bad2372b3..0000000000 --- a/dev/bots/test/analyze-test-input/root/packages/foo/stopwatch_fail.dart +++ /dev/null @@ -1,14 +0,0 @@ -// 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. - -// This should fail analysis. - -void main() { - Stopwatch(); - - // Identify more than one in a file. - Stopwatch myStopwatch; - myStopwatch = Stopwatch(); - myStopwatch.reset(); -} diff --git a/dev/bots/test/analyze-test-input/root/packages/foo/stopwatch_ignore.dart b/dev/bots/test/analyze-test-input/root/packages/foo/stopwatch_ignore.dart deleted file mode 100644 index de30ea4cd6..0000000000 --- a/dev/bots/test/analyze-test-input/root/packages/foo/stopwatch_ignore.dart +++ /dev/null @@ -1,10 +0,0 @@ -// 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. - -// This would fail analysis, but it is ignored -// flutter_ignore_for_file: stopwatch (see analyze.dart) - -void main() { - Stopwatch(); -} diff --git a/dev/bots/test/analyze_test.dart b/dev/bots/test/analyze_test.dart index dc0d8bca1c..3c06d78a3c 100644 --- a/dev/bots/test/analyze_test.dart +++ b/dev/bots/test/analyze_test.dart @@ -9,6 +9,7 @@ import 'package:path/path.dart' as path; import '../analyze.dart'; import '../custom_rules/analyze.dart'; import '../custom_rules/no_double_clamp.dart'; +import '../custom_rules/no_stop_watches.dart'; import '../utils.dart'; import 'common.dart'; @@ -94,24 +95,6 @@ void main() { expect(result[result.length - 1], ''); // trailing newline }); - test('analyze.dart - verifyNoStopwatches', () async { - final List result = (await capture(() => verifyNoStopwatches(testRootPath, minimumMatches: 6), shouldHaveErrors: true)).split('\n'); - final List lines = [ - '║ \ttest/analyze-test-input/root/packages/foo/stopwatch_fail.dart:8', - '║ \ttest/analyze-test-input/root/packages/foo/stopwatch_fail.dart:12', - ] - .map((String line) => line.replaceAll('/', Platform.isWindows ? r'\' : '/')) - .toList(); - expect(result.length, 6 + lines.length, reason: 'output had unexpected number of lines:\n${result.join('\n')}'); - expect(result[0], '╔═╡ERROR╞═══════════════════════════════════════════════════════════════════════'); - expect(result[1], '║ Stopwatch use was found in the following files:'); - expect(result.getRange(2, result.length - 4).toSet(), lines.toSet()); - expect(result[result.length - 4], '║ Stopwatches introduce flakes by falling out of sync with the FakeAsync used in testing.'); - expect(result[result.length - 3], '║ A Stopwatch that stays in sync with FakeAsync is available through the Gesture or Test bindings, through samplingClock.'); - expect(result[result.length - 2], '╚═══════════════════════════════════════════════════════════════════════════════'); - expect(result[result.length - 1], ''); // trailing newline - }); - test('analyze.dart - verifyNoMissingLicense', () async { final String result = await capture(() => verifyNoMissingLicense(testRootPath, checkMinimums: false), shouldHaveErrors: true); final String file = 'test/analyze-test-input/root/packages/foo/foo.dart' @@ -230,9 +213,10 @@ void main() { }); test('analyze.dart - clampDouble', () async { - final String result = await capture(() => analyzeFrameworkWithRules( + final String result = await capture(() => analyzeWithRules( testRootPath, [noDoubleClamp], + includePaths: ['packages/flutter/lib'], ), shouldHaveErrors: true); final String lines = [ '║ packages/flutter/lib/bar.dart:37: input.clamp(0.0, 2)', @@ -252,4 +236,35 @@ void main() { '╚═══════════════════════════════════════════════════════════════════════════════\n' ); }); + + test('analyze.dart - stopwatch', () async { + final String result = await capture(() => analyzeWithRules( + testRootPath, + [noStopwatches], + includePaths: ['packages/flutter/lib'], + ), shouldHaveErrors: true); + final String lines = [ + '║ packages/flutter/lib/stopwatch.dart:18: Stopwatch()', + '║ packages/flutter/lib/stopwatch.dart:19: Stopwatch()', + '║ packages/flutter/lib/stopwatch.dart:24: StopwatchAtHome()', + '║ packages/flutter/lib/stopwatch.dart:27: StopwatchAtHome.new', + '║ packages/flutter/lib/stopwatch.dart:30: StopwatchAtHome.create', + '║ packages/flutter/lib/stopwatch.dart:36: externallib.MyStopwatch.create()', + '║ packages/flutter/lib/stopwatch.dart:40: externallib.MyStopwatch.new', + '║ packages/flutter/lib/stopwatch.dart:45: externallib.stopwatch', + '║ packages/flutter/lib/stopwatch.dart:46: externallib.createMyStopwatch()', + '║ packages/flutter/lib/stopwatch.dart:47: externallib.createStopwatch()', + '║ packages/flutter/lib/stopwatch.dart:48: externallib.createMyStopwatch' + ] + .map((String line) => line.replaceAll('/', Platform.isWindows ? r'\' : '/')) + .join('\n'); + expect(result, + '╔═╡ERROR╞═══════════════════════════════════════════════════════════════════════\n' + '$lines\n' + '║ \n' + '║ Stopwatches introduce flakes by falling out of sync with the FakeAsync used in testing.\n' + '║ A Stopwatch that stays in sync with FakeAsync is available through the Gesture or Test bindings, through samplingClock.\n' + '╚═══════════════════════════════════════════════════════════════════════════════\n' + ); + }); } diff --git a/packages/flutter/test/gestures/gesture_binding_resample_event_test.dart b/packages/flutter/test/gestures/gesture_binding_resample_event_test.dart index f5a7a5456e..1f38309f18 100644 --- a/packages/flutter/test/gestures/gesture_binding_resample_event_test.dart +++ b/packages/flutter/test/gestures/gesture_binding_resample_event_test.dart @@ -47,7 +47,8 @@ class TestSamplingClock implements SamplingClock { DateTime now() => clock.now(); @override - Stopwatch stopwatch() => clock.stopwatch(); + Stopwatch stopwatch() => clock.stopwatch(); // flutter_ignore: stopwatch (see analyze.dart) + // Ignore context: FakeAsync controls clock.stopwatch(), this is safe in tests. } typedef ResampleEventTest = void Function(FakeAsync async);