fix(generator): no silent drops — unread STRUCTURE in a record is a build error (#131)

#127 made an unknown C++ TYPE a build error instead of a silent `any`. This is
the same hole one level down, in the record scanner: a line inside a `struct`
body that the scanner could not read as a field was `continue`d, and the struct
was published anyway — MINUS that field.

A field list is not a detail of a record, it IS the record: the promise every
other language binds to. A contract with two fields is exactly as well-formed as
one with three, so nothing downstream could tell. Measured on the generator built
from master:

  struct SplitRecord {           ->  type SplitRecord {     exit 0, no diagnostic
      std::string id;                  id: tstr
      std::vector<std::string>         n: uint
          tags;                      }
      uint64_t n;
  };                                   `tags` is simply GONE

  struct Outer {                 ->  type Outer { a: int }  exit 0
      struct Inner {                   the record is made of the INNER type's
          int64_t a;                   field and has none of its own
      };
      Inner inner;
      std::string label;
  };

  struct Pair {                  ->  method echoPair(v: Pair) with NO `type Pair`
      std::string first; std::string second;    emitted — a contract naming a
  };                                            type it never declares

  struct Defaulted {             ->  the brace-initialised field is dropped
      std::string id{"none"};        (only the `= v` form was recognised)
      int64_t n = 0;
  };

  struct Allman                  ->  not a record AT ALL, so every mention of it
  {                                  falls to the `any` fallback — since #127 an
      std::string id;                error whose hint says "declare a struct",
  };                                 which is the thing the author declared

The scanner now reads a body as DECLARATIONS rather than lines. Physical lines
are joined until the declaration is whole — a `;` at the struct's own brace
depth, or a `}` there, which is how a member function defined inline ends —
exactly as the caller already joins a method signature until its parentheses
balance. Where a brace sits, and where a line wraps, cannot decide what a header
means. Allman and base-clause openings are recognised for the same reason.

Whatever is left after that, and after the constructs that definitively are NOT
fields (member functions, `using`/`typedef`/`friend`/`static`, access
specifiers), is reported instead of skipped, naming the struct, the declaration
and the fix. Same withdrawal discipline as #127: the diagnostic is keyed on the
struct and tested against the API-REFERENCED set, so a helper struct the module
never publishes may be as unreadable as it likes — every real module carries one
(openmetrics' `ModuleSource`, the package manager's `PendingAction`).

Comments come off with a literal-aware strip. The old bare `indexOf("//")`
truncated `std::string url = "http://x";` inside the literal, which merely lost
the field before and would now reject valid code.

Blast radius, measured: all 27 universal-interface impl headers in the workspace
emit BYTE-IDENTICAL .lidl and stderr, with identical exit codes. All 6 structs
those headers declare are K&R with no unparsed body line, so nothing in tree
changes. logos-qt-generator, which compiles this same file, builds; and
test_fullapi_ext_cpp — the records-heavy module — builds end to end.

14 new tests. 10 of them fail on the unpatched parser; the rest are the "must
still work" controls, including one for an inline member-function body that an
earlier cut of this change would have broken.
This commit is contained in:
Dario Lipicar
2026-08-03 09:44:03 -03:00
committed by GitHub
parent b4c2e5bb2d
commit d972fe207a
2 changed files with 778 additions and 38 deletions
+389 -38
View File
@@ -77,6 +77,33 @@ struct UnsupportedSpelling {
};
static QList<UnsupportedSpelling> g_unsupported;
// ---------------------------------------------------------------------------
// STRUCTURE the record scanner could not read.
//
// #127 closed the hole where an unknown TYPE was admitted as `any`. This is the
// same hole one level down: a line inside a `struct` body that the scanner
// could not read as a field was `continue`d, and the struct was published
// anyway — MINUS that field. A field list is not a detail of a record, it IS
// the record: the promise every other language binds to. Publishing a shorter
// one silently ships a contract that disagrees with the header, and nothing
// downstream can tell, because a contract with three fields and a contract with
// two are both perfectly well-formed.
//
// Same discipline as g_unsupported: collected here, withdrawn when the struct
// turns out never to reach the contract (an internal helper promises nothing),
// and a hard parse error otherwise.
struct UnreadableDecl {
QString record; // the struct it was found in — the withdrawal key
QString text; // the declaration as written, comments removed
QString hint; // what to write instead
};
static QList<UnreadableDecl> g_unreadable;
// Structs that were opened but published no field at all. Referenced by the API
// they are still a defect — the emitted contract names a `type` it never
// declares — but no single line is at fault, so they are reported separately.
static QStringList g_emptyRecords;
// Drop the qualifiers that are about how a value is PASSED rather than what it
// is: cppTypeToLidl normalizes with this, and the diagnostics compare against it
// so `const nlohmann::json&` and `nlohmann::json` are recognised as the same
@@ -395,6 +422,148 @@ static TypeExpr cppTypeToLidl(const QString& raw, const QString& context = QStri
return { TypeExpr::Primitive, "any", {} };
}
// Remove comments from the already-merged logical lines, honouring string and
// character literals and carrying block-comment state across lines.
//
// The record scanner used to strip with a bare `indexOf("//")`. That is right
// for `std::string name; // what it is` and WRONG for
// `std::string url = "http://x";`, which it truncates inside the literal — the
// declaration then no longer ends in ';', and the field vanished. Harmless
// enough while an unreadable line was merely skipped; now that it is a build
// error, the same truncation would reject valid code, so the strip has to know
// what a literal is. Block comments are removed for the same reason: a field
// annotated `std::string id; /* note */` did not end in ';' either.
static QStringList stripCommentsFrom(const QStringList& lines)
{
QStringList out;
bool inBlock = false;
for (const QString& line : lines) {
QString kept;
bool inStr = false;
bool inChr = false;
for (int i = 0; i < line.size(); ++i) {
const QChar c = line[i];
const QChar n = (i + 1 < line.size()) ? line[i + 1] : QChar();
if (inBlock) {
if (c == '*' && n == '/') { inBlock = false; ++i; }
continue;
}
if (inStr || inChr) {
kept += c;
if (c == '\\' && i + 1 < line.size()) { kept += n; ++i; }
else if (inStr && c == '"') inStr = false;
else if (inChr && c == '\'') inChr = false;
continue;
}
if (c == '/' && n == '*') { inBlock = true; ++i; continue; }
if (c == '/' && n == '/') break;
if (c == '"') inStr = true;
else if (c == '\'') inChr = true;
kept += c;
}
out.append(kept.trimmed());
}
return out;
}
// A `struct` DEFINITION opening, in the forms C++ is actually written in:
//
// struct Name { K&R — the only form the scanner used to accept
//
// struct Name Allman — the opening brace on the next line
// {
//
// plus the base-clause spelling of either. `struct Name;` is a forward
// declaration and stays out: there is no body to read.
//
// Allman was not a harmless stylistic omission. The struct was not a record AT
// ALL, so every mention of it in a signature fell through to the `any` fallback
// — which since #127 is a hard error whose hint tells the author to "declare a
// struct", the very thing they did declare. Where a brace sits cannot decide
// what a header means.
struct StructOpen {
QString name;
QString text; // the opening as written, for diagnostics
bool hasBase = false;
bool bodyOnOpeningLine = false; // `struct P { int64_t a; };` all on one line
int bodyStart = -1; // index of the first line INSIDE the body
};
static bool matchStructOpen(const QStringList& code, int i, StructOpen& out)
{
// `[^{;]` in the base clause keeps `struct Name;` and the brace itself out
// of the capture.
static const QRegularExpression kandrRe(
"^struct\\s+(\\w+)\\s*(:[^{;]*)?\\{(.*)$");
static const QRegularExpression headRe("^struct\\s+(\\w+)\\s*(:[^{;]*)?$");
const QRegularExpressionMatch km = kandrRe.match(code.at(i));
if (km.hasMatch()) {
out.name = km.captured(1);
out.text = code.at(i);
out.hasBase = !km.captured(2).trimmed().isEmpty();
out.bodyOnOpeningLine = !km.captured(3).trimmed().isEmpty();
out.bodyStart = i + 1;
return true;
}
const QRegularExpressionMatch hm = headRe.match(code.at(i));
if (!hm.hasMatch()) return false;
// Allman: the next line carrying any code at all has to open the body.
// Anything else and this was not a definition (a `struct Name` mentioned in
// some other construct), so it is left alone exactly as before.
for (int j = i + 1; j < code.size(); ++j) {
if (code.at(j).isEmpty()) continue;
if (!code.at(j).startsWith('{')) return false;
out.name = hm.captured(1);
out.text = code.at(i);
out.hasBase = !hm.captured(2).trimmed().isEmpty();
out.bodyOnOpeningLine = !code.at(j).mid(1).trimmed().isEmpty();
out.bodyStart = j + 1;
return true;
}
return false;
}
static void reportUnreadable(const QString& record, const QString& text,
const QString& hint)
{
g_unreadable.append({ record, text.trimmed(), hint });
}
// Net brace depth a line adds, ignoring braces inside string and character
// literals: `std::string s = "{";` is balanced code even though it is not
// balanced text, and a body scan that believed the text would never find the
// end of the struct.
static int braceDelta(const QString& line)
{
int delta = 0;
bool inStr = false;
bool inChr = false;
for (int i = 0; i < line.size(); ++i) {
const QChar c = line[i];
if (inStr || inChr) {
if (c == '\\') { ++i; continue; }
if (inStr && c == '"') inStr = false;
else if (inChr && c == '\'') inChr = false;
continue;
}
if (c == '"') inStr = true;
else if (c == '\'') inChr = true;
else if (c == '{') ++delta;
else if (c == '}') --delta;
}
return delta;
}
// What a contract field looks like. Named once because three diagnostics quote
// it, and a hint that describes a different grammar than the one enforced is
// worse than no hint.
static const QString kFieldFormHint = QStringLiteral(
"A contract field is ONE declaration per line, ending in `;` — `Type name;`, "
"optionally with a default (`= v` or `{v}`). A declaration wrapped across "
"several lines is joined for you; two declarations sharing one line are not.");
// Find `struct Name { Type field; ... };` blocks and turn them into `type`
// declarations.
//
@@ -403,58 +572,171 @@ static TypeExpr cppTypeToLidl(const QString& raw, const QString& context = QStri
// hand-written .lidl. Worse, a method mentioning the struct still parsed: its
// type fell through to the opaque `any`, so the contract silently disagreed
// with the header.
//
// Two things beyond that are new here, and they are the same idea from opposite
// ends. The body is read as DECLARATIONS rather than lines — physical lines are
// joined until the `;`, exactly as the caller already joins a method signature
// until its parentheses balance — so a wrapped field is the field the author
// wrote rather than nothing at all. And whatever is left over after that, and
// after the constructs that definitively are NOT fields, is reported instead of
// skipped: the scanner may not quietly decide that a line it cannot read was
// not worth publishing.
static std::vector<TypeDecl> scanForRecords(const QStringList& lines)
{
static QRegularExpression openRe("^struct\\s+(\\w+)\\s*\\{\\s*$");
static QRegularExpression fieldRe("^([\\w:<>,\\s\\*]+?)\\s+(\\w+)\\s*(=[^;]*)?;$");
// `\{[^;]*\}` accepts a brace initialiser beside the `=` form: a field
// written `std::string id{"none"};` is a field, and dropping it published a
// record whose defaults decided which members a consumer could see.
static QRegularExpression fieldRe(
"^([\\w:<>,\\s\\*]+?)\\s+(\\w+)\\s*(=[^;]*|\\{[^;]*\\})?;$");
static QRegularExpression accessRe("^(public|private|protected)\\s*:");
// Declarations that are legitimately not fields, and are skipped by rule
// rather than by failing to match. A `static` data member is not part of
// the object's value and never reaches the wire.
static QRegularExpression notAFieldRe(
"^(using|typedef|friend|template|static_assert|static|constexpr|inline)\\b");
static QRegularExpression nestedTypeRe("^(struct|class|union|enum)\\b");
// Comments come off ONCE, up front, so every rule below sees code.
const QStringList code = stripCommentsFrom(lines);
// TWO passes. A record field may name another record (`Blob inner;` inside
// Wrapper), and cppTypeToLidl only answers Named() for a name already in
// g_recordNames — so every struct name has to be registered before any
// field is typed. One pass silently typed such a field as `any`, and the
// generated codec then tried to encode a Blob as a LogosMap.
for (int i = 0; i < lines.size(); ++i) {
QRegularExpressionMatch om = openRe.match(lines.at(i).trimmed());
if (om.hasMatch())
g_recordNames.insert(om.captured(1));
for (int i = 0; i < code.size(); ++i) {
StructOpen so;
if (matchStructOpen(code, i, so))
g_recordNames.insert(so.name);
}
std::vector<TypeDecl> out;
for (int i = 0; i < lines.size(); ++i) {
const QString line = lines.at(i).trimmed();
QRegularExpressionMatch om = openRe.match(line);
if (!om.hasMatch()) continue;
for (int i = 0; i < code.size(); ++i) {
StructOpen so;
if (!matchStructOpen(code, i, so)) continue;
TypeDecl td;
td.name = om.captured(1).toStdString();
td.name = so.name.toStdString();
// Withdraw this struct's diagnostics if it turns out to declare no
// fields at all — nothing is published, so nothing is misreported.
const int diagMark = g_unsupported.size();
for (int j = i + 1; j < lines.size(); ++j) {
const QString body = lines.at(j).trimmed();
if (body.startsWith("};")) break;
if (body.isEmpty() || body.startsWith("//")) continue;
// Strip a trailing line comment before matching: a field written
// `std::string name; // what it is` does not end in ';' and was
// silently DROPPED, publishing a record with a partial field list —
// the worst kind of wrong, because it looks like a contract.
QString field = body;
const int comment = field.indexOf("//");
if (comment >= 0) field = field.left(comment).trimmed();
if (field.isEmpty()) continue;
QRegularExpressionMatch fm = fieldRe.match(field);
if (!fm.hasMatch()) continue;
FieldDecl fd;
fd.name = fm.captured(2).toStdString();
const QString spelling = fm.captured(1).trimmed();
fd.type = cppTypeToLidl(
spelling,
QString("type '%1': field '%2'").arg(om.captured(1), fm.captured(2)),
spelling, om.captured(1), /*nameEmitted=*/true);
td.fields.push_back(fd);
if (so.hasBase) {
reportUnreadable(
so.name, so.text,
QString("`struct %1` has a base class, and this parser reads one "
"header as text — the inherited members are not in front "
"of it. Publishing the struct would drop exactly the "
"fields it cannot see. Declare the record without a base "
"and give it the inherited fields explicitly.")
.arg(so.name));
}
if (so.bodyOnOpeningLine) {
// The body shares the opening line, and the scan below starts on the
// NEXT one, so there is nothing for it to read. Say so instead of
// publishing an empty record.
reportUnreadable(so.name, so.text,
QString("The body shares the line with the opening "
"brace. Put each field on its own line. %1")
.arg(kFieldFormHint));
} else {
// The body is read as DECLARATIONS, not lines: physical lines are
// joined until the declaration is whole, which is a `;` at the
// struct's own brace depth — or a `}` there, which is how a member
// function DEFINED inline ends. Depth is tracked because a member
// function's body, and a nested type's, are declarations of their
// own that a `;` inside them must not be mistaken for the end of.
QString acc;
int depth = 1; // inside the struct
for (int j = so.bodyStart; j >= 0 && j < code.size(); ++j) {
QString body = code.at(j);
// An access specifier may share the line with a declaration, as
// in the class-body parser. Strip it before anything else, or
// `public: std::string id;` reads as a field whose TYPE is
// `public: std::string`.
while (true) {
const QRegularExpressionMatch am = accessRe.match(body);
if (!am.hasMatch()) break;
body = body.mid(am.capturedEnd()).trimmed();
}
if (body.isEmpty()) continue;
const int delta = braceDelta(body);
if (depth + delta <= 0) {
// End of the struct. Anything still accumulating never
// became a whole declaration — report it rather than
// dropping it on the way out.
if (!acc.isEmpty())
reportUnreadable(so.name, acc, kFieldFormHint);
break;
}
acc = acc.isEmpty() ? body : acc + ' ' + body;
depth += delta;
// Not a whole declaration yet: a field wrapped across physical
// lines is still the same field, and a `;` inside an inline
// member-function body does not end the member.
if (depth != 1 || !(acc.endsWith(';') || acc.endsWith('}')))
continue;
const QString decl = acc;
acc.clear();
// The DECLARATOR is everything before the first `=` or `{`. A
// default value may legally contain parentheses
// (`std::string id = makeId();`), and only parentheses in the
// declarator make the line a member function.
qsizetype cut = decl.size();
const qsizetype eq = decl.indexOf('=');
const qsizetype brace = decl.indexOf('{');
if (eq >= 0) cut = qMin(cut, eq);
if (brace >= 0) cut = qMin(cut, brace);
if (decl.left(cut).contains('('))
continue; // member function / constructor / destructor
if (notAFieldRe.match(decl).hasMatch())
continue;
if (nestedTypeRe.match(decl).hasMatch()) {
// A nested type is not a field — and the scanner used to
// walk straight into its body, folding the INNER type's
// members into this record's field list and stopping at the
// inner `};`, so the published record was made of another
// type's fields and missing all of its own.
reportUnreadable(
so.name, decl,
QString("A nested type is not a field, and its own "
"members were being folded into `%1`. Declare it "
"at namespace scope — it becomes a contract "
"`type` of its own — and give `%1` a field of "
"that type.")
.arg(so.name));
continue;
}
const QRegularExpressionMatch fm = fieldRe.match(decl);
if (!fm.hasMatch()) {
reportUnreadable(so.name, decl, kFieldFormHint);
continue;
}
FieldDecl fd;
fd.name = fm.captured(2).toStdString();
const QString spelling = fm.captured(1).trimmed();
fd.type = cppTypeToLidl(
spelling,
QString("type '%1': field '%2'").arg(so.name, fm.captured(2)),
spelling, so.name, /*nameEmitted=*/true);
td.fields.push_back(fd);
}
}
if (!td.fields.empty()) {
out.push_back(td);
} else {
while (g_unsupported.size() > diagMark) g_unsupported.removeLast();
if (!g_emptyRecords.contains(so.name))
g_emptyRecords.append(so.name);
}
if (!td.fields.empty()) out.push_back(td);
else while (g_unsupported.size() > diagMark) g_unsupported.removeLast();
}
return out;
}
@@ -469,7 +751,14 @@ static std::vector<TypeDecl> scanForRecords(const QStringList& lines)
// is allowed to do. A struct earns its place in the contract by appearing in a
// method or event signature — transitively, since a published record's own
// fields may name others.
static void keepOnlyReferencedRecords(ModuleDecl& module)
//
// Returns that referenced set. It is the withdrawal key for BOTH diagnostic
// channels: a struct the API never names promises nothing, so neither an
// unsupported field type nor a line the scanner could not read is a defect in
// it. The set — not the published types — is what a structural diagnostic is
// tested against, because the very failures being reported are the ones that
// keep a struct OUT of module.types.
static std::set<std::string> keepOnlyReferencedRecords(ModuleDecl& module)
{
auto mention = [](const TypeExpr& te, std::set<std::string>& out) {
std::function<void(const TypeExpr&)> walk = [&](const TypeExpr& t) {
@@ -506,6 +795,7 @@ static void keepOnlyReferencedRecords(ModuleDecl& module)
for (const TypeDecl& td : module.types)
if (referenced.count(td.name)) kept.push_back(td);
module.types = std::move(kept);
return referenced;
}
// ---------------------------------------------------------------------------
@@ -640,12 +930,14 @@ ImplParseResult parseImplHeader(const QString& headerPath,
{
ImplParseResult result;
// All three file-statics are per-parse state: one process generates for more
// Every file-static above is per-parse state: one process generates for more
// than one module. g_recordNames is cleared HERE as well as beside
// scanForRecords, because a name left over from the previous module's header
// would otherwise be visible while this one's metadata events are typed.
g_unmappableSpellings.clear();
g_unsupported.clear();
g_unreadable.clear();
g_emptyRecords.clear();
g_recordNames.clear();
QJsonArray metadataEvents;
@@ -995,7 +1287,66 @@ done:
// Now that every signature is known, drop the structs the API never
// mentions — a header's internal helpers must not become published
// contract types.
keepOnlyReferencedRecords(result.module);
const std::set<std::string> referenced = keepOnlyReferencedRecords(result.module);
// STRUCTURE the scanner could not read is a BUILD ERROR, not a shorter
// record.
//
// Reported before the type diagnostics below because it is the more
// fundamental failure: when the scanner could not read a struct's body, the
// types it did manage to read there are not a trustworthy account of it
// either. Reported after keepOnlyReferencedRecords, and tested against the
// REFERENCED set rather than the published one, for the reason given on that
// function: a helper struct the API never mentions may be as unreadable as
// it likes, while a struct that failed to publish anything is exactly the
// case that has to be caught.
{
QStringList reports;
QSet<QString> seen;
for (const UnreadableDecl& u : g_unreadable) {
if (!referenced.count(u.record.toStdString()))
continue; // struct never reaches the contract
const QString line =
QString(" type '%1': `%2`\n could not be read as a field. %3")
.arg(u.record, u.text, u.hint);
if (seen.contains(line)) continue;
seen.insert(line);
reports << line;
}
// A struct the API NAMES that published no field at all. Nothing above
// need have fired — a body of nothing but member functions reads
// perfectly well and yields no record — and the emitted contract would
// then reference a `type` it never declares, which no reader of the
// .lidl can resolve and no backend can generate.
for (const QString& name : g_emptyRecords) {
if (!referenced.count(name.toStdString())) continue;
bool explained = false;
for (const UnreadableDecl& u : g_unreadable)
if (u.record == name) { explained = true; break; }
if (explained) continue;
reports << QString(
" type '%1' is named by this module's API but declares "
"no field this parser could read, so no `type %1` is "
"emitted and the contract would name a type it never "
"declares.\n %2")
.arg(name, kFieldFormHint);
}
if (!reports.isEmpty()) {
result.error =
headerPath + ": " + QString::number(reports.size())
+ (reports.size() == 1 ? " declaration in a struct this module "
"publishes could not be read.\n\n"
: " declarations in structs this module "
"publishes could not be read.\n\n")
+ reports.join("\n\n")
+ "\n\nA `struct` in this header becomes a contract `type`, and its "
"field list IS the promise consumers in every language bind to. "
"Each of these used to be skipped, and the record published "
"without it — a contract missing a field is as well-formed as one "
"that has it, so nothing downstream could tell.\n";
return result;
}
}
// A C++ spelling with no LIDL type is a BUILD ERROR, not a silent `any`.
//