Skip to content

Compiler diagnostics leak internal C++ source paths to users ([orly/type/add_visitor.h, 50]) #557

Description

@ohohoreilly

Split out of #548, whose fourth scope item this is. That issue found it by running the published quickstart cold; this is the part of it that needs a compiler change rather than a docs change.

What a user sees

Reproduced on master today, with a release orlyc:

$ cat bad.orly
package #1;
bump = ((true) effecting { <['counter']> += n; } ) where {
  n = given::(int);
};

$ orlyc -o . bad.orly
2:28-2:47 [orly/type/add_visitor.h, 50]This expression is invalid.

Three separate problems in that one line:

  1. [orly/type/add_visitor.h, 50] is a path into this compiler's own C++ source. It is meaningless to anyone writing orlyscript, and it reads like an internal crash rather than a diagnosis of their code.
  2. Nothing says what is invalid or how to fix it. The correct form here needs a leading * and a ::(type) annotation — *<['counter']>::(int) += n — and nothing on the path says so.
  3. There is no space after the ], so the C++ location and the message run together into 50]This.

The user's next move in #548 was to read the 0 from a follow-up read as "the write landed and the data is wrong". So the cost here is not cosmetic.

Where it comes from

orly/error.h folds the C++ TCodeLocation into the exception's message at construction:

TExprError(const Base::TCodeLocation &code_location,
           const TPosRange &pos_range,
           const char *message = DefaultMessage)
    : TSourceError(pos_range, Base::AsStr(code_location, message).c_str()) {}

TCodeLocation::Write emits '[' << GetFile() << ", " << LineNumber << ']' with no trailing separator, which is where the missing space comes from. Because the location is baked into what(), every consumer inherits it — there is no way to present the message without it.

Throw sites carrying a location, by class:

class sites
TExprError 1890
TImpossibleError 147
TNotImplementedError 13
TCompileError 8

Both user-facing surfaces print what() verbatim:

  • orly/orlyc.cc:288 — cerr << src_error.GetPosRange() << ' ' << src_error.what()
  • orly/server/ws.cc:542 — reply["result"] = src_error.what(), which is the path the REPL and the MCP server see

What the fix probably is

Not deleting the location — it is genuinely useful when triaging a compiler bug, and for TImpossibleError ("Internal Compiler Error") it is the point. The problem is that it is unconditionally part of the user-visible string.

Shape worth considering:

  • Keep TCodeLocation as a field on TSourceError, not spliced into what().
  • orlyc prints it only under a verbosity flag. Note --debug/-d is already taken and means "compile the package in debug mode", so this wants its own flag.
  • ws.cc puts it in a separate JSON key rather than in result, so the REPL can show a clean message and a client that wants the detail can still ask.
  • Keep it unconditional for TImpossibleError, where "this is a compiler bug, here is where" is the right message.

That is ~2000 call sites but a mechanical change: the constructors already take the location as a distinct argument, so only error.h and the two print sites need real thought.

Related

#314 (closed) improved the wording of these diagnostics — it is where TExprError::DefaultMessage and the "recognize and upgrade the generic diagnostic" note came from. This issue is about the envelope rather than the wording, and item 2 above is the part that overlaps: This expression is invalid. is still the generic fallback on the most common path a newcomer hits.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions