Microsoft/bcquality
1
0
Fork 0
mirror of https://github.com/microsoft/BCQuality.git synced 2026-10-05 14:46:55 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

Merge pull request #72 from Curabis/rule/ambiguous-record-failure-edison-sharpen

Browse source 
[BCQuality] Sharpen ambiguous-record-failure rule per Edison eval (trace before flagging)
This commit is contained in:
Michael Dieringer 2026-08-13 22:33:29 +02:00 • committed by GitHub
commit b02eb2d6aa
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
1 changed files with 68 additions and 18 deletions
Download patch file Download diff file Expand all files Collapse all files

86
custom/knowledge/error-handling/ambiguous-record-failure-handling-must-be-clarified.md
View file

@ -1,7 +1,7 @@
--- ---
bc-version: [all] bc-version: [all]
domain: error-handling 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] technologies: [al]
countries: [w1] countries: [w1]
application-area: [all] application-area: [all]
@ -12,14 +12,20 @@ application-area: [all]
## Description ## Description
`Insert`, `Modify`, `Delete`, `Get`, and `Rename` all return a Boolean `Insert`, `Modify`, `Delete`, `Get`, and `Rename` all return a Boolean
indicating success. There are two legitimate ways to use that: let AL indicating success — and so does `Evaluate()` when parsing text into a
raise its own runtime error when the return value is ignored and the call Decimal/Date/etc., and any `[TryFunction]` call. The same ambiguity applies
fails, or check the return value explicitly and decide what happens on to all of them, not just the five record methods: there are two legitimate
failure (`if not Rec.Insert() then ...`). Both are correct in the right ways to use a failed call, and the choice between them cannot be guessed.
context — an ignored `Get()` failing is often exactly the right behavior Let AL raise its own runtime error when the return value is ignored and the
when the record is expected to exist and its absence is a genuine bug; call fails, or check the return value explicitly and decide what happens on
an ignored `Insert()` failing on a duplicate key might instead be a normal, failure (`if not Rec.Insert() then ...` / `if not Evaluate(Qty, Text) then
recoverable case the caller should handle gracefully. ...`). 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 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 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 High-Stakes Case: Financially or Legally Significant Fields
The most damaging version of guessing wrong isn't a crash — it's a guarded 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 lookup (`Get()`, `Evaluate()`, a `TryFunction`) whose failure path silently
flows into VAT, posting, or amount calculations. A crash is visible; a returns a blank/zero value that then flows into VAT, posting, or amount
silently wrong VAT posting group or a blank VAT registration number used calculations. A crash is visible; a silently wrong VAT posting group, a
in compliance reporting is not, and it can post real, wrong financial data blank VAT registration number used in compliance reporting, or a
before anyone notices. 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 ```al
// ANTI-PATTERN: a guarded lookup defaults a VAT-relevant field to blank // ANTI-PATTERN: a guarded lookup defaults a VAT-relevant field to blank
@ -73,6 +80,17 @@ begin
exit(Header."VAT Registration No."); exit(Header."VAT Registration No.");
exit(''); // silently wrong — this Get() should not be expected to fail exit(''); // silently wrong — this Get() should not be expected to fail
end; 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 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, "...Consist."::Default`), the fallback is always an explicit, named,
business-meaningful default value — never a blank string or a bare zero. 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, 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 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 favor is not optional *unless tracing clears it* — either the lookup
parent record is expected to always exist, so let `Get()`/`TestField` fail shouldn't be guarded at all (the parent record is expected to always exist,
loud), or the fallback must be an explicit, named, deliberately-chosen so let `Get()`/`TestField` fail loud), or the fallback must be an explicit,
business default, never a blank or zero value that quietly passes through. 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.