Merge remote-tracking branch 'origin/topic/robin/fixes-and-cleanup'

* origin/topic/robin/fixes-and-cleanup:
  Pretty print reference types in Spicy output.
  Do not perform automatic deref on RHS of an assignment.
  Fix line numbers.
  Update NEWS with a list of incompatibilities compared to previous version.
  Fix auto-deref of LHS references.
  Improve error message.

CHANGES

@@ -1,3 +1,34 @@
+1.11.0-dev.112 | 2024-04-15 17:22:55 +0200
+
+  * Update NEWS with a list of incompatibilities compared to previous version. (Robin Sommer)
+
+  * Pretty print reference types in Spicy output. (Robin Sommer)
+
+    We now render `strong_ref<T>` as `T&`, as one would expect on
+    the Spicy side.
+
+  * Do not perform automatic deref on RHS of an assignment. (Robin Sommer)
+
+    This used to be accepted but had not the intended effect at runtime:
+
+    ```
+    function f(s: string&) {
+         *s = new "xxx";
+    }
+    ```
+
+  * Fix line numbers. (Robin Sommer)
+
+    Line numbers could be off in the presence of comments including '#'
+    characters.
+
+  * Fix auto-deref of LHS references. (Robin Sommer)
+
+    We were checking the constness of the reference, not of the wrapped
+    value.
+
+  * Improve error message. (Robin Sommer)
+
 1.11.0-dev.105 | 2024-04-10 13:24:03 +0200
 
   * Add missing file to git. (Robin Sommer, Corelight)

NEWS.rst

@@ -9,6 +9,29 @@ Version 1.11 (in progress)
 
 .. rubric:: Changed Functionality
 
+- The Spicy compiler has become a bit more strict and is now rejecting
+  some ill-defined code constructs that previous versions ended up
+  letting through. Specifically, the following cases will need
+  updating in existing code:
+
+    - Identifiers from the (internal) `hilti::` namespace are no
+      longer accessible. Usually you can just scope them with
+      `spicy::` instead.
+
+    - Previous versions did not always enforce constness as it should
+      have. In particular, function parameters could end up being
+      mutable even when they weren't declared as `inout`. Now `inout`
+      is required for supporting any mutable operations on a
+      parameter, so make sure to add it where needed.
+
+    - When using unit parameters, the type of any `inout` parameters
+      now must be unit itself. To pass other types into a unit so that
+      they can be modified by the unit, use reference instead of
+      `inout`. For example, use `type Foo = unit(s: sink&)` instead of
+      `type Foo = unit(inout: sink)`. See
+      https://docs.zeek.org/projects/spicy/en/latest/programming/parsing.html#unit-parameters
+      for more.
+
 .. rubric:: Bug fixes
 
 .. rubric:: Documentation

VERSION

@@ -1 +1 @@
-1.11.0-dev.105
+1.11.0-dev.112

doc/autogen/types/sink.rst

@@ -9,15 +9,15 @@
     parsing unit can *not* be reconnected; trying to do so will still
     throw a ``UnitAlreadyConnected`` exception.
 
-.. spicy:method:: sink::connect sink connect False void (u: strong_ref<unit>)
+.. spicy:method:: sink::connect sink connect False void (u: unit&)
 
     Connects a parsing unit to a sink. All subsequent write operations to
     the sink will pass their data on to this parsing unit. Each unit can
     only be connected to a single sink. If the unit is already connected,
     a ``UnitAlreadyConnected`` exception is thrown. However, a sink can
     have more than one unit connected to it.
 
-.. spicy:method:: sink::connect_filter sink connect_filter False void (filter: strong_ref<unit>)
+.. spicy:method:: sink::connect_filter sink connect_filter False void (filter: unit&)
 
     Connects a filter unit to the sink that will transform its input
     transparently before forwarding it for parsing to other connected

doc/autogen/types/unit.rst

