lims, kanban: adopt the QML-surface drift guard, and stop the audit flagging guarded optional reads

examples/common/testkit/qml_surface.cpp

@@ -18,6 +18,10 @@ namespace morph::ladder::testkit {
 
 namespace {
 
+/// @brief The `typeof ` operator, trailing space included, as it appears
+///        before a dotted read the scanner is deciding about.
+const QString kTypeofPrefix = QStringLiteral("typeof ");
+
 /// @brief One-based line number of @p offset within @p text.
 /// @param text   The text @p offset indexes into.
 /// @param offset Character offset.
@@ -233,6 +237,39 @@ QmlScanResult scanQml(const QString& source, const QString& fileName, const QStr
             while (after < text.size() && text.at(after).isSpace()) {
                 ++after;
             }
+
+            // Is this read a *probe* -- `x.y !== undefined`, `x.y === undefined`,
+            // or `typeof x.y` -- rather than a use? A probe is how QML asks
+            // whether a conditionally-compiled member exists in this build, so
+            // the answer "it does not" is the expected one, not a defect.
+            {
+                const QString tail = text.mid(after, 20);
+                const bool comparedToUndefined =
+                    (tail.startsWith(QStringLiteral("!==")) || tail.startsWith(QStringLiteral("===")) ||
+                     tail.startsWith(QStringLiteral("!=")) || tail.startsWith(QStringLiteral("=="))) &&
+                    tail.contains(QStringLiteral("undefined"));
+                // `typeof` sits before the whole dotted expression, and the
+                // match starts at the *alias* -- in `typeof page.fixture.depth`
+                // the alias is `fixture`, five characters in. So walk back over
+                // any `page.`-style prefix before looking for the keyword.
+                // Checking a fixed offset only ever recognised `typeof alias.x`
+                // and silently missed every qualified read, which is the shape
+                // QML actually uses.
+                qsizetype before = match.capturedStart(0);
+                while (before > 0) {
+                    const QChar previous = text.at(before - 1);
+                    if (!previous.isLetterOrNumber() && previous != QLatin1Char('_') && previous != QLatin1Char('.')) {
+                        break;
+                    }
+                    --before;
+                }
+                const bool typeofApplied =
+                    before >= kTypeofPrefix.size() &&
+                    text.mid(before - kTypeofPrefix.size(), kTypeofPrefix.size()) == kTypeofPrefix;
+                if (comparedToUndefined || typeofApplied) {
+                    result.optionalProbes.insert(alias + QLatin1Char('.') + reference.member);
+                }
+            }
             if (after < text.size() && text.at(after) == QLatin1Char('(')) {
                 qsizetype close = 0;
                 reference.kind = QmlReferenceKind::Call;
@@ -452,6 +489,20 @@ QStringList QmlSurfaceAudit::run() const {
                         readProperties.insert(reference.member);
                     } else if (surface.methodArities.contains(reference.member)) {
                         calledMethods.insert(reference.member);  // a method reference, not a call
+                    } else if (byFile.value(reference.file)
+                                   .optionalProbes.contains(reference.alias + QLatin1Char('.') + reference.member)) {
+                        // This file probes the member for existence, so it is
+                        // written to cope with the member being absent -- see
+                        // QmlScanResult::optionalProbes. Nothing to report: the
+                        // QML is correct precisely *because* the member is
+                        // missing in this configure.
+                        //
+                        // Scoped to the probing file, not the whole audit: a
+                        // guard in one view says nothing about an unguarded read
+                        // in another. It does excuse every read of that member
+                        // within the file, which is deliberate -- the guard is
+                        // normally written once, on the `visible:` binding that
+                        // gates the rest.
                     } else {
                         findings.append(QStringLiteral("%1 reads '%2.%3' but %4 has no such property")
                                             .arg(site, reference.alias, reference.member, cls));

examples/common/testkit/qml_surface.hpp

@@ -3,6 +3,7 @@
 
 #include <QMap>
 #include <QObject>
+#include <QSet>
 #include <QString>
 #include <QStringList>
 #include <QVector>
@@ -125,6 +126,18 @@ struct QmlScanResult {
     /// `<id>.<alias>` shape every ladder rung writes. Used to notice a bridge
     /// the QML consumes signals from that the audit was never handed.
     QStringList connectionsTargets;
+    /// `alias.member` for every member this file *probes* for existence, by
+    /// comparing it against `undefined` or applying `typeof` to it.
+    ///
+    /// Such a read is a question, not a use: it is how QML asks whether a
+    /// conditionally-compiled member is present in this build. kanban's
+    /// `BoardView.qml` guards its dead-letter banner exactly this way, because
+    /// `BoardBridge::deadLetterCount` exists only under
+    /// `MORPH_BUILD_OFFLINE_SQLITE`. Reporting that as "reads a property the
+    /// bridge does not have" would be backwards -- the QML is handling the
+    /// absence correctly, and the only way to satisfy the audit would be to
+    /// delete the guard that makes it safe.
+    QSet<QString> optionalProbes;
 };
 
 /// @brief Strips comments and string-literal bodies from @p source, keeping

examples/common/testkit/test_qml_surface.cpp

@@ -26,6 +26,7 @@
 #include <QTextStream>
 #include <QVariantList>
 #include <catch2/catch_test_macros.hpp>
+#include <catch2/generators/catch_generators.hpp>
 #include <catch2/matchers/catch_matchers_string.hpp>
 #include <string>
 
@@ -568,3 +569,119 @@ TEST_CASE("QmlSurfaceAudit: an exemption that no longer suppresses anything is i
 }
 
 #include "test_qml_surface.moc"
+
+// ── Optional surface: a read that probes for `undefined` is a question ──────
+//
+// A member can exist in one configure and not another -- kanban's
+// `BoardBridge::deadLetterCount` is behind MORPH_BUILD_OFFLINE_SQLITE -- and the
+// QML that uses it guards the read so the binding stays inert when it is
+// absent. Reporting that as "reads a property the bridge does not have" is
+// backwards: the only way to satisfy the audit would be to delete the guard
+// that makes the QML correct.
+//
+// Every shape the scanner recognises gets its own case here. The first draft of
+// this rule shipped four comparison spellings and a `typeof` branch with
+// exactly one of them exercised -- by a rung whose QML happens to use `!==` --
+// and the `typeof` branch turned out not to work at all.
+
+namespace {
+
+/// @brief QML that reads `fixture.<member>`, optionally guarding the read.
+/// @param member The member to read -- one the fixture bridge does not have.
+/// @param guard  The guard expression, or empty for an unguarded read.
+/// @return QML text: the clean surface plus this one extra read.
+QString qmlReading(const QString& member, const QString& guard) {
+    const QString line =
+        guard.isEmpty()
+            ? QStringLiteral("    readonly property int extra: page.fixture.%1").arg(member)
+            : QStringLiteral("    readonly property int extra: %1 ? page.fixture.%2 : 0").arg(guard, member);
+    return cleanQml().replace(QStringLiteral("    function reload() {"),
+                              line + QStringLiteral("\n\n    function reload() {"));
+}
+
+}  // namespace
+
+TEST_CASE("QmlSurfaceAudit: a read guarded against undefined is a probe, not a finding", "[testkit][qml-surface]") {
+    // Every spelling the scanner accepts. `queueDepth` is deliberately not on
+    // the fixture bridge: the QML is asking whether it exists.
+    const QString guard = GENERATE(QStringLiteral("page.fixture.queueDepth !== undefined"),
+                                   QStringLiteral("page.fixture.queueDepth === undefined"),
+                                   QStringLiteral("page.fixture.queueDepth != undefined"),
+                                   QStringLiteral("page.fixture.queueDepth == undefined"),
+                                   QStringLiteral("typeof page.fixture.queueDepth"));
+    INFO("guard: " << guard.toStdString());
+
+    QTemporaryDir dir;
+    REQUIRE(dir.isValid());
+    writeQml(dir, QStringLiteral("Main.qml"), qmlReading(QStringLiteral("queueDepth"), guard));
+
+    SurfaceFixtureBridge bridge;
+    QmlSurfaceAudit audit{dir.path()};
+    audit.bind(QStringLiteral("fixture"), bridge);
+
+    const QStringList findings = audit.run();
+    INFO(describe(findings));
+    CHECK(findings.isEmpty());
+}
+
+TEST_CASE("QmlSurfaceAudit: an unguarded read of a member the bridge lacks is still a finding",
+          "[testkit][qml-surface]") {
+    // The teeth. A narrowing rule can only silence findings, so the case that
+    // matters is the one it must NOT silence.
+    QTemporaryDir dir;
+    REQUIRE(dir.isValid());
+    writeQml(dir, QStringLiteral("Main.qml"), qmlReading(QStringLiteral("queueDepth"), QString{}));
+
+    SurfaceFixtureBridge bridge;
+    QmlSurfaceAudit audit{dir.path()};
+    audit.bind(QStringLiteral("fixture"), bridge);
+
+    const QStringList findings = audit.run();
+    INFO(describe(findings));
+    REQUIRE(findings.size() == 1);
+    CHECK_THAT(findings.first().toStdString(), Catch::Matchers::ContainsSubstring("queueDepth"));
+    CHECK_THAT(findings.first().toStdString(), Catch::Matchers::ContainsSubstring("no such property"));
+}
+
+TEST_CASE("QmlSurfaceAudit: a probe in one file does not excuse an unguarded read in another",
+          "[testkit][qml-surface]") {
+    // The file scoping the rule documents. A guard is normally written once, on
+    // the binding that gates the rest of a view, so it excuses every read of
+    // that member *within its file* and nothing beyond it. Without the scope one
+    // guard anywhere would blind the audit to that member everywhere.
+    QTemporaryDir dir;
+    REQUIRE(dir.isValid());
+    writeQml(dir, QStringLiteral("Main.qml"),
+             qmlReading(QStringLiteral("queueDepth"), QStringLiteral("page.fixture.queueDepth !== undefined")));
+    writeQml(dir, QStringLiteral("Other.qml"), qmlReading(QStringLiteral("queueDepth"), QString{}));
+
+    SurfaceFixtureBridge bridge;
+    QmlSurfaceAudit audit{dir.path()};
+    audit.bind(QStringLiteral("fixture"), bridge);
+
+    const QStringList findings = audit.run();
+    INFO(describe(findings));
+    REQUIRE(findings.size() == 1);
+    CHECK_THAT(findings.first().toStdString(), Catch::Matchers::ContainsSubstring("Other.qml"));
+}
+
+TEST_CASE("QmlSurfaceAudit: a guarded read of a member the bridge DOES have still counts as binding it",
+          "[testkit][qml-surface]") {
+    // A probe must not become a way to hide dead surface: guarding a read of a
+    // member that exists still means the QML uses it, so it must not then be
+    // reported as unbound. `depth` is on the fixture bridge.
+    QTemporaryDir dir;
+    REQUIRE(dir.isValid());
+    const QString qml =
+        cleanQml().replace(QStringLiteral("page.fixture.depth : 0"),
+                           QStringLiteral("page.fixture.depth !== undefined ? page.fixture.depth : 0"));
+    writeQml(dir, QStringLiteral("Main.qml"), qml);
+
+    SurfaceFixtureBridge bridge;
+    QmlSurfaceAudit audit{dir.path()};
+    audit.bind(QStringLiteral("fixture"), bridge);
+
+    const QStringList findings = audit.run();
+    INFO(describe(findings));
+    CHECK(findings.isEmpty());
+}

examples/kanban/tests/test_kanban_qml_surface.cpp

@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: Apache-2.0
+//
+// The QML-visible surface of both kanban bridges, audited against `gui/qml/`
+// itself.
+//
+// kanban already guarded this by hand, in two files: `indexOfMethod`/
+// `indexOfSignal` lists naming what must exist, plus an exact
+// `propertyCount() - propertyOffset()` assertion. Those catch a *deletion* but
+// not the drift QML actually suffers -- a count stays satisfied when one
+// invokable is renamed and another added, and neither check ever reads a `.qml`
+// file, so a `Connections` handler for a signal that no longer exists remains
+// invisible. QML binds by string: such a mistake is not a compile error, not a
+// test failure, and not a QML warning. The pane simply stays empty.
+//
+// This file points `morph::ladder::testkit::QmlSurfaceAudit`
+// (`examples/common/testkit/qml_surface.hpp`) at the rung's own QML and lets
+// those files be the expectation, in both directions.
+//
+// ── The conditional surface ──────────────────────────────────────────────────
+//
+// `BoardBridge`'s QML surface depends on a compile-time switch:
+// `MORPH_BUILD_OFFLINE_SQLITE` adds `queueDepth` and `deadLetterCount`. The
+// hand-written guard has to branch on that (`test_board_qml_bridge.cpp` asserts
+// 7 properties or 5), because a count cannot describe a surface that changes
+// shape.
+//
+// The audit needs no branch to *find* the surface -- it reads the metaobject
+// that was actually built, so in a configure without the switch the two
+// properties simply are not there. But the exemption list below does need one,
+// because `queueDepth` is unbound in the configure where it exists: BoardView.qml
+// binds `deadLetterCount` and nothing reads `queueDepth`. An exemption for a
+// member the bridge does not have is itself a finding, so that entry has to be
+// compiled in only when the member is.
+//
+// This was found by CI, not locally. An earlier draft of this file asserted the
+// two configures produce identical findings; they do not, and the check behind
+// that claim had a grep that silently dropped half the audit's output. The
+// per-configure verification below is what should have been done first.
+//
+// What *also* needed work is the other direction. `BoardView.qml` reads
+// `deadLetterCount` unconditionally-looking but guards it with
+// `!== undefined`, precisely so the dead-letter banner stays hidden in an OFF
+// build. The audit used to report that as "reads a property the bridge has no
+// such property" -- backwards, since the only way to satisfy it would have been
+// to delete the guard that makes the binding safe. A read compared against
+// `undefined` is now understood as a *probe* rather than a use
+// (QmlScanResult::optionalProbes), so this rung needs no exemption for it.
+
+#include <QString>
+#include <QStringList>
+#include <catch2/catch_test_macros.hpp>
+
+#include "board_qml_bridge.hpp"
+#include "project_admin_qml_bridge.hpp"
+#include "testkit/backend_rig.hpp"
+#include "testkit/qml_surface.hpp"
+
+namespace {
+
+using morph::ladder::testkit::BackendRig;
+using morph::ladder::testkit::Mode;
+using morph::ladder::testkit::QmlSurfaceAudit;
+
+}  // namespace
+
+TEST_CASE("Every kanban bridge exposes exactly the surface gui/qml binds, and nothing more",
+          "[kanban][gui][qml-surface]") {
+    // No fixture and no session: the audit reads metaobjects and text, never
+    // dispatches an action, so a bare rig is all both constructors need.
+    BackendRig rig{Mode::Local, 1};
+    kanban::gui::BoardBridge boardBridge{rig.bridge(0), rig.executor()};
+    kanban::gui::ProjectAdminBridge projectAdminBridge{rig.bridge(0), rig.executor()};
+
+    QmlSurfaceAudit audit{QStringLiteral(MORPH_LADDER_SOURCE_ROOT "/examples/kanban/gui/qml")};
+    // gui/main.cpp supplies both under these keys, and every sub-view names its
+    // property the same, so one bind per bridge covers every file that reads it.
+    audit.bind(QStringLiteral("boardBridge"), boardBridge);
+    audit.bind(QStringLiteral("projectAdminBridge"), projectAdminBridge);
+
+    // ── The pre-existing backlog, recorded rather than swallowed ──────────
+    // The first run of this audit reported nine members BoardBridge publishes
+    // that no file under gui/qml/ binds. The other direction is clean, so no
+    // screen is broken; each is either dead surface or a missing control, and
+    // deciding which is per-member work this file does not do. They are listed
+    // here so the guard goes live now and catches the *next* drift in either
+    // direction, with the backlog itemised instead of hidden behind a lowered
+    // bar.
+    //
+    // The list is checked in both directions too: an exemption for a member
+    // that has since been deleted, or one QML has since bound, fails this test
+    // (testkit/qml_surface.hpp). It can only shrink deliberately.
+    //
+    // `syncStatusChanged` is in the list under protest: it is the NOTIFY signal
+    // behind `deadLetterCount`, which BoardView.qml *does* bind, so the signal
+    // is doing its job -- a property binding consumes a NOTIFY signal without
+    // an explicit Connections handler, and the audit does not model that.
+    // Teaching it to would change what ledger's list means too (morph#239 has
+    // the same shape), so it is recorded here and argued in morph#291 rather
+    // than fixed in passing.
+    //
+    // Same shape as ledger's (morph#239) and lims' (morph#287).
+    const QString backlog = QStringLiteral("unbound bridge surface, tracked in morph#291");
+    for (const auto* member : {"bound", "ruleCreated", "ruleDeleted", "attachmentUploaded", "attachmentDownloaded",
+                               "syncStatusChanged", "getRules", "refresh", "setAttachmentServerUrl"}) {
+        audit.allowUnbound(QStringLiteral("boardBridge"), QString::fromLatin1(member), backlog);
+    }
+#ifdef MORPH_BUILD_OFFLINE_SQLITE
+    // Only exists in this configure, and unbound in it: BoardView.qml binds
+    // `deadLetterCount` and reads `queueDepth` nowhere. Guarded by the same
+    // macro as the property, because the audit rejects an exemption naming a
+    // member the bridge does not have -- so an unguarded entry would fail the
+    // OFF configure exactly as its absence failed the ON one.
+    //
+    // Deliberately keyed off the macro rather than off
+    // `metaObject()->indexOfProperty("queueDepth") >= 0`, which would also work
+    // and is tempting because it cannot go stale. That is exactly why it is the
+    // worse choice here: asking the metaobject makes this test agree with
+    // whatever was built, including a build where the two disagree. A
+    // reconfigure from OFF to ON does not reliably re-run AUTOMOC, so the
+    // bridge can carry the macro on its compile line while its metaobject still
+    // lacks the property -- and this assertion is one of the few places that
+    // notices. Keyed off the macro it fails loudly on such a tree; keyed off the
+    // metaobject it would pass and let the stale build through.
+    audit.allowUnbound(QStringLiteral("boardBridge"), QStringLiteral("queueDepth"), backlog);
+#endif
+
+    const QStringList findings = audit.run();
+    INFO(findings.join(QStringLiteral("\n")).toStdString());
+    CHECK(findings.isEmpty());
+
+    // The audit is only as good as the files it found: the seven gui/qml ships.
+    CHECK(audit.scannedFiles() == QStringList{QStringLiteral("BoardView.qml"), QStringLiteral("LoginView.qml"),
+                                              QStringLiteral("Main.qml"), QStringLiteral("MembersView.qml"),
+                                              QStringLiteral("ProjectListView.qml"), QStringLiteral("RulesView.qml"),
+                                              QStringLiteral("TaskDetailPopup.qml")});
+}