From 203a309612eeed33390fd0c173e52bc413d130ab Mon Sep 17 00:00:00 2001
From: Arne Welzel <arne.welzel@corelight.com>
Date: Thu, 24 Nov 2022 12:48:44 +0100
Subject: [PATCH] parse.y: Allow trailing commas for table, set, vector and
 record construction

Python, Ruby, Javascript, Go, ..., allow use of trailing commas and is even
recommended in some style-guides as it keeps diffs smaller. The black formatter
for Python even goes as far to take a trailing comma as an indication to
format a list one-item on a line.

It has been a bit unusual to not be able to put trailing commas in Zeek
scripts, so this change allows for it.

It explicitly prevents trailing commas in list expressions on the left
hand side. Concretely, this disallows trailing commas in the key list
expression during table initializations.

It probably allows for commas in more places that I haven't fully grasped.
Maybe we should tighten those down again if we find them surprising.
---
 NEWS                                          |  25 ++++
 src/parse.y                                   |  33 ++---
 .../language.trailing-comma-2/.stderr         |   1 +
 .../Baseline/language.trailing-comma-2/out    |   2 +
 .../language.trailing-comma-error-10/out      |   2 +
 .../language.trailing-comma-error-11/out      |   2 +
 .../language.trailing-comma-error-12/out      |   2 +
 .../language.trailing-comma-error-2/out       |   2 +
 .../language.trailing-comma-error-3/out       |   2 +
 .../language.trailing-comma-error-4/out       |   2 +
 .../language.trailing-comma-error-5/out       |   2 +
 .../language.trailing-comma-error-6/out       |   2 +
 .../language.trailing-comma-error-7/out       |   2 +
 .../language.trailing-comma-error-8/out       |   2 +
 .../language.trailing-comma-error-9/out       |   2 +
 .../language.trailing-comma-error/out         |   2 +
 .../Baseline/language.trailing-comma/.stderr  |   1 +
 .../Baseline/language.trailing-comma/out      |  51 ++++++++
 .../btest/language/trailing-comma-error.zeek  |  52 ++++++++
 testing/btest/language/trailing-comma.zeek    | 113 ++++++++++++++++++
 20 files changed, 288 insertions(+), 14 deletions(-)
 create mode 100644 testing/btest/Baseline/language.trailing-comma-2/.stderr
 create mode 100644 testing/btest/Baseline/language.trailing-comma-2/out
 create mode 100644 testing/btest/Baseline/language.trailing-comma-error-10/out
 create mode 100644 testing/btest/Baseline/language.trailing-comma-error-11/out
 create mode 100644 testing/btest/Baseline/language.trailing-comma-error-12/out
 create mode 100644 testing/btest/Baseline/language.trailing-comma-error-2/out
 create mode 100644 testing/btest/Baseline/language.trailing-comma-error-3/out
 create mode 100644 testing/btest/Baseline/language.trailing-comma-error-4/out
 create mode 100644 testing/btest/Baseline/language.trailing-comma-error-5/out
 create mode 100644 testing/btest/Baseline/language.trailing-comma-error-6/out
 create mode 100644 testing/btest/Baseline/language.trailing-comma-error-7/out
 create mode 100644 testing/btest/Baseline/language.trailing-comma-error-8/out
 create mode 100644 testing/btest/Baseline/language.trailing-comma-error-9/out
 create mode 100644 testing/btest/Baseline/language.trailing-comma-error/out
 create mode 100644 testing/btest/Baseline/language.trailing-comma/.stderr
 create mode 100644 testing/btest/Baseline/language.trailing-comma/out
 create mode 100644 testing/btest/language/trailing-comma-error.zeek
 create mode 100644 testing/btest/language/trailing-comma.zeek

diff --git a/NEWS b/NEWS
index 7369607f12..c216cf3944 100644
--- a/NEWS
+++ b/NEWS
@@ -81,6 +81,31 @@ New Functionality
   As noted under breaking changes, the blank identifier ``_`` cannot be
   referenced in expression anymore.
 
