diff --git a/custom/knowledge/error-handling/ambiguous-record-failure-handling-must-be-clarified.md b/custom/knowledge/error-handling/ambiguous-record-failure-handling-must-be-clarified.md index 1e12b15..b43f1bc 100644 --- a/custom/knowledge/error-handling/ambiguous-record-failure-handling-must-be-clarified.md +++ b/custom/knowledge/error-handling/ambiguous-record-failure-handling-must-be-clarified.md @@ -1,7 +1,7 @@ --- bc-version: [all] domain: error-handling -keywords: [insert, modify, delete, get, rename, boolean-return, clarify-before-building, ambiguous] +keywords: [insert, modify, delete, get, rename, evaluate, tryfunction, boolean-return, clarify-before-building, ambiguous] technologies: [al] countries: [w1] application-area: [all] @@ -12,14 +12,20 @@ application-area: [all] ## Description `Insert`, `Modify`, `Delete`, `Get`, and `Rename` all return a Boolean -indicating success. There are two legitimate ways to use that: let AL -raise its own runtime error when the return value is ignored and the call -fails, or check the return value explicitly and decide what happens on -failure (`if not Rec.Insert() then ...`). Both are correct in the right -context — an ignored `Get()` failing is often exactly the right behavior -when the record is expected to exist and its absence is a genuine bug; -an ignored `Insert()` failing on a duplicate key might instead be a normal, -recoverable case the caller should handle gracefully. +indicating success — and so does `Evaluate()` when parsing text into a +Decimal/Date/etc., and any `[TryFunction]` call. The same ambiguity applies +to all of them, not just the five record methods: there are two legitimate +ways to use a failed call, and the choice between them cannot be guessed. +Let AL raise its own runtime error when the return value is ignored and the +call fails, or check the return value explicitly and decide what happens on +failure (`if not Rec.Insert() then ...` / `if not Evaluate(Qty, Text) then +...`). Both are correct in the right context — an ignored `Get()` failing is +often exactly the right behavior when the record is expected to exist and +its absence is a genuine bug; an ignored `Insert()` failing on a duplicate +key might instead be a normal, recoverable case the caller should handle +gracefully; an ignored `Evaluate()` on user-supplied text defaulting to 0 +is fine for a display estimate but not for a value that becomes a posted +quantity. Because both are legitimate, this is not a pattern an AI assistant should resolve by guessing. When it is unclear from the surrounding code, the @@ -56,11 +62,12 @@ already signals which behavior is intended. ## The High-Stakes Case: Financially or Legally Significant Fields The most damaging version of guessing wrong isn't a crash — it's a guarded -`Get()` whose failure path silently returns a blank/zero value that then -flows into VAT, posting, or amount calculations. A crash is visible; a -silently wrong VAT posting group or a blank VAT registration number used -in compliance reporting is not, and it can post real, wrong financial data -before anyone notices. +lookup (`Get()`, `Evaluate()`, a `TryFunction`) whose failure path silently +returns a blank/zero value that then flows into VAT, posting, or amount +calculations. A crash is visible; a silently wrong VAT posting group, a +blank VAT registration number used in compliance reporting, or a +mis-parsed weight that silently becomes a zero-quantity posted line is +not — and it can post real, wrong financial data before anyone notices. ```al // ANTI-PATTERN: a guarded lookup defaults a VAT-relevant field to blank @@ -73,6 +80,17 @@ begin exit(Header."VAT Registration No."); exit(''); // silently wrong — this Get() should not be expected to fail end; + +// ANTI-PATTERN: the same shape via Evaluate() instead of Get() — a parse +// failure silently becomes 0, and 0 flows unchecked into a posted quantity. +local procedure ParseWeight(RawValue: Text): Decimal +var + Weight: Decimal; +begin + if Evaluate(Weight, RawValue) then + exit(Weight); + exit(0); // silently wrong if this feeds a posted line's Quantity +end; ``` Verified against Microsoft's own current Base Application source @@ -86,9 +104,41 @@ field (e.g. `VATSetup.Get() ? VATSetup."Alt. Cust. VAT Reg. Consistent" : "...Consist."::Default`), the fallback is always an explicit, named, business-meaningful default value — never a blank string or a bare zero. +### Trace before flagging — a matching shape is not automatically a violation + +A guarded lookup that superficially matches the anti-pattern above is only +a real violation if tracing forward confirms both: + +1. **The guard is actually reachable.** If the record's own creation and + deletion invariants guarantee the parent always exists by the time this + code runs (e.g. the parent is always inserted before the child can be + validated, and deleting the parent is blocked or cascades to the + child), the `else` branch is dead code in practice, not a live risk — + even though `TableRelation` alone never *enforces* that guarantee, so + guarding defensively is still reasonable engineering, just not evidence + of a bug. +2. **The defaulted value isn't caught by a downstream fail-loud check.** + If the blank/zero value only ever reaches a boolean gate or classifier + that itself calls `TestField`/`Error` before anything posts, the guard + two hops upstream isn't where the real safety property lives — the + downstream check is, and it's already doing its job. + +Evaluated case that looked like the anti-pattern but wasn't: a guarded +`Header.Get(DocumentNo)` feeding a VAT-registration-style field, where (1) +every real creation path inserted the header before the line could be +validated and the header's `OnDelete` blocked orphaning, and (2) the blank +fallback only ever reached a boolean classification gate whose own +consuming branch called `TestField` before touching anything postable. +Flagging that case would have been a false positive — the shape matched, +the risk didn't, because both tracing steps came back negative. + The rule this sharpens to: if the field being read feeds a VAT, posting, or amount calculation, resolving the ambiguity in [[clarify-before-building]]'s -favor is not optional — either the lookup shouldn't be guarded at all (the -parent record is expected to always exist, so let `Get()`/`TestField` fail -loud), or the fallback must be an explicit, named, deliberately-chosen -business default, never a blank or zero value that quietly passes through. +favor is not optional *unless tracing clears it* — either the lookup +shouldn't be guarded at all (the parent record is expected to always exist, +so let `Get()`/`TestField` fail loud), or the fallback must be an explicit, +named, deliberately-chosen business default, never a blank or zero value +that quietly passes through — or the trace shows the guard is unreachable +and/or downstream-protected, in which case it's a false alarm, not a fix. +See [[defensive-vs-offensive-code-must-match-blast-radius]] for the general +decision framework this case is an instance of.