compiler: keep the C++ throw site out of user diagnostics (#557)

README.md

@@ -157,6 +157,8 @@ the cache). Check-only modes: `--syntax-only` stops after parsing,
 `--semantic-only` after type-checking — neither writes output — and
 `--transient-cc` keeps only the linked `.so`, removing the generated C++
 intermediates.
+Diagnostics name only your code's position; `--compiler-locations` appends
+the compiler source line that raised each one, for filing a compiler bug.
 
 Exercise the Orlyscript test suite against compiled `.orly` programs:
 

changelog.d/557-clean-diagnostics.md

@@ -0,0 +1 @@
+- **Fixed**: compiler diagnostics no longer show users a path into the compiler's own C++ source. `orlyc` printed `2:28-2:47 [orly/type/add_visitor.h, 50]This expression is invalid.`: an internal path that reads like a crash, glued to the message with no space. The throw site is now a separate field on `TSourceError` instead of being spliced into `what()`. `orlyc --compiler-locations` shows it, and WS `source_error` replies carry it in a separate `compiler_loc` key, leaving `result` clean. Internal compiler errors keep it in the message, because there it is what the report needs. A mutation whose target is not a stored value now says so. A bare address (`<['counter']> += n`, the form the quickstart led a newcomer to write) gets a message that shows the working form, `*<['counter']>::(int) += n`, and a plain value, which used to **segfault orlyc** with no output at all, gets a clean error. Both are pinned by new diagnostic lang_tests (#557).

docs/PROTOCOL.md

@@ -31,6 +31,11 @@ sent as one WebSocket text message. The server replies with one JSON message:
 - `result` is present on statements that produce a value (see each statement).
   Its shape depends on the statement; for `try` it is the JSON marshaling of the
   method's return value (see "JSON marshaling" below).
+- A statement that fails to compile replies `"status": "source_error"`, with
+  the message in `result` and its position in `pos` (`line:col-line:col`).
+  `compiler_loc` names the compiler source line that raised it -- useful in a
+  compiler bug report, meaningless to the statement's author, so clients should
+  not show it by default (#557).
 
 ## Statements
 

orly/error.h

@@ -21,25 +21,39 @@
 #include <cassert>
 
 #include <base/as_str.h>
+#include <base/code_location.h>
 #include <base/thrower.h>
 #include <orly/pos_range.h>
 
 namespace Orly {
 
+  /* what() is the message for the person who wrote the orlyscript. The
+     compiler source line that threw it is kept apart, in GetCodeLocation(),
+     because to that person it reads like a crash rather than a diagnosis of
+     their code (#557). Front ends show it only on request. */
   class TSourceError : public std::runtime_error {
     public:
 
+    const Base::TCodeLocation &GetCodeLocation() const {
+      return CodeLocation;
+    }
+
     const TPosRange &GetPosRange() const {
       return PosRange;
     }
 
     protected:
 
-    TSourceError(const TPosRange &pos_range, const char *msg)
-        : std::runtime_error(msg), PosRange(pos_range) {}
+    TSourceError(
+        const Base::TCodeLocation &code_location,
+        const TPosRange &pos_range,
+        const char *msg)
+          : std::runtime_error(msg), CodeLocation(code_location), PosRange(pos_range) {}
 
     private:
 
+    const Base::TCodeLocation CodeLocation;
+
     const TPosRange PosRange;
 
   };  // TSourceError
@@ -51,18 +65,20 @@ namespace Orly {
         const Base::TCodeLocation &code_location,
         const TPosRange &pos_range,
         const char *message = "This feature is not yet implemented")
-          : TSourceError(pos_range, Base::AsStr(code_location, message).c_str()) {}
+          : TSourceError(code_location, pos_range, message) {}
 
   };  // TNotImplementedError
 
+  /* A compiler bug, not a user error, so the message keeps the compiler
+     source location: "here is where" is what the report needs. */
   class TImpossibleError : public TSourceError {
     public:
 
     TImpossibleError(
         const Base::TCodeLocation &code_location,
         const TPosRange &pos_range,
         const char *message = "Internal Compiler Error: We shouldn't have reached this line of code in the compiler.")
-          : TSourceError(pos_range, Base::AsStr(code_location, message).c_str()) {}
+          : TSourceError(code_location, pos_range, Base::AsStr(code_location, ' ', message).c_str()) {}
 
   };  // TImpossibleError
 
@@ -73,7 +89,7 @@ namespace Orly {
         const Base::TCodeLocation &code_location,
         const TPosRange &pos_range,
         const char *message)
-          : TSourceError(pos_range, Base::AsStr(code_location, message).c_str()) {}
+          : TSourceError(code_location, pos_range, message) {}
 
   };  // TCompileError
 