@@ -6,7 +6,7 @@
     recent ``&try`` attribute. Turns into a parse error if there's no
     ``&try`` in scope.
 
-.. spicy:method:: unit::connect_filter unit connect_filter False void (filter: strong_ref<unit>)
+.. spicy:method:: unit::connect_filter unit connect_filter False void (filter: unit&)
 
     Connects a separate filter unit to transform the unit's input
     transparently before parsing. The filter unit will see the original

hilti/toolchain/src/compiler/coercer.cc

@@ -836,9 +836,16 @@ Result<std::pair<bool, Expressions>> hilti::coerceOperands(Builder* builder, ope
             case parameter::Kind::Unknown: logger().internalError("unknown operand kind"); break;
         }
 
-        if ( needs_mutable && exprs[i]->isConstant() ) {
-            HILTI_DEBUG(logging::debug::Coercer, util::fmt("  [param %d] need mutable expression -> failure", i));
-            return result::Error("parameter requires non-constant expression");
+        if ( needs_mutable ) {
+            auto t = exprs[i]->type();
+
+            if ( t->type()->isReferenceType() && (style & CoercionStyle::TryDeref) )
+                t = t->type()->dereferencedType();
+
+            if ( t->isConstant() ) {
+                HILTI_DEBUG(logging::debug::Coercer, util::fmt("  [param %d] need mutable expression -> failure", i));
+                return result::Error("parameter requires non-constant expression");
+            }
         }
 
         CoercedExpression result;
@@ -978,7 +985,8 @@ static CoercedExpression _coerceExpression(Builder* builder, Expression* e, Qual
         }
     }
 
