From 3cd3efd29c686dcbd8e1f94aeeb62c68c55e4dbf Mon Sep 17 00:00:00 2001 From: Jake Macdonald Date: Wed, 8 May 2024 17:57:23 +0000 Subject: [PATCH] Handle diagnostics from nested builders, as well as ParallelWaitErrors with diagnostic exceptions and AsyncErrors. This fixes diagnostic reporting in the updated JSON macro which uses Future.wait and Record.wait, as well as nested builders. Change-Id: I0f80ae7f93b1ecaf24ed98512cd8a4f86e6b125a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/365840 Commit-Queue: Leaf Petersen Auto-Submit: Jake Macdonald Reviewed-by: Bob Nystrom Commit-Queue: Jake Macdonald Reviewed-by: Leaf Petersen --- pkg/_macros/CHANGELOG.md | 5 + .../lib/src/executor/builder_impls.dart | 21 ++- .../lib/src/executor/execute_macro.dart | 157 +++++++++++++++--- pkg/_macros/pubspec.yaml | 2 +- .../application/data/tests/crash.dart.expect | 6 +- pkg/macros/CHANGELOG.md | 5 + pkg/macros/pubspec.yaml | 4 +- 7 files changed, 165 insertions(+), 35 deletions(-) diff --git a/pkg/_macros/CHANGELOG.md b/pkg/_macros/CHANGELOG.md index 12a9300a245..0156f6a8fd0 100644 --- a/pkg/_macros/CHANGELOG.md +++ b/pkg/_macros/CHANGELOG.md @@ -1,3 +1,8 @@ +## 0.1.5 + +- Handle ParallelWaitError with DiagnosticException errors nicely. +- Fix a bug where we weren't reporting diagnostics for nested builders. + ## 0.1.4 - Improve formatting of constructor initializer augmentations. diff --git a/pkg/_macros/lib/src/executor/builder_impls.dart b/pkg/_macros/lib/src/executor/builder_impls.dart index eac681f80fc..013f118f100 100644 --- a/pkg/_macros/lib/src/executor/builder_impls.dart +++ b/pkg/_macros/lib/src/executor/builder_impls.dart @@ -12,7 +12,7 @@ import 'response_impls.dart'; abstract class TypeBuilderBase implements TypePhaseIntrospector, Builder { /// All the collected diagnostics for this builder. - final List _diagnostics = []; + final List _diagnostics; /// If execution was stopped by an exception, the exception. MacroExceptionImpl? _exception; @@ -60,7 +60,9 @@ abstract class TypeBuilderBase implements TypePhaseIntrospector, Builder { List? parentLibraryAugmentations, Map>? parentMixinAugmentations, Map>? parentTypeAugmentations, - }) : _enumValueAugmentations = parentEnumValueAugmentations ?? {}, + List? parentDiagnostics, + }) : _diagnostics = parentDiagnostics ?? [], + _enumValueAugmentations = parentEnumValueAugmentations ?? {}, _interfaceAugmentations = parentInterfaceAugmentations ?? {}, _libraryAugmentations = parentLibraryAugmentations ?? [], _mixinAugmentations = parentMixinAugmentations ?? {}, @@ -152,6 +154,7 @@ abstract class DeclarationBuilderBase extends TypeBuilderBase DeclarationPhaseIntrospector get introspector; DeclarationBuilderBase({ + super.parentDiagnostics, super.parentEnumValueAugmentations, super.parentInterfaceAugmentations, super.parentLibraryAugmentations, @@ -241,6 +244,7 @@ class DefinitionBuilderBase extends DeclarationBuilderBase DefinitionBuilderBase( this.introspector, { + super.parentDiagnostics, super.parentEnumValueAugmentations, super.parentInterfaceAugmentations, super.parentLibraryAugmentations, @@ -273,6 +277,7 @@ class TypeDefinitionBuilderImpl extends DefinitionBuilderBase TypeDefinitionBuilderImpl( this.declaration, super.introspector, { + super.parentDiagnostics, super.parentEnumValueAugmentations, super.parentInterfaceAugmentations, super.parentLibraryAugmentations, @@ -288,6 +293,7 @@ class TypeDefinitionBuilderImpl extends DefinitionBuilderBase .firstWhere((constructor) => constructor.identifier == identifier) as ConstructorDeclarationImpl; return ConstructorDefinitionBuilderImpl(constructor, introspector, + parentDiagnostics: _diagnostics, parentTypeAugmentations: _typeAugmentations, parentLibraryAugmentations: _libraryAugmentations); } @@ -297,6 +303,7 @@ class TypeDefinitionBuilderImpl extends DefinitionBuilderBase FieldDeclaration field = (await introspector.fieldsOf(declaration)) .firstWhere((field) => field.identifier == identifier); return VariableDefinitionBuilderImpl(field, introspector, + parentDiagnostics: _diagnostics, parentTypeAugmentations: _typeAugmentations, parentLibraryAugmentations: _libraryAugmentations); } @@ -307,6 +314,7 @@ class TypeDefinitionBuilderImpl extends DefinitionBuilderBase .firstWhere((method) => method.identifier == identifier) as MethodDeclarationImpl; return FunctionDefinitionBuilderImpl(method, introspector, + parentDiagnostics: _diagnostics, parentTypeAugmentations: _typeAugmentations, parentLibraryAugmentations: _libraryAugmentations); } @@ -336,6 +344,7 @@ class EnumDefinitionBuilderImpl extends TypeDefinitionBuilderImpl return EnumValueDefinitionBuilderImpl( entry, introspector, + parentDiagnostics: _diagnostics, parentEnumValueAugmentations: _enumValueAugmentations, parentInterfaceAugmentations: _interfaceAugmentations, parentLibraryAugmentations: _libraryAugmentations, @@ -352,6 +361,7 @@ class EnumValueDefinitionBuilderImpl extends DefinitionBuilderBase EnumValueDefinitionBuilderImpl( this.declaration, super.introspector, { + super.parentDiagnostics, super.parentEnumValueAugmentations, super.parentInterfaceAugmentations, super.parentLibraryAugmentations, @@ -375,6 +385,7 @@ class FunctionDefinitionBuilderImpl extends DefinitionBuilderBase FunctionDefinitionBuilderImpl( this.declaration, super.introspector, { + super.parentDiagnostics, super.parentEnumValueAugmentations, super.parentInterfaceAugmentations, super.parentLibraryAugmentations, @@ -404,6 +415,7 @@ class ConstructorDefinitionBuilderImpl extends DefinitionBuilderBase ConstructorDefinitionBuilderImpl( this.declaration, super.introspector, { + super.parentDiagnostics, super.parentEnumValueAugmentations, super.parentInterfaceAugmentations, super.parentLibraryAugmentations, @@ -436,6 +448,7 @@ class VariableDefinitionBuilderImpl extends DefinitionBuilderBase VariableDefinitionBuilderImpl( this.declaration, super.introspector, { + super.parentDiagnostics, super.parentEnumValueAugmentations, super.parentInterfaceAugmentations, super.parentLibraryAugmentations, @@ -473,6 +486,7 @@ class LibraryDefinitionBuilderImpl extends DefinitionBuilderBase LibraryDefinitionBuilderImpl( this.library, super.introspector, { + super.parentDiagnostics, super.parentEnumValueAugmentations, super.parentInterfaceAugmentations, super.parentLibraryAugmentations, @@ -487,6 +501,7 @@ class LibraryDefinitionBuilderImpl extends DefinitionBuilderBase .firstWhere((declaration) => declaration.identifier == identifier) as FunctionDeclarationImpl; return FunctionDefinitionBuilderImpl(function, introspector, + parentDiagnostics: _diagnostics, parentTypeAugmentations: _typeAugmentations, parentLibraryAugmentations: _libraryAugmentations); } @@ -497,6 +512,7 @@ class LibraryDefinitionBuilderImpl extends DefinitionBuilderBase .firstWhere((declaration) => declaration.identifier == identifier) as TypeDeclaration; return TypeDefinitionBuilderImpl(type, introspector, + parentDiagnostics: _diagnostics, parentTypeAugmentations: _typeAugmentations, parentLibraryAugmentations: _libraryAugmentations); } @@ -508,6 +524,7 @@ class LibraryDefinitionBuilderImpl extends DefinitionBuilderBase .firstWhere((declaration) => declaration.identifier == identifier) as VariableDeclarationImpl; return VariableDefinitionBuilderImpl(variable, introspector, + parentDiagnostics: _diagnostics, parentTypeAugmentations: _typeAugmentations, parentLibraryAugmentations: _libraryAugmentations); } diff --git a/pkg/_macros/lib/src/executor/execute_macro.dart b/pkg/_macros/lib/src/executor/execute_macro.dart index de9df195f37..e544e2931ea 100644 --- a/pkg/_macros/lib/src/executor/execute_macro.dart +++ b/pkg/_macros/lib/src/executor/execute_macro.dart @@ -2,6 +2,8 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. +import 'dart:async'; + import '../api.dart'; import '../executor.dart'; import 'builder_impls.dart'; @@ -61,15 +63,7 @@ Future executeTypesMacro( 'macro: $macro\ntarget: $target'); } } catch (e, s) { - if (e is DiagnosticException) { - builder.report(e.diagnostic); - } else if (e is MacroExceptionImpl) { - // Preserve `MacroException`s thrown by SDK tools. - builder.failWithException(e); - } else { - // Convert exceptions thrown by macro implementations into diagnostics. - builder.report(_unexpectedExceptionDiagnostic(e, s)); - } + _handleError(e, s, builder); } return builder.result; } @@ -141,15 +135,7 @@ Future executeDeclarationsMacro(Macro macro, 'macro: $macro\ntarget: $target'); } } catch (e, s) { - if (e is DiagnosticException) { - builder.report(e.diagnostic); - } else if (e is MacroExceptionImpl) { - // Preserve `MacroException`s thrown by SDK tools. - builder.failWithException(e); - } else { - // Convert exceptions thrown by macro implementations into diagnostics. - builder.report(_unexpectedExceptionDiagnostic(e, s)); - } + _handleError(e, s, builder); } return builder.result; } @@ -216,19 +202,136 @@ Future executeDefinitionMacro(Macro macro, Object target, 'macro: $macro\ntarget: $target'); } } catch (e, s) { - if (e is DiagnosticException) { - builder.report(e.diagnostic); - } else if (e is MacroExceptionImpl) { - // Preserve `MacroException`s thrown by SDK tools. - builder.failWithException(e); - } else { - // Convert exceptions thrown by macro implementations into diagnostics. - builder.report(_unexpectedExceptionDiagnostic(e, s)); - } + _handleError(e, s, builder); } return builder.result; } +/// Handles macro execution errors, specifically handling [DiagnosticException]s +/// and [MacroException]s in the expected ways. +/// +/// Also unwraps [ParallelWaitError]s and [AsyncError]s, such that we can +/// recognize properly the nested errors if they are of specially handled types. +void _handleError( + Object error, StackTrace stackTrace, TypeBuilderBase builder) { + switch (error) { + case ParallelWaitError(errors: List errors): + _handleErrors(errors, stackTrace, builder); + case ParallelWaitError(errors: (var e1,)): + _handleErrors([e1], stackTrace, builder); + case ParallelWaitError( + errors: ( + var e1, + var e2, + ) + ): + _handleErrors([e1, e2], stackTrace, builder); + case ParallelWaitError( + errors: ( + var e1, + var e2, + var e3, + ) + ): + _handleErrors([e1, e2, e3], stackTrace, builder); + case ParallelWaitError( + errors: ( + var e1, + var e2, + var e3, + var e4, + ) + ): + _handleErrors([e1, e2, e3, e4], stackTrace, builder); + case ParallelWaitError( + errors: ( + var e1, + var e2, + var e3, + var e4, + var e5, + ) + ): + _handleErrors([e1, e2, e3, e4, e5], stackTrace, builder); + case ParallelWaitError( + errors: ( + var e1, + var e2, + var e3, + var e4, + var e5, + var e6, + ) + ): + _handleErrors([e1, e2, e3, e4, e5, e6], stackTrace, builder); + case ParallelWaitError( + errors: ( + var e1, + var e2, + var e3, + var e4, + var e5, + var e6, + var e7, + ) + ): + _handleErrors([e1, e2, e3, e4, e5, e6, e7], stackTrace, builder); + case ParallelWaitError( + errors: ( + var e1, + var e2, + var e3, + var e4, + var e5, + var e6, + var e7, + var e8, + ) + ): + _handleErrors([e1, e2, e3, e4, e5, e6, e7, e8], stackTrace, builder); + case ParallelWaitError( + errors: ( + var e1, + var e2, + var e3, + var e4, + var e5, + var e6, + var e7, + var e8, + var e9, + ) + ): + _handleErrors([e1, e2, e3, e4, e5, e6, e7, e8, e9], stackTrace, builder); + // Unwrap async errors. + case AsyncError(): + _handleError(error.error, error.stackTrace, builder); + // Custom diagnostics from macros, these should just be reported. + case DiagnosticException(): + builder.report(error.diagnostic); + // Preserve `MacroException`s thrown by SDK tools. + case MacroExceptionImpl(): + builder.failWithException(error); + case _: + // Convert exceptions thrown by macro implementations into diagnostics. + builder.report(_unexpectedExceptionDiagnostic(error, stackTrace)); + } +} + +/// Handles a number of [errors], ignoring null values. +/// +/// This is used for parallel wait scenarios such as [Future.wait]. +void _handleErrors( + List errors, StackTrace outerStackTrace, TypeBuilderBase builder) { + for (var error in errors) { + if (error == null) continue; + // Passing the outerStackTrace here is the best we can do - but most of the + // time `error` will actually be an `AsyncError`, and we will end up using + // that stack trace anyways. + _handleError(error, outerStackTrace, builder); + } +} + // It's a bug in the macro but we need to show something to the user; put the // debug detail in a context message and suggest reporting to the author. Diagnostic _unexpectedExceptionDiagnostic( diff --git a/pkg/_macros/pubspec.yaml b/pkg/_macros/pubspec.yaml index 93b14280bb3..f6f14d76874 100644 --- a/pkg/_macros/pubspec.yaml +++ b/pkg/_macros/pubspec.yaml @@ -1,5 +1,5 @@ name: _macros -version: 0.1.4 +version: 0.1.5 description: >- This is a private SDK vendored package, which is re-exported by the public `macros` package, which is a pub package. Every change to this package is diff --git a/pkg/front_end/test/macros/application/data/tests/crash.dart.expect b/pkg/front_end/test/macros/application/data/tests/crash.dart.expect index 9521b232eed..5373a5b5326 100644 --- a/pkg/front_end/test/macros/application/data/tests/crash.dart.expect +++ b/pkg/front_end/test/macros/application/data/tests/crash.dart.expect @@ -7,7 +7,7 @@ library; // ^ // org-dartlang-test:///a/b/c/main.dart:7:2: Context: Error in buildTypesForClass // #0 CrashTypesMacro.buildTypesForClass (package:macro/crash.dart:13:5) -// #1 executeTypesMacro (package:_macros/src/executor/execute_macro.dart:37:21) +// #1 executeTypesMacro (package:_macros/src/executor/execute_macro.dart:39:21) // #2 MacroExpansionClient._executeTypesPhase (package:_macros/src/executor/client.dart:219:17) // #3 MacroExpansionClient._handleMessage. (package:_macros/src/executor/client.dart:145:18) // #4 _rootRun (dart:async/zone.dart:1399:13) @@ -33,7 +33,7 @@ library; // ^ // org-dartlang-test:///a/b/c/main.dart:8:2: Context: Error in buildDeclarationsForClass // #0 CrashDeclarationsMacro.buildDeclarationsForClass (package:macro/crash.dart:22:5) -// #1 executeDeclarationsMacro (package:_macros/src/executor/execute_macro.dart:113:21) +// #1 executeDeclarationsMacro (package:_macros/src/executor/execute_macro.dart:107:21) // #2 MacroExpansionClient._executeDeclarationsPhase (package:_macros/src/executor/client.dart:247:43) // #3 MacroExpansionClient._handleMessage. (package:_macros/src/executor/client.dart:135:18) // #4 _rootRun (dart:async/zone.dart:1399:13) @@ -59,7 +59,7 @@ library; // ^ // org-dartlang-test:///a/b/c/main.dart:9:2: Context: Error in buildDefinitionForClass // #0 CrashDefinitionMacro.buildDefinitionForClass (package:macro/crash.dart:31:5) -// #1 executeDefinitionMacro (package:_macros/src/executor/execute_macro.dart:181:21) +// #1 executeDefinitionMacro (package:_macros/src/executor/execute_macro.dart:167:21) // #2 MacroExpansionClient._executeDefinitionsPhase (package:_macros/src/executor/client.dart:276:17) // #3 MacroExpansionClient._handleMessage. (package:_macros/src/executor/client.dart:140:18) // #4 _rootRun (dart:async/zone.dart:1399:13) diff --git a/pkg/macros/CHANGELOG.md b/pkg/macros/CHANGELOG.md index 3b322f08054..5553ad873ba 100644 --- a/pkg/macros/CHANGELOG.md +++ b/pkg/macros/CHANGELOG.md @@ -1,3 +1,8 @@ +## 0.1.0-main.5 + +- Handle ParallelWaitError with DiagnosticException errors nicely. +- Fix a bug where we weren't reporting diagnostics for nested builders. + ## 0.1.0-main.4 - Improve formatting of constructor initializer augmentations. diff --git a/pkg/macros/pubspec.yaml b/pkg/macros/pubspec.yaml index 07d2dc269bb..6fdea15dc16 100644 --- a/pkg/macros/pubspec.yaml +++ b/pkg/macros/pubspec.yaml @@ -1,5 +1,5 @@ name: macros -version: 0.1.0-main.4 +version: 0.1.0-main.5 description: >- This package is for macro authors, and exposes the APIs necessary to write a macro. It exports the APIs from the private `_macros` SDK vendored package. @@ -11,4 +11,4 @@ environment: dependencies: _macros: sdk: dart - version: 0.1.4 + version: 0.1.5