+- It is now possible to put trailing commas within table, vector, set and record
+  construction. For example, the following code is now valid, which can make
+  for more uniform style and smaller diffs.
+
+	local vec = vector(
+		"1",
+		"2",
+	);
+
+	local tab: table[string] of count = [
+		["a"] = 1,
+		["b"] = 2,
+	];
+
+  Function calls and record constructors can have a trailing comma after the
+  last argument.
+
+	Analyzer::schedule_analyzer(
+		chan$orig_h,
+		chan$resp_h,
+		chan$resp_p,
+		Analyzer::ANALYZER_FTP_DATA,
+		5mins,
+	);
+
 - Re-introduce event groups. Allow the ``&group`` attribute on event and hook
   handlers for annotating them with one or more event groups. These groups
   can be disabled and enable during runtime. Disabling an event group implies
diff --git a/src/parse.y b/src/parse.y
index c386a5e831..b0668163d3 100644
--- a/src/parse.y
+++ b/src/parse.y
@@ -146,6 +146,7 @@ static int in_enum_redef = 0;
 bool resolving_global_ID = false;
 bool defining_global_ID = false;
 std::vector<int> saved_in_init;
+static int expr_list_has_opt_comma = 0;
 
 std::vector<std::set<const ID*>> locals_at_this_scope;
 static std::unordered_set<const ID*> out_of_scope_locals;
@@ -719,7 +720,16 @@ expr:
 			$$ = new CondExpr({AdoptRef{}, $1}, {AdoptRef{}, $3}, {AdoptRef{}, $5});
 			}
 
-	|	expr '=' rhs
+	|	expr '='
+			{
+			// Prevent usage of trailing commas on the left-hand
+			// side of list expressions (e.g. in table inits).
+			if ( $1->Tag() == EXPR_LIST && expr_list_has_opt_comma )
+				$1->Error("incorrect syntax for list expression "
+				          "on left-hand side of assignment: "
+				          "trailing comma not allowed");
+			}
+		rhs
 			{
 			set_location(@1, @3);
 
@@ -728,7 +738,7 @@ expr:
 				                      " in arbitrary expression contexts, only"
 				                      " as a statement");
 
-			$$ = get_assign_expr({AdoptRef{}, $1}, {AdoptRef{}, $3}, in_init).release();
+			$$ = get_assign_expr({AdoptRef{}, $1}, {AdoptRef{}, $4}, in_init).release();
 			}
 
 	|	TOK_WHEN_LOCAL local_id '=' rhs
@@ -787,7 +797,7 @@ expr:
 			        ExprPtr{AdoptRef{}, $3}));
 			}
 