@@ -89,7 +105,7 @@ namespace Orly {
         const Base::TCodeLocation &code_location,
         const TPosRange &pos_range,
         const char *message = DefaultMessage)
-          : TSourceError(pos_range, Base::AsStr(code_location, message).c_str()) {}
+          : TSourceError(code_location, pos_range, message) {}
 
   };  // TExprError
 

orly/orlyc.cc

@@ -52,6 +52,7 @@ class TCompilerConfig : public Base::TCmd {
         MachineForm(false),
         OutputDir(Util::GetCwd()),
         SemanticOnly(false),
+        ShowCompilerLocations(false),
         SkipTests(false),
         SyntaxOnly(false),
         TransientCc(false),
@@ -69,6 +70,8 @@ class TCompilerConfig : public Base::TCmd {
       Param(&TCompilerConfig::MachineForm, "machine_form", Optional, "machine-form\0m\0", "Print out machine readable progress.");
       Param(&TCompilerConfig::OutputDir, "output_directory", Optional, "output\0o\0", "The directory to write output to.");
       Param(&TCompilerConfig::SemanticOnly, "semantic_only", Optional, "semantic-only\0", "Don't produce output, just syntactically and semantically validate the program.");
+      Param(&TCompilerConfig::ShowCompilerLocations, "show_compiler_locations", Optional, "compiler-locations\0",
+          "Append the compiler source line that raised each diagnostic. For reporting a compiler bug; it says nothing about your code.");
       Param(&TCompilerConfig::SkipTests, "skip_tests", Optional, "skip-tests\0", "Don't run tests after compiling.");
       Param(&TCompilerConfig::SyntaxOnly, "syntax_only", Optional, "syntax-only\0", "Don't produce output or type-check, just syntactically validate the program.");
       Param(&TCompilerConfig::TransientCc, "transient_cc", Optional, "transient-cc\0", "Remove the generated C++ intermediates after a successful compile, leaving only the linked package.");
@@ -98,6 +101,7 @@ class TCompilerConfig : public Base::TCmd {
   bool MachineForm;
   std::string OutputDir;
   bool SemanticOnly;
+  bool ShowCompilerLocations;
   std::string Source;
   bool SkipTests;
   bool SyntaxOnly;
@@ -285,7 +289,12 @@ int CompileCode(const TCompilerConfig &cmd) {
       cerr << "compile failure: " << ex.what() << endl;
     }
   } catch (const TSourceError &src_error) {
-    cerr << src_error.GetPosRange() << ' ' << src_error.what() << endl;
+    cerr << src_error.GetPosRange() << ' ' << src_error.what();
+    /* An impossible error already names its location in the message. */
+    if (cmd.ShowCompilerLocations && !dynamic_cast<const TImpossibleError *>(&src_error)) {
+      cerr << ' ' << src_error.GetCodeLocation();
+    }
+    cerr << endl;
   } catch (const exception &ex) {
     cerr << "error: " << ex.what() << endl;
   }

orly/server/ws.cc

@@ -541,6 +541,9 @@ class TWsImpl final
       } catch (const TSourceError &src_error) {
         reply["result"] = src_error.what();
         reply["pos"] = AsStr(src_error.GetPosRange());
+        /* Kept out of "result" so a client shows a clean message; the
+           compiler line is there for whoever is reporting a bug (#557). */
+        reply["compiler_loc"] = AsStr(src_error.GetCodeLocation());
         reply["status"] = "source_error";
       } catch (const exception &ex) {
         reply["result"] = ex.what();

orly/symbol/stmt/mutate.cc

@@ -23,6 +23,7 @@
 #include <base/as_str.h>
 #include <orly/error.h>
 #include <orly/type/add_visitor.h>
+#include <orly/type/addr.h>
 #include <orly/type/comp_visitor.h>
 #include <orly/type/div_visitor.h>
 #include <orly/type/equal_visitor.h>
@@ -32,6 +33,8 @@
 #include <orly/type/min_max_visitor.h>
 #include <orly/type/mod_visitor.h>
 #include <orly/type/mult_visitor.h>
+#include <orly/type/mutable.h>
+#include <orly/type/seq.h>
 #include <orly/type/set_ops_visitor.h>
 #include <orly/type/sub_visitor.h>
 #include <orly/type/unwrap.h>
@@ -120,6 +123,20 @@ const TMutator &TMutate::GetMutator() const {
 }
 
 void TMutate::TypeCheck() const {
+  /* A mutation writes to the database, so its target must be a stored value:
+     `*<[key]>::(type)`, or something bound to one. Code gen assumes this and
+     dereferences the target's mutable type, so a plain value here crashed
+     orlyc, and a bare address fell through to the operator's visitor as
+     "This expression is invalid." (#557). Sequences are left for the visitor,
+     which has its own diagnostic for them. */
+  const Type::TType lhs_type = GetLhs()->GetExpr()->GetType();
+  if (!lhs_type.Is<Type::TSeq>() && !Type::UnwrapOptional(lhs_type).Is<Type::TMutable>()) {
+    throw TExprError(HERE, GetPosRange(), lhs_type.Is<Type::TAddr>()
+        ? "The left side of a mutation must be a stored value, but this is an address. "
+          "Name the value stored at it with `*` and its type, as in `*<['counter']>::(int) += n`."
+        : "The left side of a mutation must be a stored value, such as `*<['counter']>::(int)`, "
+          "but this is a plain value.");
+  }
   Type::TType dummy;
   /* NOTE: It would be nice to use a templatized helper function for this but that would require
            the helper function and the TMutateTypeVisitor to be in the header.

tests/lang_tests/.xfail

@@ -18,3 +18,5 @@ general/unsorted/sequence_in_effecting.orly #75
 # DESIGN -- their .state baselines pin the exact compiler error messages.
 general/diag_mutable_type.orly #314
 general/diag_assign_type_mismatch.orly #314
+general/diag_mutate_address.orly #557
+general/diag_mutate_plain_value.orly #557

tests/lang_tests/general/.diag_assign_type_mismatch.orly.test.state

@@ -7,7 +7,7 @@
     ],
     [
       "TypeCheck",
-      "23:40-23:52 [orly/symbol/stmt/mutate.cc]cannot assign a value of type std::string to a mutable holding int64_t"
+      "23:40-23:52 cannot assign a value of type std::string to a mutable holding int64_t"
     ]
   ]
 ]

tests/lang_tests/general/.diag_mutate_address.orly.test.state

@@ -0,0 +1,13 @@
+[
+  1,
+  [
+    [],
+    [
+      "Synth + Symbols"
+    ],
+    [
+      "TypeCheck",
+      "24:28-24:47 The left side of a mutation must be a stored value, but this is an address. Name the value stored at it with `*` and its type, as in `*<['counter']>::(int) += n`."
+    ]
+  ]
+]

tests/lang_tests/general/.diag_mutate_plain_value.orly.test.state

@@ -0,0 +1,13 @@
+[
+  1,
+  [
+    [],
+    [
+      "Synth + Symbols"
+    ],
+    [
+      "TypeCheck",
+      "22:28-22:35 The left side of a mutation must be a stored value, such as `*<['counter']>::(int)`, but this is a plain value."
+    ]
+  ]
+]

tests/lang_tests/general/diag_mutate_address.orly

@@ -0,0 +1,26 @@
+/* <orly/lang_tests/general/diag_mutate_address.orly>
+
+   Diagnostics pin (issue #557): a mutation whose target is a bare address
+   -- `<['counter']>` with no `*` or `::(type)` -- must say it needs the
+   stored value and show the form that works, not the generic "This
+   expression is invalid." This is the snippet the published quickstart led
+   a newcomer to write. EXPECTED to fail compilation (see .xfail); the
+   .state baseline pins the exact message.
+
+   Copyright 2010-2026 Atomic Kismet Company
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License. */
+
+bump = ((true) effecting { <['counter']> += n; } ) where {
+  n = given::(int);
+};

tests/lang_tests/general/diag_mutate_plain_value.orly

@@ -0,0 +1,25 @@
+/* <orly/lang_tests/general/diag_mutate_plain_value.orly>
+
+   Diagnostics pin (issue #557): a mutation whose target is a plain value,
+   not a stored one, used to crash orlyc with a segfault and no message.
+   It must report a clean error. EXPECTED to fail compilation (see
+   .xfail); the .state baseline pins the exact message.
+
+   Copyright 2010-2026 Atomic Kismet Company
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License. */
+
+bump = ((true) effecting { x += n; } ) where {
+  n = given::(int);
+  x = 1;
+};

tests/lang_tests/general/unsorted/.sequence_in_effecting.orly.test.state

@@ -10,7 +10,7 @@
     ],
     [
       "Code Gen",
-      "2:7-2:12 [orly/code_gen/builder.cc]Sequences in effecting blocks"
+      "2:7-2:12 Sequences in effecting blocks"
     ]
   ]
 ]