From b7ce672a4e835c6be0c77beed940b817327aedf5 Mon Sep 17 00:00:00 2001 From: Patrick O'Reilly Date: Thu, 1 Oct 2026 19:23:17 -0700 Subject: [PATCH] compiler: keep the C++ throw site out of user diagnostics (#557) orlyc printed 2:28-2:47 [orly/type/add_visitor.h, 50]This expression is invalid. for a newcomer's `<['counter']> += n`. That is a path into the compiler's own source, glued to the message with no space, and it reads like a crash. TSourceError now keeps the TCodeLocation as a field instead of splicing it into what(). orlyc shows it under --compiler-locations; WS source_error replies carry it as a separate compiler_loc key. TImpossibleError keeps it in the message, since for an internal compiler error the location is the report. TMutate::TypeCheck now requires the target to be a stored value. A bare address used to fall through to the operator visitor's generic message; it now names the problem and shows `*<['counter']>::(int) += n`. A plain value (`x += n` with `x = 1`) used to segfault orlyc in TMutation::GetValType, which dereferences the target's mutable type unchecked; it now gets a clean error. Both are pinned by new diagnostic lang_tests, and the two baselines that quoted a compiler path are updated. --- README.md | 2 ++ changelog.d/557-clean-diagnostics.md | 1 + docs/PROTOCOL.md | 5 ++++ orly/error.h | 28 +++++++++++++++---- orly/orlyc.cc | 11 +++++++- orly/server/ws.cc | 3 ++ orly/symbol/stmt/mutate.cc | 17 +++++++++++ tests/lang_tests/.xfail | 2 ++ ....diag_assign_type_mismatch.orly.test.state | 2 +- .../.diag_mutate_address.orly.test.state | 13 +++++++++ .../.diag_mutate_plain_value.orly.test.state | 13 +++++++++ .../general/diag_mutate_address.orly | 26 +++++++++++++++++ .../general/diag_mutate_plain_value.orly | 25 +++++++++++++++++ .../.sequence_in_effecting.orly.test.state | 2 +- 14 files changed, 141 insertions(+), 9 deletions(-) create mode 100644 changelog.d/557-clean-diagnostics.md create mode 100644 tests/lang_tests/general/.diag_mutate_address.orly.test.state create mode 100644 tests/lang_tests/general/.diag_mutate_plain_value.orly.test.state create mode 100644 tests/lang_tests/general/diag_mutate_address.orly create mode 100644 tests/lang_tests/general/diag_mutate_plain_value.orly diff --git a/README.md b/README.md index 9626c7ee..341b23e4 100644 --- a/README.md +++ b/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: diff --git a/changelog.d/557-clean-diagnostics.md b/changelog.d/557-clean-diagnostics.md new file mode 100644 index 00000000..e9ad3089 --- /dev/null +++ b/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). diff --git a/docs/PROTOCOL.md b/docs/PROTOCOL.md index 088f55c5..96af45c0 100644 --- a/docs/PROTOCOL.md +++ b/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 diff --git a/orly/error.h b/orly/error.h index 1c3facf7..ca5b014e 100644 --- a/orly/error.h +++ b/orly/error.h @@ -21,25 +21,39 @@ #include #include +#include #include #include 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,10 +65,12 @@ 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: @@ -62,7 +78,7 @@ namespace Orly { 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 diff --git a/orly/orlyc.cc b/orly/orlyc.cc index 378a7678..71eda632 100644 --- a/orly/orlyc.cc +++ b/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(&src_error)) { + cerr << ' ' << src_error.GetCodeLocation(); + } + cerr << endl; } catch (const exception &ex) { cerr << "error: " << ex.what() << endl; } diff --git a/orly/server/ws.cc b/orly/server/ws.cc index f8fb9800..4799be26 100644 --- a/orly/server/ws.cc +++ b/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(); diff --git a/orly/symbol/stmt/mutate.cc b/orly/symbol/stmt/mutate.cc index 77e116a7..82a80f7e 100644 --- a/orly/symbol/stmt/mutate.cc +++ b/orly/symbol/stmt/mutate.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -32,6 +33,8 @@ #include #include #include +#include +#include #include #include #include @@ -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::UnwrapOptional(lhs_type).Is()) { + throw TExprError(HERE, GetPosRange(), lhs_type.Is() + ? "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. diff --git a/tests/lang_tests/.xfail b/tests/lang_tests/.xfail index afb4bd4e..265f54d6 100644 --- a/tests/lang_tests/.xfail +++ b/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 diff --git a/tests/lang_tests/general/.diag_assign_type_mismatch.orly.test.state b/tests/lang_tests/general/.diag_assign_type_mismatch.orly.test.state index 34334ea4..4704de6d 100644 --- a/tests/lang_tests/general/.diag_assign_type_mismatch.orly.test.state +++ b/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" ] ] ] diff --git a/tests/lang_tests/general/.diag_mutate_address.orly.test.state b/tests/lang_tests/general/.diag_mutate_address.orly.test.state new file mode 100644 index 00000000..1c6d1dff --- /dev/null +++ b/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`." + ] + ] +] diff --git a/tests/lang_tests/general/.diag_mutate_plain_value.orly.test.state b/tests/lang_tests/general/.diag_mutate_plain_value.orly.test.state new file mode 100644 index 00000000..d8f40d15 --- /dev/null +++ b/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." + ] + ] +] diff --git a/tests/lang_tests/general/diag_mutate_address.orly b/tests/lang_tests/general/diag_mutate_address.orly new file mode 100644 index 00000000..89f01691 --- /dev/null +++ b/tests/lang_tests/general/diag_mutate_address.orly @@ -0,0 +1,26 @@ +/* + + 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); +}; diff --git a/tests/lang_tests/general/diag_mutate_plain_value.orly b/tests/lang_tests/general/diag_mutate_plain_value.orly new file mode 100644 index 00000000..5430fe29 --- /dev/null +++ b/tests/lang_tests/general/diag_mutate_plain_value.orly @@ -0,0 +1,25 @@ +/* + + 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; +}; diff --git a/tests/lang_tests/general/unsorted/.sequence_in_effecting.orly.test.state b/tests/lang_tests/general/unsorted/.sequence_in_effecting.orly.test.state index 4b867d4c..631deaeb 100644 --- a/tests/lang_tests/general/unsorted/.sequence_in_effecting.orly.test.state +++ b/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" ] ] ]