From c93470a5ea5bed93e8191e50be60203878e171e0 Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Mon, 6 Nov 2023 16:06:06 -0800 Subject: [PATCH 1/2] Format fields and methods. Almost all of the real work was already done to handle variables and functions, so this just extends that to fields and methods defined in classes and covers a few loose ends: - External declarations in their various forms. - The covariant modifier on parameters and fields. - Operator declarations (which apparently were never tested on the old style!). - Getters and setters. - The `static`, `abstract`, and `late` modifiers. I also tweaked the cost heuristics around splitting between return types, parameter lists, and `=>` expression bodies. It looks pretty weird to split after a return type, so I made that more costly than splitting at the body or parameter list. I also decided to put the tests for member declarations inside declaration/ instead of member/. My impression is that there won't be a huge number of separate files in declaration/ and member/, so it's easier to just merge them together. --- lib/src/front_end/ast_node_visitor.dart | 42 +++++--- lib/src/front_end/piece_factory.dart | 61 +++++++++--- lib/src/piece/function.dart | 9 +- test/README.md | 6 +- test/declaration/external.unit | 84 ++++++++++++++++ test/declaration/field.unit | 67 +++++++++++++ test/declaration/member.unit | 41 ++++++++ test/declaration/method.unit | 123 ++++++++++++++++++++++++ test/function/expression_body.unit | 13 ++- test/function/getter.unit | 6 ++ test/function/operator.unit | 20 ++++ test/function/parameter.unit | 6 +- test/function/setter.unit | 6 ++ test/type/function.stmt | 24 ++++- test/variable/local.stmt | 12 ++- test/variable/top_level.unit | 8 +- 16 files changed, 488 insertions(+), 40 deletions(-) create mode 100644 test/declaration/external.unit create mode 100644 test/declaration/field.unit create mode 100644 test/declaration/member.unit create mode 100644 test/declaration/method.unit create mode 100644 test/function/getter.unit create mode 100644 test/function/operator.unit create mode 100644 test/function/setter.unit diff --git a/lib/src/front_end/ast_node_visitor.dart b/lib/src/front_end/ast_node_visitor.dart index 7cdf6db3..93befa18 100644 --- a/lib/src/front_end/ast_node_visitor.dart +++ b/lib/src/front_end/ast_node_visitor.dart @@ -415,7 +415,12 @@ class AstNodeVisitor extends ThrowingAstVisitor @override void visitFieldDeclaration(FieldDeclaration node) { - throw UnimplementedError(); + modifier(node.externalKeyword); + modifier(node.staticKeyword); + modifier(node.abstractKeyword); + modifier(node.covariantKeyword); + visit(node.fields); + token(node.semicolon); } @override @@ -556,19 +561,14 @@ class AstNodeVisitor extends ThrowingAstVisitor @override void visitFunctionDeclaration(FunctionDeclaration node) { - modifier(node.externalKeyword); - - Piece? returnType; - if (node.returnType case var returnTypeNode?) { - visit(returnTypeNode); - returnType = pieces.split(); - } - - // TODO(tall): Get or set keywords for getters and setters. - if (node.propertyKeyword != null) throw UnimplementedError(); - token(node.name); - - finishFunction(returnType, node.functionExpression); + createFunction( + externalKeyword: node.externalKeyword, + returnType: node.returnType, + propertyKeyword: node.propertyKeyword, + name: node.name, + typeParameters: node.functionExpression.typeParameters, + parameters: node.functionExpression.parameters, + body: node.functionExpression.body); } @override @@ -578,7 +578,7 @@ class AstNodeVisitor extends ThrowingAstVisitor @override void visitFunctionExpression(FunctionExpression node) { - finishFunction(null, node); + finishFunction(null, node.typeParameters, node.parameters, node.body); } @override @@ -786,7 +786,16 @@ class AstNodeVisitor extends ThrowingAstVisitor @override void visitMethodDeclaration(MethodDeclaration node) { - throw UnimplementedError(); + createFunction( + externalKeyword: node.externalKeyword, + modifierKeyword: node.modifierKeyword, + returnType: node.returnType, + operatorKeyword: node.operatorKeyword, + propertyKeyword: node.propertyKeyword, + name: node.name, + typeParameters: node.typeParameters, + parameters: node.parameters, + body: node.body); } @override @@ -1209,6 +1218,7 @@ class AstNodeVisitor extends ThrowingAstVisitor @override void visitTopLevelVariableDeclaration(TopLevelVariableDeclaration node) { + modifier(node.externalKeyword); visit(node.variables); token(node.semicolon); } diff --git a/lib/src/front_end/piece_factory.dart b/lib/src/front_end/piece_factory.dart index b18fd969..a06a153d 100644 --- a/lib/src/front_end/piece_factory.dart +++ b/lib/src/front_end/piece_factory.dart @@ -158,6 +158,39 @@ mixin PieceFactory implements CommentWriter { } } + /// Creates a function, method, getter, or setter declaration. + /// + /// If [modifierKeyword] is given, it should be the `static` or `abstract` + /// modifier on a method declaration. If [operatorKeyword] is given, it + /// should be the `operator` keyword on an operator declaration. If + /// [propertyKeyword] is given, it should be the `get` or `set` keyword on a + /// getter or setter declaration. + void createFunction( + {Token? externalKeyword, + Token? modifierKeyword, + AstNode? returnType, + Token? operatorKeyword, + Token? propertyKeyword, + required Token name, + TypeParameterList? typeParameters, + FormalParameterList? parameters, + required FunctionBody body}) { + modifier(externalKeyword); + modifier(modifierKeyword); + + Piece? returnTypePiece; + if (returnType != null) { + visit(returnType); + returnTypePiece = pieces.split(); + } + + modifier(operatorKeyword); + modifier(propertyKeyword); + token(name); + + finishFunction(returnTypePiece, typeParameters, parameters, body); + } + /// Creates a function type or function-typed formal. void createFunctionType( TypeAnnotation? returnType, @@ -424,7 +457,6 @@ mixin PieceFactory implements CommentWriter { List members, Token rightBracket) { if (metadata.isNotEmpty) throw UnimplementedError('Type metadata.'); - if (members.isNotEmpty) throw UnimplementedError('Type members.'); modifiers.forEach(modifier); token(keyword); @@ -483,7 +515,7 @@ mixin PieceFactory implements CommentWriter { leftBracket: leftBracket, elements, rightBracket: rightBracket, - style: const ListStyle(commas: Commas.nonTrailing, splitCost: 2)); + style: const ListStyle(commas: Commas.nonTrailing, splitCost: 3)); } /// Writes the parts of a formal parameter shared by all formal parameter @@ -492,7 +524,7 @@ mixin PieceFactory implements CommentWriter { if (parameter.metadata.isNotEmpty) throw UnimplementedError(); modifier(parameter.requiredKeyword); - if (parameter.covariantKeyword != null) throw UnimplementedError(); + modifier(parameter.covariantKeyword); } /// Handles the `async`, `sync*`, or `async*` modifiers on a function body. @@ -565,24 +597,25 @@ mixin PieceFactory implements CommentWriter { /// Finishes writing a named function declaration or anonymous function /// expression after the return type (if any) and name (if any) has been /// written. - void finishFunction(Piece? returnType, FunctionExpression function) { - visit(function.typeParameters); - visit(function.parameters); + void finishFunction(Piece? returnType, TypeParameterList? typeParameters, + FormalParameterList? parameters, FunctionBody body) { + visit(typeParameters); + visit(parameters); - Piece parameters; - Piece? body; - if (function.body case EmptyFunctionBody body) { + Piece parametersPiece; + Piece? bodyPiece; + if (body is EmptyFunctionBody) { // If the body is just `;`, then don't allow a space or split before the // semicolon by making it part of the parameters piece. token(body.semicolon); - parameters = pieces.split(); + parametersPiece = pieces.split(); } else { - parameters = pieces.split(); - visit(function.body); - body = pieces.take(); + parametersPiece = pieces.split(); + visit(body); + bodyPiece = pieces.take(); } - pieces.give(FunctionPiece(returnType, parameters, body)); + pieces.give(FunctionPiece(returnType, parametersPiece, bodyPiece)); } /// Writes an optional modifier that precedes other code. diff --git a/lib/src/piece/function.dart b/lib/src/piece/function.dart index b6f36cfc..5a519800 100644 --- a/lib/src/piece/function.dart +++ b/lib/src/piece/function.dart @@ -9,6 +9,8 @@ import 'piece.dart'; /// /// Handles splitting between the return type and the rest of the function. class FunctionPiece extends Piece { + static const _splitAfterReturnType = State(1, cost: 2); + /// The return type annotation, if any. final Piece? _returnType; @@ -22,20 +24,21 @@ class FunctionPiece extends Piece { FunctionPiece(this._returnType, this._signature, [this._body]); @override - List get additionalStates => [if (_returnType != null) State.split]; + List get additionalStates => + [if (_returnType != null) _splitAfterReturnType]; @override void format(CodeWriter writer, State state) { if (_returnType case var returnType?) { // A split inside the return type forces splitting after the return type. - writer.setAllowNewlines(state == State.split); + writer.setAllowNewlines(state == _splitAfterReturnType); writer.format(returnType); // A split in the type parameters or parameters does not force splitting // after the return type. writer.setAllowNewlines(true); - writer.splitIf(state == State.split); + writer.splitIf(state == _splitAfterReturnType); } writer.format(_signature); diff --git a/test/README.md b/test/README.md index 8fa7bcc3..5fbc70a9 100644 --- a/test/README.md +++ b/test/README.md @@ -63,10 +63,12 @@ These tests are all run by `short_format_test.dart`. The newer tall style tests are: ``` -declaration/ - Typedef, class, enum, and extension declarations. +declaration/ - Typedef, class, enum, extension, mixin, and member declarations. + Includes constructors, getters, setters, methods, and fields, + but not functions and variables, which are in their own + directories below. expression/ - Expressions. invocation/ - Function and member invocations. -member/ - Constructor, method, field, getter, and setter declarations. selection/ - Test preserving selection information. statement/ - Statements. top_level/ - Top-level directives. diff --git a/test/declaration/external.unit b/test/declaration/external.unit new file mode 100644 index 00000000..80199b5b --- /dev/null +++ b/test/declaration/external.unit @@ -0,0 +1,84 @@ +40 columns | +>>> Top-level variable. + external final a , b ; + external final Set < int > a , b ; + external var a ; + external List < int > a ; +<<< +external final a, b; +external final Set a, b; +external var a; +external List a; +>>> Static field. +class C { + external static final a , b ; +external static final Set < int > a , b ; + external static var a ; + external static List < int > a ; +} +<<< +class C { + external static final a, b; + external static final Set a, b; + external static var a; + external static List a; +} +>>> Instance field. +class C { + external final a , b ; + external final Set < String > a , b ; + external var a ; + external List < int > a ; +} +<<< +class C { + external final a, b; + external final Set a, b; + external var a; + external List a; +} +>>> Top-level function. +external int function(); +external int get getter; +external void set setter(int value); +<<< +external int function(); +external int get getter; +external void set setter(int value); +>>> Instance member. +class A { + external int function(); + external int get getter; + external void set setter(int value); +} +<<< +class A { + external int function(); + external int get getter; + external void set setter(int value); +} +>>> Static member. +class A { + external static int function(); + external static int get getter; + external static void set setter(int value); +} +<<< +class A { + external static int function(); + external static int get getter; + external static void set setter( + int value, + ); +} +>>> Don't split after `external`. +class Foo { + external var soMuchSoVeryLongFieldNameHere; + external SuperLongTypeAnnotation field; +} +<<< +class Foo { + external var soMuchSoVeryLongFieldNameHere; + external SuperLongTypeAnnotation + field; +} \ No newline at end of file diff --git a/test/declaration/field.unit b/test/declaration/field.unit new file mode 100644 index 00000000..7fbb3825 --- /dev/null +++ b/test/declaration/field.unit @@ -0,0 +1,67 @@ +40 columns | +>>> Covariant. +class Foo { + covariant var bar; + covariant int baz; +} +<<< +class Foo { + covariant var bar; + covariant int baz; +} +>>> Late. +class Foo { +static late final int i; +static late int i; +static late var i; +covariant late var i; +covariant late int i; + late final int i; + late int i; + late var i; +} +<<< +class Foo { + static late final int i; + static late int i; + static late var i; + covariant late var i; + covariant late int i; + late final int i; + late int i; + late var i; +} +>>> Abstract. +class Foo { +abstract covariant var a , b ; + abstract final int c; + abstract int i; +} +<<< +class Foo { + abstract covariant var a, b; + abstract final int c; + abstract int i; +} +>>> Don't split after `covariant`. +class Foo { + covariant var soMuchSoVeryLongFieldNameHere; + covariant VeryLongTypeAnnotation field; +} +<<< +class Foo { + covariant var soMuchSoVeryLongFieldNameHere; + covariant VeryLongTypeAnnotation + field; +} +>>> Don't split after `abstract`. +class Foo { + abstract var soMuchSoVeryLongFieldNameHere; + abstract SuperLongTypeAnnotation field; +} +<<< +class Foo { + abstract var soMuchSoVeryLongFieldNameHere; + abstract SuperLongTypeAnnotation + field; +} \ No newline at end of file diff --git a/test/declaration/member.unit b/test/declaration/member.unit new file mode 100644 index 00000000..9150bd08 --- /dev/null +++ b/test/declaration/member.unit @@ -0,0 +1,41 @@ +40 columns | +### Tests of mixed kinds of members in a declaration. +>>> Insert a blank line after non-empty block-bodied members. +class Foo { +var a = 1; b() {;} c() => null; get d {;} get e => null; set f(value) {; +} set g(value) => null; var h = 1;} +<<< +class Foo { + var a = 1; + b() { + ; + } + + c() => null; + get d { + ; + } + + get e => null; + set f(value) { + ; + } + + set g(value) => null; + var h = 1; +} +>>> No blank line required after empty block-bodied members. +class Foo { +var a = 1; b() {} c() => null; get d {} get e => null; set f(value) { +} set g(value) => null; var h = 1;} +<<< +class Foo { + var a = 1; + b() {} + c() => null; + get d {} + get e => null; + set f(value) {} + set g(value) => null; + var h = 1; +} \ No newline at end of file diff --git a/test/declaration/method.unit b/test/declaration/method.unit new file mode 100644 index 00000000..42e42680 --- /dev/null +++ b/test/declaration/method.unit @@ -0,0 +1,123 @@ +40 columns | +### Methods are formatted using the same code as function declarations, so most +### of the tests are in function/. These tests just ensure that everything still +### works inside a type declaration and test the features that can only be used +### inside a type, like `covariant`. +>>> Empty block body. +class A {void x(){}} +<<< +class A { + void x() {} +} +>>> Non-empty block body. +class A {void x(){body;}} +<<< +class A { + void x() { + body; + } +} +>>> Expression body. +class A{int x()=>42+3;} +<<< +class A { + int x() => 42 + 3; +} +>>> Static method. +class A{static bool x(){return true;}} +<<< +class A { + static bool x() { + return true; + } +} +>>> Covariant. +class A { + pos( covariant int a,covariant b ); + opt([ covariant int a,covariant b ]); + named({ covariant int a,covariant b }); + fn( covariant int f(bool b)); +} +<<< +class A { + pos(covariant int a, covariant b); + opt([covariant int a, covariant b]); + named({covariant int a, covariant b}); + fn(covariant int f(bool b)); +} +>>> Split before `covariant`. +class A { + longMethod(covariant parameterNameHere) {} +} +<<< +class A { + longMethod( + covariant parameterNameHere, + ) {} +} +>>> Split before `covariant` with multiple parameters. +class A { + longMethod(covariant first, second, covariant int third(parameter), fourth) {} +} +<<< +class A { + longMethod( + covariant first, + second, + covariant int third(parameter), + fourth, + ) {} +} +>>> Don't split after `covariant`. +class A { + longMethod(covariant int veryLongParameterNameWow) {} +} +<<< +class A { + longMethod( + covariant int + veryLongParameterNameWow, + ) {} +} +>>> Required covariant parameter. +class A { +f({ required covariant int a}) {} +} +<<< +class A { + f({required covariant int a}) {} +} +>>> Don't split between `required` and `covariant`. +class A { + longMethod({required covariant int veryLongParameterNameWow}) {} +} +<<< +class A { + longMethod({ + required covariant int + veryLongParameterNameWow, + }) {} +} +>>> Getter in type. +class A { + int get instanceProperty => 1; + static String get classProperty => "value"; +} +<<< +class A { + int get instanceProperty => 1; + static String get classProperty => + "value"; +} +>>> Setter in type. +class A { + set instanceProperty(int value) {} + static set classProperty(String value) {} +} +<<< +class A { + set instanceProperty(int value) {} + static set classProperty( + String value, + ) {} +} \ No newline at end of file diff --git a/test/function/expression_body.unit b/test/function/expression_body.unit index adb57de9..2eaf5495 100644 --- a/test/function/expression_body.unit +++ b/test/function/expression_body.unit @@ -68,4 +68,15 @@ function( longElement, longElement, longElement, -]; \ No newline at end of file +]; +>>> Prefer splitting parameters instead of after `=>`. +LongReturnType function(parameter) => longFunctionBody; +<<< +LongReturnType function( + parameter, +) => longFunctionBody; +>>> Prefer splitting after `=>` instead of after return type. +LongReturnType function() => longFunctionBody; +<<< +LongReturnType function() => + longFunctionBody; \ No newline at end of file diff --git a/test/function/getter.unit b/test/function/getter.unit new file mode 100644 index 00000000..7d89ab02 --- /dev/null +++ b/test/function/getter.unit @@ -0,0 +1,6 @@ +40 columns | +>>> Split before `get`. +VeryLongTypeAnnotation get veryLongGetter => null; +<<< +VeryLongTypeAnnotation +get veryLongGetter => null; \ No newline at end of file diff --git a/test/function/operator.unit b/test/function/operator.unit new file mode 100644 index 00000000..67857527 --- /dev/null +++ b/test/function/operator.unit @@ -0,0 +1,20 @@ +40 columns | +>>> Operator declaration. +class A { + bool operator == ( Object other ) => true ; + int operator + ( int other ) { return value + other ; } + int operator - ( ) { return - value ; } +} +<<< +class A { + bool operator ==( + Object other, + ) => true; + int operator +(int other) { + return value + other; + } + + int operator -() { + return -value; + } +} \ No newline at end of file diff --git a/test/function/parameter.unit b/test/function/parameter.unit index bdb63884..5f0420fd 100644 --- a/test/function/parameter.unit +++ b/test/function/parameter.unit @@ -33,4 +33,8 @@ function( var x, final y, final String z, -) {} \ No newline at end of file +) {} +>>> Required old style function typed parameter. +f({ required callback()}) {} +<<< +f({required callback()}) {} \ No newline at end of file diff --git a/test/function/setter.unit b/test/function/setter.unit new file mode 100644 index 00000000..8b1e89da --- /dev/null +++ b/test/function/setter.unit @@ -0,0 +1,6 @@ +40 columns | +>>> Split after `set`. +VeryLongTypeAnnotation set veryLongSetter(v) {} +<<< +VeryLongTypeAnnotation +set veryLongSetter(v) {} \ No newline at end of file diff --git a/test/type/function.stmt b/test/type/function.stmt index e74c95bc..b21622f2 100644 --- a/test/type/function.stmt +++ b/test/type/function.stmt @@ -195,4 +195,26 @@ Function(String) Function(num) Function(int) Funct Function(String) Function(num) Function(int) -Function(bool) longVariable; \ No newline at end of file +Function(bool) longVariable; +>>> Split before `required`. +longMethod({required parameterNameHere}) {} +<<< +longMethod({ + required parameterNameHere, +}) {} +>>> Split before `required` with multiple parameters. +longMethod({required first, second, required int third(parameter), fourth}) {} +<<< +longMethod({ + required first, + second, + required int third(parameter), + fourth, +}) {} +>>> Never split after `required`. +longMethod({required int reallyLongParameterNameWow}) {} +<<< +longMethod({ + required int + reallyLongParameterNameWow, +}) {} \ No newline at end of file diff --git a/test/variable/local.stmt b/test/variable/local.stmt index d57da84e..1ae2cbf9 100644 --- a/test/variable/local.stmt +++ b/test/variable/local.stmt @@ -262,4 +262,14 @@ var a = 1, element, element, element, - ]; \ No newline at end of file + ]; +>>> Late local variable. +{ + late var x = 1; + late int y = 2; +} +<<< +{ + late var x = 1; + late int y = 2; +} \ No newline at end of file diff --git a/test/variable/top_level.unit b/test/variable/top_level.unit index 55964a1e..4e12d762 100644 --- a/test/variable/top_level.unit +++ b/test/variable/top_level.unit @@ -12,4 +12,10 @@ late final longVariable = 1, SomeLongTypeName longVariable = longInitializer; <<< SomeLongTypeName longVariable = - longInitializer; \ No newline at end of file + longInitializer; +>>> Late top level variable. +late var x = 1; +late int y = 2; +<<< +late var x = 1; +late int y = 2; \ No newline at end of file From 16dcd1029fc571d1649657bc920f5793090603fb Mon Sep 17 00:00:00 2001 From: Robert Nystrom Date: Tue, 7 Nov 2023 13:46:22 -0800 Subject: [PATCH 2/2] Add tests for const fields and variables. --- test/declaration/field.unit | 16 ++++++++++++++++ test/variable/local.stmt | 10 +++++++++- test/variable/top_level.unit | 10 +++++++++- 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/test/declaration/field.unit b/test/declaration/field.unit index 7fbb3825..9a064dea 100644 --- a/test/declaration/field.unit +++ b/test/declaration/field.unit @@ -64,4 +64,20 @@ class Foo { abstract var soMuchSoVeryLongFieldNameHere; abstract SuperLongTypeAnnotation field; +} +>>> Constant. +class Foo { + static const uptyped = 123 ; + static const String typed = 'string' ; + + const uptypedInstance = 123 ; + const StringInstance typed = 'string' ; +} +<<< +class Foo { + static const uptyped = 123; + static const String typed = 'string'; + + const uptypedInstance = 123; + const StringInstance typed = 'string'; } \ No newline at end of file diff --git a/test/variable/local.stmt b/test/variable/local.stmt index 1ae2cbf9..38da4dcb 100644 --- a/test/variable/local.stmt +++ b/test/variable/local.stmt @@ -272,4 +272,12 @@ var a = 1, { late var x = 1; late int y = 2; -} \ No newline at end of file +} +>>> Constant. +const uptyped = 123 ; +<<< +const uptyped = 123; +>>> +const String typed = 'string' ; +<<< +const String typed = 'string'; \ No newline at end of file diff --git a/test/variable/top_level.unit b/test/variable/top_level.unit index 4e12d762..9b5ed3a8 100644 --- a/test/variable/top_level.unit +++ b/test/variable/top_level.unit @@ -18,4 +18,12 @@ late var x = 1; late int y = 2; <<< late var x = 1; -late int y = 2; \ No newline at end of file +late int y = 2; +>>> Constant. +const uptyped = 123 ; +<<< +const uptyped = 123; +>>> +const String typed = 'string' ; +<<< +const String typed = 'string'; \ No newline at end of file