From 8b2b9d7c8d18a20ea666fe33593cf9c1960d13a8 Mon Sep 17 00:00:00 2001 From: Ben Konyi Date: Mon, 18 Aug 2025 15:49:29 -0400 Subject: [PATCH] [ Widget Preview ] Don't try to instantiate invalid `@Preview()` applications (#173984) Applying `@Preview()` to an invalid AST node shouldn't cause the preview environment to throw an exception due to invalid generated code. This change adds some additional checks to ensure that invalid `@Preview()` applications are ignored. Related issue: https://github.com/flutter/flutter/issues/173959 Related stable hotfix: https://github.com/flutter/flutter/pull/173979 --- .../src/widget_preview/dependency_graph.dart | 61 +++--- .../lib/src/widget_preview/utils.dart | 3 + ...tor_invalid_preview_applications_test.dart | 181 ++++++++++++++++++ 3 files changed, 222 insertions(+), 23 deletions(-) create mode 100644 packages/flutter_tools/test/commands.shard/hermetic/widget_preview/preview_detector/preview_detector_invalid_preview_applications_test.dart diff --git a/packages/flutter_tools/lib/src/widget_preview/dependency_graph.dart b/packages/flutter_tools/lib/src/widget_preview/dependency_graph.dart index 4582f9b8ba8..84b5a8c4f0a 100644 --- a/packages/flutter_tools/lib/src/widget_preview/dependency_graph.dart +++ b/packages/flutter_tools/lib/src/widget_preview/dependency_graph.dart @@ -76,6 +76,10 @@ class _PreviewVisitor extends RecursiveAstVisitor { _scopedVisitChildren(node, (MethodDeclaration? node) => _currentMethod = node); } + bool hasRequiredParams(FormalParameterList? params) { + return params?.parameters.any((p) => p.isRequired) ?? false; + } + @override void visitAnnotation(Annotation node) { final previewsToProcess = []; @@ -88,18 +92,24 @@ class _PreviewVisitor extends RecursiveAstVisitor { } for (final preview in previewsToProcess) { - assert(_currentFunction != null || _currentConstructor != null || _currentMethod != null); - if (_currentFunction != null) { - final returnType = _currentFunction!.returnType! as NamedType; - previewEntries.add( - PreviewDetails( - packageName: packageName, - functionName: _currentFunction!.name.toString(), - isBuilder: returnType.name.isWidgetBuilder, - previewAnnotation: preview, - ), - ); - } else if (_currentConstructor != null) { + if (_currentFunction != null && + !hasRequiredParams(_currentFunction!.functionExpression.parameters)) { + final TypeAnnotation? returnTypeAnnotation = _currentFunction!.returnType; + if (returnTypeAnnotation is NamedType) { + final Token returnType = returnTypeAnnotation.name; + if (returnType.isWidget || returnType.isWidgetBuilder) { + previewEntries.add( + PreviewDetails( + packageName: packageName, + functionName: _currentFunction!.name.toString(), + isBuilder: returnType.isWidgetBuilder, + previewAnnotation: preview, + ), + ); + } + } + } else if (_currentConstructor != null && + !hasRequiredParams(_currentConstructor!.parameters)) { final returnType = _currentConstructor!.returnType as SimpleIdentifier; final Token? name = _currentConstructor!.name; previewEntries.add( @@ -110,17 +120,22 @@ class _PreviewVisitor extends RecursiveAstVisitor { previewAnnotation: preview, ), ); - } else if (_currentMethod != null) { - final returnType = _currentMethod!.returnType! as NamedType; - final parentClass = _currentMethod!.parent! as ClassDeclaration; - previewEntries.add( - PreviewDetails( - packageName: packageName, - functionName: '${parentClass.name}.${_currentMethod!.name}', - isBuilder: returnType.name.isWidgetBuilder, - previewAnnotation: preview, - ), - ); + } else if (_currentMethod != null && !hasRequiredParams(_currentMethod!.parameters)) { + final TypeAnnotation? returnTypeAnnotation = _currentMethod!.returnType; + if (returnTypeAnnotation is NamedType) { + final Token returnType = returnTypeAnnotation.name; + if (returnType.isWidget || returnType.isWidgetBuilder) { + final parentClass = _currentMethod!.parent! as ClassDeclaration; + previewEntries.add( + PreviewDetails( + packageName: packageName, + functionName: '${parentClass.name}.${_currentMethod!.name}', + isBuilder: returnType.isWidgetBuilder, + previewAnnotation: preview, + ), + ); + } + } } } } diff --git a/packages/flutter_tools/lib/src/widget_preview/utils.dart b/packages/flutter_tools/lib/src/widget_preview/utils.dart index 3a2144aeed0..b4fed3e0da8 100644 --- a/packages/flutter_tools/lib/src/widget_preview/utils.dart +++ b/packages/flutter_tools/lib/src/widget_preview/utils.dart @@ -22,6 +22,9 @@ extension TokenExtension on Token { /// Convenience getter to identify WidgetBuilder types. bool get isWidgetBuilder => toString() == 'WidgetBuilder'; + + /// Convenience getter to identify Widget types. + bool get isWidget => toString() == 'Widget'; } extension AnnotationExtension on Annotation { diff --git a/packages/flutter_tools/test/commands.shard/hermetic/widget_preview/preview_detector/preview_detector_invalid_preview_applications_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/widget_preview/preview_detector/preview_detector_invalid_preview_applications_test.dart new file mode 100644 index 00000000000..0a978cece00 --- /dev/null +++ b/packages/flutter_tools/test/commands.shard/hermetic/widget_preview/preview_detector/preview_detector_invalid_preview_applications_test.dart @@ -0,0 +1,181 @@ +// 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:flutter_tools/src/base/file_system.dart'; +import 'package:flutter_tools/src/widget_preview/dependency_graph.dart'; +import 'package:flutter_tools/src/widget_preview/preview_detector.dart'; +import 'package:test/test.dart'; + +import '../../../../src/common.dart'; +import '../../../../src/context.dart'; +import '../utils/preview_details_matcher.dart'; +import '../utils/preview_detector_test_utils.dart'; +import '../utils/preview_project.dart'; + +// Note: this test isn't under the general.shard since tests under that directory +// have a 2000ms time out and these tests write to the real file system and watch +// directories for changes. This can be slow on heavily loaded machines and cause +// flaky failures. + +/// Creates a project with files containing invalid preview applications. +class BasicProjectWithInvalidPreviews extends WidgetPreviewProject { + BasicProjectWithInvalidPreviews._({ + required super.projectRoot, + required List pathsWithPreviews, + required List pathsWithoutPreviews, + }) { + final initialSources = []; + for (final path in pathsWithPreviews) { + initialSources.add((path: path, source: _invalidPreviewContainingFileContents)); + librariesWithPreviews.add(toPreviewPath(path)); + } + for (final path in pathsWithoutPreviews) { + initialSources.add((path: path, source: _emptySource)); + librariesWithoutPreviews.add(toPreviewPath(path)); + } + initialSources.forEach(writeFile); + } + + static Future create({ + required Directory projectRoot, + required List pathsWithPreviews, + required List pathsWithoutPreviews, + }) async { + final project = BasicProjectWithInvalidPreviews._( + projectRoot: projectRoot, + pathsWithPreviews: pathsWithPreviews, + pathsWithoutPreviews: pathsWithoutPreviews, + ); + await project.initializePubspec(); + return project; + } + + final librariesWithPreviews = {}; + final librariesWithoutPreviews = {}; + + /// Adds a file containing previews at [path]. + void addPreviewContainingFile({required String path}) { + writeFile((path: path, source: _invalidPreviewContainingFileContents)); + final PreviewPath previewPath = toPreviewPath(path); + librariesWithoutPreviews.remove(previewPath); + librariesWithPreviews.add(previewPath); + } + + Map> get matcherMapping => + >{ + for (final PreviewPath path in librariesWithPreviews) path: [], + }; + + static const _emptySource = ''' +void main() {} +'''; + + static const _invalidPreviewContainingFileContents = ''' + + +@Preview(name: 'Invalid preview on class declaration') +class ClassDeclaration extends StatelessWidget { + @Preview(name: 'Invalid preview on constructor with required parameters') + ClassDeclaration(int i); + + @Preview(name: 'Invalid preview on getter'); + int get foo => 1; + + @Preview(name: 'Invalid preview on setter'); + set foo(x) { + print('foo set'); + }; + + @Preview(name: 'Invalid preview on field') + final int bar = 2; + + @Preview(name: 'Invalid preview on member function') + Widget memberFunction() => Text('Member'); + + @override + Widget build(BuildContext context) => Text('Foo'); +} + +@Preview(name: 'Invalid preview on function with void return') +void previews() => Text('Foo'); + +@Preview(name: 'Invalid preview on function with parameter') +Widget foo(int bar) => Text('Foo'); + +@Preview(name: 'Invalid preview on extension') +extension on ClassDeclaration {} +'''; +} + +void main() { + initializeTestPreviewDetectorState(); + group('$PreviewDetector', () { + // Note: we don't use a MemoryFileSystem since we don't have a way to + // provide it to package:analyzer APIs without writing a significant amount + // of wrapper logic. + late PreviewDetector previewDetector; + late BasicProjectWithInvalidPreviews project; + + setUp(() { + previewDetector = createTestPreviewDetector(); + }); + + tearDown(() async { + await previewDetector.dispose(); + }); + + testUsingContext('ignores invalid previews in existing files', () async { + project = await BasicProjectWithInvalidPreviews.create( + projectRoot: previewDetector.projectRoot, + pathsWithPreviews: ['foo.dart'], + pathsWithoutPreviews: [], + ); + final PreviewDependencyGraph mapping = await previewDetector.initialize(); + expectContainsPreviews(mapping, project.matcherMapping); + }); + + testUsingContext('ignores invalid previews in updated files', () async { + project = await BasicProjectWithInvalidPreviews.create( + projectRoot: previewDetector.projectRoot, + pathsWithPreviews: [], + pathsWithoutPreviews: ['foo.dart'], + ); + + // Initialize the file watcher. + final PreviewDependencyGraph initialPreviews = await previewDetector.initialize(); + expectContainsPreviews(initialPreviews, project.matcherMapping); + + await waitForChangeDetected( + onChangeDetected: (PreviewDependencyGraph updated) { + // There should be no valid previews in foo.dart. + expectContainsPreviews(updated, project.matcherMapping); + }, + changeOperation: () => project.addPreviewContainingFile(path: 'foo.dart'), + ); + }); + + testUsingContext('ignores invalid previews in newly added files', () async { + project = await BasicProjectWithInvalidPreviews.create( + projectRoot: previewDetector.projectRoot, + pathsWithPreviews: [], + pathsWithoutPreviews: [], + ); + // The initial mapping should be empty as there's no files containing previews. + const expectedInitialMapping = {}; + + // Initialize the file watcher. + final PreviewDependencyGraph initialPreviews = await previewDetector.initialize(); + expect(initialPreviews, expectedInitialMapping); + + await waitForChangeDetected( + onChangeDetected: (PreviewDependencyGraph updated) { + // There should be no valid previews in baz.dart. + expectContainsPreviews(updated, project.matcherMapping); + }, + // Create baz.dart, which contains previews. + changeOperation: () => project.addPreviewContainingFile(path: 'baz.dart'), + ); + }); + }); +}