-	|	'[' expr_list ']'
+	|	'[' opt_expr_list ']'
 			{
 			set_location(@1, @3);
 
@@ -795,7 +805,8 @@ expr:
 
 			// If every expression in the list is a field assignment,
 			// then treat it as a record constructor, else as a list
-			// used for an initializer.
+			// used for an initializer. Interpret no expressions
+			// as an empty record constructor.
 
 			for ( int i = 0; i < $2->Exprs().length(); ++i )
 				{
@@ -812,13 +823,6 @@ expr:
 				$$ = $2;
 			}
 
-	|	'[' ']'
-			{
-			// We interpret this as an empty record constructor.
-			$$ = new RecordConstructorExpr(make_intrusive<ListExpr>());
-			}
-
-
 	|	TOK_RECORD '(' expr_list ')'
 			{
 			set_location(@1, @4);
@@ -1044,7 +1048,7 @@ rhs:		'{' { ++in_init; } rhs_expr_list '}'
 	|	expr
 	;
 
-rhs_expr_list: expr_list opt_comma
+rhs_expr_list: expr_list expr_list_opt_comma
 	|
 		{ $$ = new ListExpr(); }
 	;
@@ -1059,12 +1063,13 @@ expr_list:
 	|	expr
 			{
 			set_location(@1);
+			expr_list_has_opt_comma = 0;
 			$$ = new ListExpr({AdoptRef{}, $1});
 			}
 	;
 
 opt_expr_list:
-		expr_list
+		expr_list expr_list_opt_comma
 	|
 		{ $$ = new ListExpr(); }
 	;
@@ -2259,7 +2264,7 @@ opt_deprecated:
 			{ $$ = nullptr; }
 	;
 
-opt_comma: ','
+expr_list_opt_comma: ',' { expr_list_has_opt_comma = 1; }
 	|
 	;
 
diff --git a/testing/btest/Baseline/language.trailing-comma-2/.stderr b/testing/btest/Baseline/language.trailing-comma-2/.stderr
new file mode 100644
index 0000000000..49d861c74c
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma-2/.stderr
@@ -0,0 +1 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
diff --git a/testing/btest/Baseline/language.trailing-comma-2/out b/testing/btest/Baseline/language.trailing-comma-2/out
new file mode 100644
index 0000000000..563817f9c6
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma-2/out
@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+f() x=1 y=2
diff --git a/testing/btest/Baseline/language.trailing-comma-error-10/out b/testing/btest/Baseline/language.trailing-comma-error-10/out
new file mode 100644
index 0000000000..2b293365cb
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma-error-10/out
@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/trailing-comma-error.zeek, line 2: syntax error, at or near ","
diff --git a/testing/btest/Baseline/language.trailing-comma-error-11/out b/testing/btest/Baseline/language.trailing-comma-error-11/out
new file mode 100644
index 0000000000..fc60ce2920
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma-error-11/out
@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/trailing-comma-error.zeek, line 2: incorrect syntax for list expression on left-hand side of assignment: trailing comma not allowed (abc, def)
diff --git a/testing/btest/Baseline/language.trailing-comma-error-12/out b/testing/btest/Baseline/language.trailing-comma-error-12/out
new file mode 100644
index 0000000000..fc60ce2920
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma-error-12/out
@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/trailing-comma-error.zeek, line 2: incorrect syntax for list expression on left-hand side of assignment: trailing comma not allowed (abc, def)
diff --git a/testing/btest/Baseline/language.trailing-comma-error-2/out b/testing/btest/Baseline/language.trailing-comma-error-2/out
new file mode 100644
index 0000000000..71fda04efa
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma-error-2/out
@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/trailing-comma-error.zeek, line 1: syntax error, at or near ","
diff --git a/testing/btest/Baseline/language.trailing-comma-error-3/out b/testing/btest/Baseline/language.trailing-comma-error-3/out
new file mode 100644
index 0000000000..71fda04efa
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma-error-3/out
@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/trailing-comma-error.zeek, line 1: syntax error, at or near ","
diff --git a/testing/btest/Baseline/language.trailing-comma-error-4/out b/testing/btest/Baseline/language.trailing-comma-error-4/out
new file mode 100644
index 0000000000..d561b98539
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma-error-4/out
@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/trailing-comma-error.zeek, line 4: syntax error, at or near ","
diff --git a/testing/btest/Baseline/language.trailing-comma-error-5/out b/testing/btest/Baseline/language.trailing-comma-error-5/out
new file mode 100644
index 0000000000..d561b98539
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma-error-5/out
@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/trailing-comma-error.zeek, line 4: syntax error, at or near ","
diff --git a/testing/btest/Baseline/language.trailing-comma-error-6/out b/testing/btest/Baseline/language.trailing-comma-error-6/out
new file mode 100644
index 0000000000..e000ca0690
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma-error-6/out
@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/trailing-comma-error.zeek, line 1: syntax error, at or near ")"
diff --git a/testing/btest/Baseline/language.trailing-comma-error-7/out b/testing/btest/Baseline/language.trailing-comma-error-7/out
new file mode 100644
index 0000000000..2b293365cb
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma-error-7/out
@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/trailing-comma-error.zeek, line 2: syntax error, at or near ","
diff --git a/testing/btest/Baseline/language.trailing-comma-error-8/out b/testing/btest/Baseline/language.trailing-comma-error-8/out
new file mode 100644
index 0000000000..2b293365cb
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma-error-8/out
@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/trailing-comma-error.zeek, line 2: syntax error, at or near ","
diff --git a/testing/btest/Baseline/language.trailing-comma-error-9/out b/testing/btest/Baseline/language.trailing-comma-error-9/out
new file mode 100644
index 0000000000..7769d8454f
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma-error-9/out
@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/trailing-comma-error.zeek, line 2: syntax error, at or near "]"
diff --git a/testing/btest/Baseline/language.trailing-comma-error/out b/testing/btest/Baseline/language.trailing-comma-error/out
new file mode 100644
index 0000000000..a5043c16e8
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma-error/out
@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/trailing-comma-error.zeek, line 5: syntax error, at or near ","
diff --git a/testing/btest/Baseline/language.trailing-comma/.stderr b/testing/btest/Baseline/language.trailing-comma/.stderr
new file mode 100644
index 0000000000..49d861c74c
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma/.stderr
@@ -0,0 +1 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
diff --git a/testing/btest/Baseline/language.trailing-comma/out b/testing/btest/Baseline/language.trailing-comma/out
new file mode 100644
index 0000000000..3c95ca6f8d
--- /dev/null
+++ b/testing/btest/Baseline/language.trailing-comma/out
@@ -0,0 +1,51 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+vec1, vector of count, [1, 2]
+vec2, vector of count, [1, 2]
+vec3, vector of count, [1, 2]
+set1, set[count], {
+2,
+1
+}
+set2, set[count], {
+2,
+1
+}
+set3, set[count], {
+2,
+1
+}
+rec1, MyRecord, [a=a, b=<uninitialized>]
+rec2, MyRecord, [a=a, b=<uninitialized>]
+rec3, record {  }, []
+tab1, table[string] of count, {
+[a] = 1
+}
+tab2, table[string,string] of count, {
+[a, b] = 1
+}
+tab3, table[string,string] of count, {
+[a, b] = 1
+}
+tab4, table[string,string] of vector of count, {
+[a, b] = [1, 2],
+[c, d] = [3, 4]
+}
+tab5, table[string,string] of vector of count, {
+[a, b] = [1, 2],
+[c, d] = [3, 4]
+}
+tab6, table[string,string] of vector of count, {
+[a, b] = [1, 2],
+[c, d] = [3, 4]
+}
+tab7, table[string,string] of list of count,count, {
+[a, b] = 1, 2,
+[c, d] = 3, 4
+}
+tab8, table[MyRecord] of count, {
+[[a=c, b=d]] = 43,
+[[a=a, b=b]] = 42
+}
+tab9, table[MyRecord] of count, {
+[[a=abc, b=def]] = 42
+}
diff --git a/testing/btest/language/trailing-comma-error.zeek b/testing/btest/language/trailing-comma-error.zeek
new file mode 100644
index 0000000000..a2a05edb23
--- /dev/null
+++ b/testing/btest/language/trailing-comma-error.zeek
@@ -0,0 +1,52 @@
+# @TEST-EXEC: cat %INPUT
+# @TEST-EXEC-FAIL: zeek -b %INPUT >out 2>&1
+# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff out
+
+global vec = vector(, 99, 99);
+
+@TEST-START-NEXT
+global vec = vector(99, 99, , );
+
+@TEST-START-NEXT
+global rec = [,];
+
+@TEST-START-NEXT
+type MyRecord: record {
+	a: string;
+};
+global rec = MyRecord(,$a="aaa");
+
+@TEST-START-NEXT
+type MyRecord: record {
+	a: string;
+};
+global rec = MyRecord($a="aaa", ,);
+
+@TEST-START-NEXT
+function f(x: count, y: count, ) { }
+
+@TEST-START-NEXT
+function f(x: count, y: count) { }
+f(, 1, 2);
+
+@TEST-START-NEXT
+function f(x: count, y: count) { }
+f(1, 2, ,);
+
+@TEST-START-NEXT
+global tab: table[string, string] of string;
+tab["abc", "def", ] = "ghi";
+
+@TEST-START-NEXT
+global tab: table[string, string] of string;
+tab[, "abc", "def"] = "ghi";
+
+@TEST-START-NEXT
+global tab: table[string, string] of string = {
+	["abc", "def", ] = "ghi",
+};
+
+@TEST-START-NEXT
+global tab = table(
+	["abc", "def", ] = "ghi",
+);
diff --git a/testing/btest/language/trailing-comma.zeek b/testing/btest/language/trailing-comma.zeek
new file mode 100644
index 0000000000..2084651c66
--- /dev/null
+++ b/testing/btest/language/trailing-comma.zeek
@@ -0,0 +1,113 @@
+# @TEST-EXEC: zeek -b %INPUT >out
+# @TEST-EXEC: btest-diff out
+# @TEST-EXEC: btest-diff .stderr
+
+type MyRecord: record {
+	a: string;
+	b: string &optional;
+};
+
+event zeek_init()
+	{
+	local vec1 = vector(1, 2, );
+	print "vec1", type_name(vec1), vec1;
+
+	local vec2: vector of count = [1, 2, ];
+	print "vec2", type_name(vec2), vec2;
+
+	local vec3: vector of count = {1, 2, };
+	print "vec3", type_name(vec3), vec3;
+
+	local set1 = set(1, 2, );
+	print "set1", type_name(set1), set1;
+
+	local set2 = [1, 2, ];
+	print "set2", type_name(set2), set2;
+
+	local set3 = {1, 2, };
+	print "set3", type_name(set3), set3;
+
+	local rec1 = MyRecord($a="a", );
+	print "rec1", type_name(rec1), rec1;
+
+	local rec2: MyRecord = [$a="a", ];
+	print "rec2", type_name(rec2), rec2;
+
+	local rec3 = [];
+	print "rec3", type_name(rec3), rec3;
+
+	local tab1: table[string] of count = [
+		["a"] = 1,
+	];
+	print "tab1", type_name(tab1), tab1;
+
+	local tab2: table[string, string] of count = [
+		["a", "b"] = 1,
+	];
+	print "tab2", type_name(tab2), tab2;
+
+	local tab3: table[string, string] of count = {
+		["a", "b"] = 1,
+	};
+	print "tab3", type_name(tab3), tab3;
+
+	# Very verbose
+	local tab4: table[string, string] of vector of count = {
+		["a", "b"] = vector(
+			1,
+			2,
+		),
+		["c", "d"] = vector(
+			3,
+			4,
+		),
+	};
+	print "tab4", type_name(tab4), tab4;
+
+	# Slightly compressed
+	local tab5: table[string, string] of vector of count = {
+		["a", "b"] = vector(1, 2,),
+		["c", "d"] = vector(3, 4,),
+	};
+	print "tab5", type_name(tab5), tab5;
+
+	# Inferred types
+	local tab6 = table(
+		["a", "b"] = vector(1,2,),
+		["c", "d"] = vector(3,4,),
+	);
+	print "tab6", type_name(tab6), tab6;
+
+	local tab7 = table(
+		["a", "b"] = [1, 2, ],
+		["c", "d"] = [3, 4, ],
+	);
+	print "tab7", type_name(tab7), tab7;
+
+	# Trailing comma in left-hand side in record constructor expression
+	# I'm not saying these look good, just that they are possible.
+	local tab8: table[MyRecord] of count = {
+		[MyRecord(
+			$a="a",
+			$b="b",
+		)] = 42,
+		[MyRecord(
+			$a="c",
+			$b="d",
+		)] = 43,
+	};
+	print "tab8", type_name(tab8), tab8;
+
+	local tab9: table[MyRecord] of count = {
+		[[
+			$a="abc",
+			$b="def",
+		]] = 42,
+	};
+	print "tab9", type_name(tab9), tab9;
+	}
+
+@TEST-START-NEXT
+# Function calls can have trailing commas.
+function f(x: count, y: count) { print fmt("f() x=%s y=%s", x, y); }
+f(1, 2,);