From 156f5c8c3bfe4e3b8e7adc95f7cf24d9500f11c6 Mon Sep 17 00:00:00 2001 From: Bob Nystrom Date: Tue, 9 May 2023 13:11:09 -0700 Subject: [PATCH] Split metadata on parameters independently. (#1217) Also, don't indent parameters with metadata annotations. Before this change, the formatter indents any parameter that contains metadata annotations that end up splitting: function( @required int n, @Deprecated('Some long deprecation string.') String s) { ... } It also treats all parameters in a parameter list as a unit when deciding how to split their metadata. If any annotation in any parameter splits, then they all do. That's why there's a split after `@required` in the above example. The intent of both of these is so that a parameter with unsplit metadata doesn't get confused for an annotation on the next parameter: function( @first('some string') int n, @second('another string forces a split') String s) { ... } Note here that there are two parameters, `n`, and `s`, and not just a single parameter `s` with two annotations. Unfortunately line splitting all of the parameters as a unit is very bad for performance when there is a large number of parameters (#1212). Also, it's not very helpful. In practice, parameter metadata is rare and most parameters that have any annotations only have one. And the indentation is just strange looking and inconsistent with how annotations are formatted elsewhere. It also means that parameters with split metadata don't align with parameters that don't have metadata. This change determines whether each parameter's annotations should split independently from the other parameters' and removes that indentation. The above example becomes: function( @required int n, @Deprecated('Some long deprecation string.') String s) { ... } This improves performance on large parameter lists and I think looks better on real-world examples. I ran it on a large corpus (2,112,352 lines in 6,911 files) and I think the impact is small enough to not go through the full change process: 293 insertions + 443 deletions = 736 changes 1 changed line for every 2870.04 lines of code 0.3484 changed lines for every 1,000 lines of code The full diff is: https://gist.github.com/munificent/1dc7361438934a3587f6149049682e29 Fix #1212. --- CHANGELOG.md | 4 + lib/src/chunk_builder.dart | 30 +------ lib/src/source_visitor.dart | 44 ++-------- test/comments/functions.unit | 7 +- test/regression/0200/0247.unit | 6 +- test/regression/0300/0387.unit | 11 +-- test/regression/0400/0444.unit | 8 +- test/regression/1200/1212.unit | 153 +++++++++++++++++++++++++++++++++ test/whitespace/metadata.unit | 11 ++- 9 files changed, 182 insertions(+), 92 deletions(-) create mode 100644 test/regression/1200/1212.unit diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c7e758f..c65fcd59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # 2.3.2-dev +* Don't indent parameters that have metadata annotations. Instead, align them + with the metadata and other parameters. +* Allow metadata annotations on parameters to split independently of annotations + on other parameters (#1212). * Don't split before `.` following a record literal (#1213). * Don't force split on a line comment before a switch expression case (#1215). diff --git a/lib/src/chunk_builder.dart b/lib/src/chunk_builder.dart index 480bc792..dcf50f74 100644 --- a/lib/src/chunk_builder.dart +++ b/lib/src/chunk_builder.dart @@ -60,23 +60,6 @@ class ChunkBuilder { /// Whether the next chunk should be flush left. bool _pendingFlushLeft = false; - /// Whether subsequent hard splits should be allowed to divide for line - /// splitting. - /// - /// Most rules used by multiple chunks will never have a hard split on a - /// chunk between two chunks using that rule. That means when we see a hard - /// split, we can line split the chunks before and after it independently. - /// - /// However, there are a couple of places where a single rule spans - /// multiple chunks where hard splits also appear interleaved between them. - /// Currently, that's the rule for splitting switch case bodies, and the - /// rule for parameter list metadata. - /// - /// In those circumstances, we mark the chunk with the hard split as not - /// being allowed to divide. That way, all of the chunks using the rule are - /// split together. - bool _pendingPreventDivide = false; - /// Whether the most recently written output was a comment. bool _afterComment = false; @@ -173,7 +156,6 @@ class ChunkBuilder { _nesting.commitNesting(); _afterComment = false; - _pendingPreventDivide = false; } /// Writes one or two hard newlines. @@ -186,14 +168,10 @@ class ChunkBuilder { /// nesting. If [nest] is `true` then the next line will use expression /// nesting. void writeNewline( - {bool isDouble = false, - bool flushLeft = false, - bool nest = false, - bool preventDivide = false}) { + {bool isDouble = false, bool flushLeft = false, bool nest = false}) { _pendingNewlines = isDouble ? 2 : 1; _pendingFlushLeft = flushLeft; _pendingNested = nest; - _pendingPreventDivide |= preventDivide; } /// Writes a space before the subsequent non-whitespace text. @@ -877,11 +855,7 @@ class ChunkBuilder { isHard: isHard, isDouble: isDouble, space: space); } - if (chunk.rule.isHardened) { - _handleHardSplit(); - - if (_pendingPreventDivide) chunk.preventDivide(); - } + if (chunk.rule.isHardened) _handleHardSplit(); _pendingNewlines = 0; _pendingNested = false; diff --git a/lib/src/source_visitor.dart b/lib/src/source_visitor.dart index cbbd3dda..589c245a 100644 --- a/lib/src/source_visitor.dart +++ b/lib/src/source_visitor.dart @@ -81,14 +81,6 @@ class SourceVisitor extends ThrowingAstVisitor { /// split. final List _collectionSplits = []; - /// The stack of current rules for handling parameter metadata. - /// - /// Each time a parameter (or type parameter) list is begun, a single rule - /// for all of the metadata annotations on parameters in that list is pushed - /// onto this stack. We reuse this rule for all annotations so that they split - /// in unison. - final List _metadataRules = []; - /// The mapping for blocks that are managed by the argument list that contains /// them. /// @@ -1343,8 +1335,6 @@ class SourceVisitor extends ThrowingAstVisitor { if (nestExpression) builder.nestExpression(); token(node.leftParenthesis); - _metadataRules.add(Rule()); - PositionalRule? rule; if (requiredParams.isNotEmpty) { rule = PositionalRule(null, argumentCount: requiredParams.length); @@ -1405,8 +1395,6 @@ class SourceVisitor extends ThrowingAstVisitor { token(node.rightDelimiter); } - _metadataRules.removeLast(); - token(node.rightParenthesis); if (nestExpression) builder.unnest(); } @@ -2507,7 +2495,6 @@ class SourceVisitor extends ThrowingAstVisitor { return; } - _metadataRules.add(Rule()); token(node.leftParenthesis); builder.startRule(); @@ -2562,7 +2549,6 @@ class SourceVisitor extends ThrowingAstVisitor { builder = builder.endBlock(forceSplit: force); builder.endRule(); - _metadataRules.removeLast(); // Now write the delimiter(s) themselves. _writeText(firstClosingDelimiter.lexeme, firstClosingDelimiter); @@ -2983,11 +2969,7 @@ class SourceVisitor extends ThrowingAstVisitor { @override void visitTypeParameterList(TypeParameterList node) { - _metadataRules.add(Rule()); - _visitGenericList(node.leftBracket, node.rightBracket, node.typeParameters); - - _metadataRules.removeLast(); } @override @@ -3135,24 +3117,12 @@ class SourceVisitor extends ThrowingAstVisitor { return; } - // Split before all of the annotations or none. - builder.startLazyRule(_metadataRules.last); + // Split before all of the annotations on this parameter or none of them. + builder.startLazyRule(); - visitNodes(metadata, between: split, after: () { - // Don't nest until right before the last metadata. Ensures we only - // indent the parameter and not any of the metadata: - // - // function( - // @LongAnnotation - // @LongAnnotation - // indentedParameter) {} - builder.nestExpression(now: true); - split(); - }); + visitNodes(metadata, between: split, after: split); visitParameter(); - builder.unnest(); - // Wrap the rule around the parameter too. If it splits, we want to force // the annotations to split as well. builder.endRule(); @@ -3672,8 +3642,6 @@ class SourceVisitor extends ThrowingAstVisitor { // Can't have a trailing comma if there are no parameters. assert(parameters.parameters.isNotEmpty); - _metadataRules.add(Rule()); - // Always split the parameters. builder.startRule(Rule.hard()); @@ -3698,7 +3666,7 @@ class SourceVisitor extends ThrowingAstVisitor { builder = builder.startBlock(); for (var parameter in parameters.parameters) { - builder.writeNewline(preventDivide: true); + builder.writeNewline(); visit(parameter); _writeCommaAfter(parameter); @@ -3715,15 +3683,13 @@ class SourceVisitor extends ThrowingAstVisitor { var firstDelimiter = parameters.rightDelimiter ?? parameters.rightParenthesis; if (firstDelimiter.precedingComments != null) { - builder.writeNewline(preventDivide: true); + builder.writeNewline(); writePrecedingCommentsAndNewlines(firstDelimiter); } builder = builder.endBlock(); builder.endRule(); - _metadataRules.removeLast(); - // Now write the delimiter itself. _writeText(firstDelimiter.lexeme, firstDelimiter); if (firstDelimiter != parameters.rightParenthesis) { diff --git a/test/comments/functions.unit b/test/comments/functions.unit index d0d9ea1f..c111d03e 100644 --- a/test/comments/functions.unit +++ b/test/comments/functions.unit @@ -235,17 +235,16 @@ function({ }) { ; } ->>> metadata in trailing comma parameter list splits together even with comment +>>> splitting in none parameter's metadata doesn't force others to split function(@Annotation longParameter, // Comment. @Annotation @Other @Third longParameter2,) {} <<< function( - @Annotation - longParameter, + @Annotation longParameter, // Comment. @Annotation @Other @Third - longParameter2, + longParameter2, ) {} \ No newline at end of file diff --git a/test/regression/0200/0247.unit b/test/regression/0200/0247.unit index 218c2bde..0a845a76 100644 --- a/test/regression/0200/0247.unit +++ b/test/regression/0200/0247.unit @@ -10,8 +10,8 @@ <<< init( {@Option(help: 'The git Uri containing the jefe.yaml.', abbr: 'g') - String gitUri, + String gitUri, @Option(help: 'The directory to install into', abbr: 'd') - String installDirectory: '.', + String installDirectory: '.', @Flag(help: 'Skips the checkout of the develop branch', abbr: 's') - bool skipCheckout: false}) async {} \ No newline at end of file + bool skipCheckout: false}) async {} \ No newline at end of file diff --git a/test/regression/0300/0387.unit b/test/regression/0300/0387.unit index 88fcfbcd..387acbcd 100644 --- a/test/regression/0300/0387.unit +++ b/test/regression/0300/0387.unit @@ -7,13 +7,10 @@ greet(@Rest(valueHelp: 'who', help: 'Name(s) to greet.') List who, @Option(name: 'greeting', help: 'Alternate word to greet with e.g. "Hi".') String salutation: 'Hello'}) {} <<< -greet( - @Rest(valueHelp: 'who', help: 'Name(s) to greet.') - List who, +greet(@Rest(valueHelp: 'who', help: 'Name(s) to greet.') List who, {@Group.start(title: 'Output') @Option(help: 'How many !\'s to append.') - int enthusiasm: 0, - @Flag(abbr: 'l', help: 'Put names on separate lines.') - bool lineMode: false, + int enthusiasm: 0, + @Flag(abbr: 'l', help: 'Put names on separate lines.') bool lineMode: false, @Option(name: 'greeting', help: 'Alternate word to greet with e.g. "Hi".') - String salutation: 'Hello'}) {} \ No newline at end of file + String salutation: 'Hello'}) {} \ No newline at end of file diff --git a/test/regression/0400/0444.unit b/test/regression/0400/0444.unit index 944f0f27..8a6b6cef 100644 --- a/test/regression/0400/0444.unit +++ b/test/regression/0400/0444.unit @@ -17,10 +17,8 @@ class MyComponent { Object secondArgument; MyComponent( - @Inject(const MyFirstArgument()) - this.firstArgument, + @Inject(const MyFirstArgument()) this.firstArgument, @Inject(const MySuperDuperLongNamedArgument()) - this.superDuperLongNamedArgument, // LOOK AT ME - @Inject(const MySecondArgument()) - this.secondArgument); + this.superDuperLongNamedArgument, // LOOK AT ME + @Inject(const MySecondArgument()) this.secondArgument); } \ No newline at end of file diff --git a/test/regression/1200/1212.unit b/test/regression/1200/1212.unit new file mode 100644 index 00000000..a785c82b --- /dev/null +++ b/test/regression/1200/1212.unit @@ -0,0 +1,153 @@ +>>> +class _$_ProjectOptions extends _ProjectOptions { + const _$_ProjectOptions({required this.routeId, required this.action}): super._(); + +@override +@optionalTypeArgs TResult when({required TResult Function() initialize,required TResult Function( AppState state) receiveUpdatedAppState,required TResult Function() completedInitialFlow,required TResult Function( RouteId routeId) dismissPaywall,required TResult Function( NotificationPermission status) receiveNotificationPermissionStatus,required TResult Function( Object error, StackTrace stackTrace) receiveError,required TResult Function( Object error, StackTrace stackTrace) receiveNonFatalError,required TResult Function( Object error, StackTrace stackTrace) receiveProjectOperationError,required TResult Function( String? error) showErrorDialog,required TResult Function( bool success) handleNetworkOperationResult,required TResult Function( MineralProject project) openProject,required TResult Function( LegacyProject legacyProject, MineralProject mineralProject) migratedProject,required TResult Function( String collectionId, Template template, bool asContentCreator) openTemplate,required TResult Function( EditorSize size) openEditorWithSize,required TResult Function() openEditorAndCreateNewProject,required TResult Function( Uri uri, bool openedAppThroughUri) receivedDynamicLink,required TResult Function( NavigationAction action) mainNavigation,required TResult Function( RouteId routeId, HomeAction action) home,required TResult Function( RouteId routeId, SplashAction action) splash,required TResult Function( RouteId routeId, MagmaSurveyAction action) survey,required TResult Function( RouteId routeId, AckAction action) ack,required TResult Function( RouteId routeId, MagmaSubInfoAction action) subInfo,required TResult Function( RouteId routeId, ProjectOptionsAction action) projectOptions,required TResult Function( InitializationAction action) initialization,required TResult Function( ContentAction action) content,required TResult Function( RouteId routeId, ErrorDialogAction action) errorDialog,required TResult Function( RouteId routeId, InfoDialogAction action) infoDialog,required TResult Function( MagmaRatingConfiguration configuration) dismissedRating,required TResult Function( RouteId routeId, MagmaSupportAction action) support,required TResult Function( RouteId routeId, EditorRootAction action) clayEditor,required TResult Function( RouteId routeId, MagmaTemplateSurveyAction action) templateSurvey,required TResult Function( RouteId routeId, MagmaCollabAction action) collab,required TResult Function( RouteId routeId, MagmaRedeemCodeAction action) redeemCode,required TResult Function( RouteId routeId, OutdatedDialogAction action) outdatedDialog,required TResult Function( MagazineAction action) magazine,required TResult Function( RouteId routeId, TemplateCollectionListAction action) templateCollectionList,required TResult Function( RouteId routeId, CollectionDetailAction action) collectionDetail,required TResult Function( RouteId routeId, SimpleNewProjectAction action) simpleNewProject,required TResult Function( RouteId routeId, OnboardingFlowAction action) onboardingFlow,required TResult Function( String locale) receiveUserLocale,required TResult Function( AuthenticationAction action) authentication,required TResult Function( ToastType type, Duration? autoDismissAfter) showToast,required TResult Function() hideToast,required TResult Function() dispose,required TResult Function( LoginRouteState routeState) pushLogin,required TResult Function( RouteId routeId, UpdateEmailAction action) updateEmail,required TResult Function( RouteId routeId, DeleteAccountAction action) deleteAccount,required TResult Function( RouteId routeId, MultipleSubscriptionsAction action) multipleSubscriptions,required TResult Function( RouteId routeId, MagmaExpiredLinkAction action) expiredLink,required TResult Function( CommonPageAction action) common,required TResult Function() fetchFcmToken,required TResult Function( String? token) receivedFcmToken,required TResult Function( RouteId routeId, UnlockAction action) unlock,required TResult Function() setShouldSyncPaymentsAfterFirstStartup,required TResult Function( CustomerPurchaseResult result) handleCustomerPurchaseResult,required TResult Function() handleIntent,required TResult Function( RouteId routeId, PaymentIssuesAction action) paymentIssues,required TResult Function( RouteId routeId, SubscriptionCancellationFlowAction action) subscriptionCancellationFlow,required TResult Function( BuiltList remoteProjects) receivedRemoteProjects,required TResult Function() uploadRemoteProject,required TResult Function( bool shouldRestore) abortUpload,required TResult Function( bool background) setBackgroundState,required TResult Function( RouteId routeId, PaymentMethodSettingsAction action) paymentMethodSettings,required TResult Function( String id) showSurvey,required TResult Function( String id, Iterable templateIds) showTemplateSurvey,required TResult Function( RouteId routeId, SubscriptionAlreadyAssignedAction action) subscriptionAlreadyAssigned,}) { + return projectOptions(routeId,action); +} +} +<<< +class _$_ProjectOptions extends _ProjectOptions { + const _$_ProjectOptions({required this.routeId, required this.action}) + : super._(); + + @override + @optionalTypeArgs + TResult when({ + required TResult Function() initialize, + required TResult Function(AppState state) receiveUpdatedAppState, + required TResult Function() completedInitialFlow, + required TResult Function(RouteId routeId) + dismissPaywall, + required TResult Function(NotificationPermission status) + receiveNotificationPermissionStatus, + required TResult Function(Object error, StackTrace stackTrace) receiveError, + required TResult Function(Object error, StackTrace stackTrace) + receiveNonFatalError, + required TResult Function(Object error, StackTrace stackTrace) + receiveProjectOperationError, + required TResult Function(String? error) showErrorDialog, + required TResult Function(bool success) handleNetworkOperationResult, + required TResult Function(MineralProject project) openProject, + required TResult Function( + LegacyProject legacyProject, MineralProject mineralProject) + migratedProject, + required TResult Function( + String collectionId, Template template, bool asContentCreator) + openTemplate, + required TResult Function(EditorSize size) openEditorWithSize, + required TResult Function() openEditorAndCreateNewProject, + required TResult Function(Uri uri, bool openedAppThroughUri) + receivedDynamicLink, + required TResult Function(NavigationAction action) mainNavigation, + required TResult Function(RouteId routeId, HomeAction action) + home, + required TResult Function(RouteId routeId, SplashAction action) + splash, + required TResult Function( + RouteId routeId, MagmaSurveyAction action) + survey, + required TResult Function(RouteId routeId, AckAction action) ack, + required TResult Function( + RouteId routeId, MagmaSubInfoAction action) + subInfo, + required TResult Function( + RouteId routeId, ProjectOptionsAction action) + projectOptions, + required TResult Function(InitializationAction action) initialization, + required TResult Function(ContentAction action) content, + required TResult Function( + RouteId routeId, ErrorDialogAction action) + errorDialog, + required TResult Function( + RouteId routeId, InfoDialogAction action) + infoDialog, + required TResult Function(MagmaRatingConfiguration configuration) + dismissedRating, + required TResult Function( + RouteId routeId, MagmaSupportAction action) + support, + required TResult Function( + RouteId routeId, EditorRootAction action) + clayEditor, + required TResult Function(RouteId routeId, + MagmaTemplateSurveyAction action) + templateSurvey, + required TResult Function( + RouteId routeId, MagmaCollabAction action) + collab, + required TResult Function( + RouteId routeId, MagmaRedeemCodeAction action) + redeemCode, + required TResult Function( + RouteId routeId, OutdatedDialogAction action) + outdatedDialog, + required TResult Function(MagazineAction action) magazine, + required TResult Function(RouteId routeId, + TemplateCollectionListAction action) + templateCollectionList, + required TResult Function(RouteId routeId, + CollectionDetailAction action) + collectionDetail, + required TResult Function(RouteId routeId, + SimpleNewProjectAction action) + simpleNewProject, + required TResult Function(RouteId routeId, + OnboardingFlowAction action) + onboardingFlow, + required TResult Function(String locale) receiveUserLocale, + required TResult Function(AuthenticationAction action) authentication, + required TResult Function(ToastType type, Duration? autoDismissAfter) + showToast, + required TResult Function() hideToast, + required TResult Function() dispose, + required TResult Function(LoginRouteState routeState) pushLogin, + required TResult Function( + RouteId routeId, UpdateEmailAction action) + updateEmail, + required TResult Function( + RouteId routeId, DeleteAccountAction action) + deleteAccount, + required TResult Function(RouteId routeId, + MultipleSubscriptionsAction action) + multipleSubscriptions, + required TResult Function( + RouteId routeId, MagmaExpiredLinkAction action) + expiredLink, + required TResult Function(CommonPageAction action) common, + required TResult Function() fetchFcmToken, + required TResult Function(String? token) receivedFcmToken, + required TResult Function( + RouteId routeId, UnlockAction action) + unlock, + required TResult Function() setShouldSyncPaymentsAfterFirstStartup, + required TResult Function(CustomerPurchaseResult result) + handleCustomerPurchaseResult, + required TResult Function() handleIntent, + required TResult Function( + RouteId routeId, PaymentIssuesAction action) + paymentIssues, + required TResult Function( + RouteId routeId, + SubscriptionCancellationFlowAction action) + subscriptionCancellationFlow, + required TResult Function(BuiltList remoteProjects) + receivedRemoteProjects, + required TResult Function() uploadRemoteProject, + required TResult Function(bool shouldRestore) abortUpload, + required TResult Function(bool background) setBackgroundState, + required TResult Function(RouteId routeId, + PaymentMethodSettingsAction action) + paymentMethodSettings, + required TResult Function(String id) showSurvey, + required TResult Function(String id, Iterable templateIds) + showTemplateSurvey, + required TResult Function( + RouteId routeId, + SubscriptionAlreadyAssignedAction action) + subscriptionAlreadyAssigned, + }) { + return projectOptions(routeId, action); + } +} \ No newline at end of file diff --git a/test/whitespace/metadata.unit b/test/whitespace/metadata.unit index 7613836f..0d047095 100644 --- a/test/whitespace/metadata.unit +++ b/test/whitespace/metadata.unit @@ -238,7 +238,7 @@ function(@Annotation @VeryLongMetadataAnnotation longParameter) {} function( @Annotation @VeryLongMetadataAnnotation - longParameter) {} + longParameter) {} >>> unsplit with trailing commas function(@Annotation longParameter,@Annotation @Other longParameter2,) {} <<< @@ -250,12 +250,11 @@ function( function(@Annotation longParameter,@Annotation @Other @Third longParameter2,) {} <<< function( - @Annotation - longParameter, + @Annotation longParameter, @Annotation @Other @Third - longParameter2, + longParameter2, ) {} >>> keep "covariant" with parameter class A { function(@Annotation @VeryLongMetadataAnnotation covariant longParameter) {} } @@ -264,7 +263,7 @@ class A { function( @Annotation @VeryLongMetadataAnnotation - covariant longParameter) {} + covariant longParameter) {} } >>> keep "required" with parameter class A { function({@Annotation @VeryLongMetadataAnnotation required longParameter}) {} } @@ -273,7 +272,7 @@ class A { function( {@Annotation @VeryLongMetadataAnnotation - required longParameter}) {} + required longParameter}) {} } >>> metadata on function typedef @foo typedef Fn = Function();