-    if ( (style & CoercionStyle::TryDeref) && ! (style & CoercionStyle::DisallowTypeChanges) ) {
+    if ( (style & CoercionStyle::TryDeref) &&
+         ! (style & (CoercionStyle::DisallowTypeChanges | CoercionStyle::Assignment)) ) {
         if ( src->type()->isReferenceType() ) {
             auto nsrc = src->type()->dereferencedType();
             if ( type::same(nsrc, dst) )

spicy/toolchain/src/compiler/parser/scanner.ll

@@ -46,6 +46,10 @@ static std::string expandEscapes(Driver* driver, std::string s, spicy::detail::p
     }
 }
 
+static auto countNewLines(const char* s , int size) {
+    return std::count(s, s + size, '\n');
+}
+
 static std::string preprocessor_directive;
 
 %}
@@ -91,10 +95,10 @@ preprocessor @[a-zA-Z_][a-zA-Z_0-9-]*
 
 {blank}+              yylloc->step();
 [\n]+                 yylloc->lines(yyleng); yylloc->step();
-{comment}             yylloc->lines(1); yylloc->step();
-{doc_summary}         yylloc->lines(1); driver->docSummary(std::string(yytext)); yylloc->step();
-{doc_field}           yylloc->lines(1); driver->docField(std::string(yytext)); yylloc->step();
-{doc_text}            yylloc->lines(1); driver->docText(std::string(yytext)); yylloc->step();
+{comment}             yylloc->lines(countNewLines(yytext, yyleng)); yylloc->step();
+{doc_summary}         yylloc->lines(countNewLines(yytext, yyleng)); driver->docSummary(std::string(yytext)); yylloc->step();
+{doc_field}           yylloc->lines(countNewLines(yytext, yyleng)); driver->docField(std::string(yytext)); yylloc->step();
+{doc_text}            yylloc->lines(countNewLines(yytext, yyleng)); driver->docText(std::string(yytext)); yylloc->step();
 __library_type        return token::LIBRARY_TYPE;
 addr                  return token::ADDRESS;
 add                   return token::ADD;

spicy/toolchain/src/compiler/printer.cc

@@ -2,6 +2,8 @@
 
 #include <csignal>
 
+#include <hilti/ast/declarations/module.h>
+#include <hilti/ast/types/reference.h>
 #include <hilti/base/logger.h>
 #include <hilti/base/timing.h>
 #include <hilti/compiler/plugin.h>
@@ -28,6 +30,18 @@ struct VisitorPrinter : visitor::PreOrder {
         result = true;
     }
 
+    void operator()(hilti::type::StrongReference* n) final {
+        if ( auto m = n->parent<hilti::declaration::Module>(); m && m->uid().process_extension != ".spicy" )
+            return;
+
+        if ( n->isWildcard() )
+            out << "T&";
+        else
+            out << n->dereferencedType() << "&";
+
+        result = true;
+    }
+
     void operator()(type::Unit* n) final {
         if ( ! out.isExpandSubsequentType() ) {
             if ( auto id = n->typeID() ) {

spicy/toolchain/src/compiler/validator.cc

@@ -673,10 +673,10 @@ struct VisitorPost : visitor::PreOrder, hilti::validator::VisitorMixIn {
             if ( p->kind() == hilti::parameter::Kind::InOut ) {
                 auto t = p->type()->type();
                 if ( ! t->isA<type::Unit>() )
-                    error(
-                        "type of inout unit parameter must itself be a unit; for other parameter types, use references "
-                        "instead of inout",
-                        p);
+                    error(fmt("unsupported type for unit parameter '%s': type of inout unit parameters must "
+                              "itself be a unit; for other parameter types, use references instead of inout",
+                              p->id()),
+                          p);
             }
         }
 

tests/Baseline/hilti.types.struct.params-write-fail/output

@@ -1,5 +1,4 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
 [error] <...>/params-write-fail.hlt:17:5-17:12: cannot assign to constant expression: X = True
 [error] <...>/params-write-fail.hlt:18:5-18:15: unsupported operator: add <const set<string>>[<string>]
-[error] <...>/params-write-fail.hlt:19:5-19:12: cannot assign to constant expression: (*Z).a = 42
 [error] hiltic: aborting after errors

tests/Baseline/spicy.types.real.coercion-fail/output

@@ -1,7 +1,7 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
 [error] <...>/coercion-fail.spicy:7:1-7:27: cannot coerce expression '0x1.028f5c28f5c29p+0' of type 'real' to type 'bool'
 [error] <...>/coercion-fail.spicy:8:1-8:26: cannot coerce expression '0x0p+0' of type 'real' to type 'bool'
-[error] <...>/coercion-fail.spicy:13:1-13:41: cannot coerce expression '18446744073709550592' of type 'uint<64>' to type 'real'
-[error] <...>/coercion-fail.spicy:15:1-15:41: cannot coerce expression '-9223372036854775296' of type 'int<64>' to type 'real'
-[error] <...>/coercion-fail.spicy:17:1-17:34: cannot coerce expression '9007199254740993' of type 'uint<64>' to type 'real'
+[error] <...>/coercion-fail.spicy:12:1-12:41: cannot coerce expression '18446744073709550592' of type 'uint<64>' to type 'real'
+[error] <...>/coercion-fail.spicy:14:1-14:41: cannot coerce expression '-9223372036854775296' of type 'int<64>' to type 'real'
+[error] <...>/coercion-fail.spicy:16:1-16:34: cannot coerce expression '9007199254740993' of type 'uint<64>' to type 'real'
 [error] spicyc: aborting after errors

tests/Baseline/spicy.types.struct.init-fail/output

@@ -1,4 +1,4 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-[error] <...>/init-fail.spicy:14:1-14:21: declaration needs a concrete struct type
-[error] <...>/init-fail.spicy:18:5-18:24: declaration needs a concrete struct type
+[error] <...>/init-fail.spicy:13:1-13:21: declaration needs a concrete struct type
+[error] <...>/init-fail.spicy:17:5-17:24: declaration needs a concrete struct type
 [error] spicyc: aborting after errors

tests/Baseline/spicy.types.unit.begin-uninit/output

@@ -1,2 +1,2 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-[error] terminating with uncaught exception of type hilti::rt::AssertionFailure: failed expression '(*self).__offset != 0' (<...>/begin-uninit.spicy:10:16-10:41)
+[error] terminating with uncaught exception of type hilti::rt::AssertionFailure: failed expression '(*self).__offset != 0' (<...>/begin-uninit.spicy:9:16-9:41)

tests/Baseline/spicy.types.unit.context-fail/output

@@ -4,6 +4,6 @@
 [error] <...>/context-fail.spicy:17:5-17:18: %context requires a type
 [error] <...>/context-fail.spicy:20:17-23:1: unit cannot have more than one %context
 [error] <...>/context-fail.spicy:26:16-26:29: context() used with a unit which did not declare %context
-[error] <...>/context-fail.spicy:33:3-33:18: %context requires a type
-[error] <...>/context-fail.spicy:35:14-35:27: context() used with a unit which did not declare %context
+[error] <...>/context-fail.spicy:32:3-32:18: %context requires a type
+[error] <...>/context-fail.spicy:34:14-34:27: context() used with a unit which did not declare %context
 [error] spicyc: aborting after errors

tests/Baseline/spicy.types.unit.context-undeclared/output

@@ -1,3 +1,3 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-[error] <...>/context-undeclared.spicy:10:15-10:28: context() used with a unit which did not declare %context
+[error] <...>/context-undeclared.spicy:9:15-9:28: context() used with a unit which did not declare %context
 [error] spicyc: aborting after errors

tests/Baseline/spicy.types.unit.params-inout-fail/output

@@ -1,4 +1,4 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-[error] <...>/params-inout-fail.spicy:10:15-10:30: type of inout unit parameter must itself be a unit; for other parameter types, use references instead of inout
-[error] <...>/params-inout-fail.spicy:12:15-12:29: type of inout unit parameter must itself be a unit; for other parameter types, use references instead of inout
+[error] <...>/params-inout-fail.spicy:10:15-10:30: unsupported type for unit parameter 'n' (type of inout unit parameters must itself be a unit; for other parameter types, use references instead of inout)
+[error] <...>/params-inout-fail.spicy:12:15-12:29: unsupported type for unit parameter 'n' (type of inout unit parameters must itself be a unit; for other parameter types, use references instead of inout)
 [error] spicyc: aborting after errors

tests/Baseline/spicy.types.unit.params-reference-fail/output

@@ -0,0 +1,3 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+[error] <...>/params-reference-fail.spicy:7:5-7:18: cannot coerce expression 'new "xxx"()' of type 'string&' to type 'string'
+[error] spicyc: aborting after errors

tests/Baseline/spicy.types.unit.params-reference/output

@@ -1,3 +1,4 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
 X, old
 Y, new
+struct_, [$x="xxx"]

tests/Baseline/spicy.types.unit.params-sink-fail-2/output

@@ -1,3 +1,3 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-[error] <...>/params-sink-fail.spicy:4:15-4:27: type of inout unit parameter must itself be a unit; for other parameter types, use references instead of inout
+[error] <...>/params-sink-fail.spicy:4:15-4:27: unsupported type for unit parameter 's' (type of inout unit parameters must itself be a unit; for other parameter types, use references instead of inout)
 [error] spicy-driver: aborting after errors

tests/Baseline/spicy.types.unit.params-write-fail/output

@@ -1,4 +1,3 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
 [error] <...>/params-write-fail.spicy:16:9-16:17: cannot assign to constant expression: x = False
-[error] <...>/params-write-fail.spicy:17:9-17:16: cannot assign to constant expression: (*y).a = 42
 [error] spicyc: aborting after errors

tests/Baseline/spicy.types.unit.size-validation-fail/output

@@ -1,4 +1,4 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-[error] <...>/size-validation-fail.spicy:9:11-11:1: &size expression cannot use 'self' since it is only available after parsing of unit has started
-[error] <...>/size-validation-fail.spicy:13:11-15:1: &max-size expression cannot use 'self' since it is only available after parsing of unit has started
+[error] <...>/size-validation-fail.spicy:8:11-10:1: &size expression cannot use 'self' since it is only available after parsing of unit has started
+[error] <...>/size-validation-fail.spicy:12:11-14:1: &max-size expression cannot use 'self' since it is only available after parsing of unit has started
 [error] spicyc: aborting after errors

tests/Baseline/spicy.types.unit.struct-ctor-init-2/output

@@ -1,4 +1,4 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-[error] <...>/struct-ctor-init-2.spicy:14:1-14:9: unsupported operator: <map<Key, uint<8>>>[<struct { uint<64> a; }>]
-[error] <...>/struct-ctor-init-2.spicy:13:11-13:25: type cannot be used as key type for maps (because type 'Key' is not sortable)
+[error] <...>/struct-ctor-init-2.spicy:13:1-13:9: unsupported operator: <map<Key, uint<8>>>[<struct { uint<64> a; }>]
+[error] <...>/struct-ctor-init-2.spicy:12:11-12:25: type cannot be used as key type for maps (because type 'Key' is not sortable)
 [error] spicyc: aborting after errors

tests/Baseline/spicy.types.unit.synchronize-nested/output

@@ -9,10 +9,10 @@
 [$x=(not set), $a=[$a1=(not set), $a2=b"2", $a3=(not set), $a4=(not set)], $as=[]]
 [$x=(not set), $a=[$a1=(not set), $a2=(not set), $a3=3, $a4=(not set)], $as=[]]
 [$x=(not set), $a=[$a1=(not set), $a2=(not set), $a3=(not set), $a4=b"4"], $as=[]]
-[error] terminating with uncaught exception of type spicy::rt::ParseError: failed to synchronize: failed to match regular expression (<...>/synchronize-nested.spicy:33:8-33:10)
+[error] terminating with uncaught exception of type spicy::rt::ParseError: failed to synchronize: failed to match regular expression (<...>/synchronize-nested.spicy:30:8-30:10)
 # Synchronize on vector field
 [$x=b"X", $a=[$a1=(not set), $a2=(not set), $a3=(not set), $a4=(not set)], $as=[[$a1=b"1", $a2=(not set), $a3=(not set), $a4=(not set)]]]
 [$x=b"X", $a=[$a1=(not set), $a2=(not set), $a3=(not set), $a4=(not set)], $as=[[$a1=(not set), $a2=b"2", $a3=(not set), $a4=(not set)]]]
 [$x=b"X", $a=[$a1=(not set), $a2=(not set), $a3=(not set), $a4=(not set)], $as=[[$a1=(not set), $a2=(not set), $a3=3, $a4=(not set)]]]
 [$x=b"X", $a=[$a1=(not set), $a2=(not set), $a3=(not set), $a4=(not set)], $as=[[$a1=(not set), $a2=(not set), $a3=(not set), $a4=b"4"]]]
-[error] terminating with uncaught exception of type spicy::rt::ParseError: failed to synchronize: no expected look-ahead token found (<...>/synchronize-nested.spicy:48:9-48:19)
+[error] terminating with uncaught exception of type spicy::rt::ParseError: failed to synchronize: no expected look-ahead token found (<...>/synchronize-nested.spicy:45:9-45:19)

tests/Baseline/spicy.types.unit.synchronize/output

@@ -192,7 +192,7 @@ Done: [$a=b"A", $b=(not set), $c=(not set), $d=b"D", $e=b"E"]
 [spicy-verbose]     - state: type=test::A input="" stream=0xXXXXXXXX offsets=2/0/2/2 chunks=0 frozen=yes mode=default trim=yes lah=n/a lah_token="n/a" recovering=yes
 [spicy-verbose]     - state: type=test::A input="" stream=0xXXXXXXXX offsets=2/0/2/2 chunks=0 frozen=yes mode=default trim=yes lah=n/a lah_token="n/a" recovering=yes
 Error: [$a=b"A", $b=(not set), $c=(not set), $d=(not set), $e=(not set)]
-[error] terminating with uncaught exception of type spicy::rt::ParseError: failed to synchronize: expecting 'B' (<...>/synchronize.spicy:40:8-40:11)
+[error] terminating with uncaught exception of type spicy::rt::ParseError: failed to synchronize: expecting 'B' (<...>/synchronize.spicy:39:8-39:11)
 [spicy-verbose] - state: type=test::A input="A" stream=0xXXXXXXXX offsets=0/0/0/1 chunks=1 frozen=yes mode=default trim=yes lah=n/a lah_token="n/a" recovering=no
 [spicy-verbose] - parsing production: Unit: test__A -> a b c d e
 [spicy-verbose]   - state: type=test::A input="A" stream=0xXXXXXXXX offsets=0/0/0/1 chunks=1 frozen=yes mode=default trim=yes lah=n/a lah_token="n/a" recovering=no
@@ -205,7 +205,7 @@ Error: [$a=b"A", $b=(not set), $c=(not set), $d=(not set), $e=(not set)]
 [spicy-verbose]     failed to parse, will try to synchronize at 'd'
 [spicy-verbose]     - state: type=test::A input="" stream=0xXXXXXXXX offsets=1/0/1/1 chunks=0 frozen=yes mode=default trim=yes lah=n/a lah_token="n/a" recovering=yes
 Error: [$a=b"A", $b=(not set), $c=(not set), $d=(not set), $e=(not set)]
-[error] terminating with uncaught exception of type spicy::rt::ParseError: failed to synchronize: expecting 'B' (<...>/synchronize.spicy:40:8-40:11)
+[error] terminating with uncaught exception of type spicy::rt::ParseError: failed to synchronize: expecting 'B' (<...>/synchronize.spicy:39:8-39:11)
 [spicy-verbose] - state: type=test::A input="1" stream=0xXXXXXXXX offsets=0/0/0/1 chunks=1 frozen=no mode=default trim=yes lah=n/a lah_token="n/a" recovering=no
 [spicy-verbose] - parsing production: Unit: test__A -> a b c d e
 [spicy-verbose]   - state: type=test::A input="1" stream=0xXXXXXXXX offsets=0/0/0/1 chunks=1 frozen=no mode=default trim=yes lah=n/a lah_token="n/a" recovering=no
@@ -220,7 +220,7 @@ Error: [$a=b"A", $b=(not set), $c=(not set), $d=(not set), $e=(not set)]
 [spicy-verbose]     resuming after insufficient input, now have 1 for stream 0xXXXXXXXX
 [spicy-verbose]     - trimming input
 Error: [$a=(not set), $b=(not set), $c=(not set), $d=(not set), $e=(not set)]
-[error] terminating with uncaught exception of type spicy::rt::ParseError: failed to synchronize: failed to match regular expression (<...>/synchronize.spicy:39:8-39:10)
+[error] terminating with uncaught exception of type spicy::rt::ParseError: failed to synchronize: failed to match regular expression (<...>/synchronize.spicy:38:8-38:10)
 [spicy-verbose] - state: type=test::E input="B" stream=0xXXXXXXXX offsets=0/0/0/1 chunks=1 frozen=no mode=default trim=yes lah=n/a lah_token="n/a" recovering=no
 [spicy-verbose] - parsing production: Unit: test__E -> _anon Resolved_6 _anon_2
 [spicy-verbose]   - state: type=test::E input="B" stream=0xXXXXXXXX offsets=0/0/0/1 chunks=1 frozen=no mode=default trim=yes lah=n/a lah_token="n/a" recovering=no