Merge da5a28819e into ac249ba4c9

evaluation/review-fixtures.json

@@ -33,13 +33,15 @@
       "articles": [
         "collect-validation-errors-with-errorbehavior",
         "defensive-vs-offensive-code-must-match-blast-radius",
-        "log-writes-must-survive-rollback"
+        "log-writes-must-survive-rollback",
+        "ignored-tryfunction-return-disables-try-semantics"
       ]
     },
     "events": {
       "articles": [
         "reset-ishandled-only-when-the-value-can-carry-over",
-        "changecompany-runs-triggers-in-the-calling-company"
+        "changecompany-runs-triggers-in-the-calling-company",
+        "avoid-raising-events-inside-try-functions"
       ]
     },
     "finance": {
@@ -85,7 +87,8 @@
         "use-setloadfields-for-partial-records",
         "prefer-modifyall-over-per-row-modify",
         "al-methods-limited-during-write-transactions",
-        "avoid-user-prompts-inside-transactions"
+        "avoid-user-prompts-inside-transactions",
+        "use-tryfunction-for-error-catching-not-rollback"
       ]
     },
     "privacy": {

microsoft/knowledge/error-handling/bare-tryfunction-call-propagates-errors.md

@@ -0,0 +1,31 @@
+---
+bc-version: [all]
+domain: error-handling
+keywords: [tryfunction, try-method, bare-call, error-propagation, swallowed-error, try-prefix, false-positive]
+technologies: [al]
+countries: [w1]
+application-area: [all]
+---
+
+# A bare call to a [TryFunction] propagates its error; it is not a swallowed failure
+
+## Description
+
+A bare call to a `[TryFunction]` procedure propagates errors like any ordinary method call. When the caller ignores the Boolean result, the platform does not treat the invocation as a try-method call: an error raised inside it stops the caller exactly as an unattributed procedure would, and nothing is caught or converted to `false`. Calling a try-API this way — for example the System Application `Xml Validation` procedures `TrySetValidatedDocument`, `TryAddValidationSchema`, and `TryValidateAgainstSchema` — is therefore a legitimate way to let a validation error reach the user. Reviewers who know only that try methods "catch errors" misread such a call as a silent failure.
+
+## Best Practice
+
+Treat a bare call to a `[TryFunction]` as a throwing call. Do not report that its failure is swallowed, ignored, or invisible to the caller, and do not ask the author to capture the result only to re-raise it: when the error should propagate, the bare call already does so and keeps the original error text, code, and call stack. Recommend consuming the result only when the surrounding code visibly expects to continue past or handle the failure; that case is owned by `microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.md`.
+
+A `Try` name prefix is a convention, not a semantic. Resolve the called procedure's declaration before reasoning about its error behaviour. A `Try`-named procedure without `[TryFunction]` is an ordinary Boolean method; whether ignoring its result loses a failure depends on its body, not on its name.
+
+## Anti Pattern
+
+Review findings that flag a bare `[TryFunction]` call whose surrounding code and documentation intend the error to propagate, for example: "the Try-prefixed calls ignore their Boolean return value, so a parse or validation failure is silently swallowed", "the caller can never learn whether validation succeeded", or "the try semantics never activate, so errors escape as ordinary exceptions". The first two claims are false; the third describes the intended behaviour, not a defect.
+
+Recommending `if not Try...() then Error(GetLastErrorText())` as the fix is part of the same false positive. It adds code, replaces the original error with a re-raised copy, and can move unsanitized customer content into the error message (see `microsoft/knowledge/privacy/getlasterrortext-customer-content-in-errors.md`).
+
+## References
+
+- [Handling errors using try methods](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-handling-errors-using-try-methods): "If a try method call doesn't use the return value, the try method operates like an ordinary method, and errors are exposed as usual."
+- [System Application `Xml Validation` codeunit](https://github.com/microsoft/BCApps/blob/main/src/System%20Application/App/XML%20Validation/src/XmlValidation.Codeunit.al): every `Try*` procedure is declared `[TryFunction]`.

microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.bad.al

@@ -1,9 +1,13 @@
 codeunit 50301 "Try Return Bad"
 {
-    procedure ImportDocument()
+    procedure ImportDocument(): Boolean
     begin
-        // Ignoring the Boolean result makes this an ordinary, throwing call.
+        // The bare call is not a try-method call: the error stops this procedure here,
+        // so the check below never sees it.
         TryImportDocument();
+        if GetLastErrorText() <> '' then
+            exit(false);
+        exit(true);
     end;
 
     [TryFunction]

microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.good.al

@@ -1,9 +1,17 @@
 codeunit 50300 "Try Return Good"
 {
-    procedure ImportDocument()
+    procedure ImportDocument(): Boolean
     begin
+        // The caller continues on failure, so the result is consumed.
         if not TryImportDocument() then
-            Error(ImportFailedErr);
+            exit(false);
+        exit(true);
+    end;
+
+    procedure ImportRequiredDocument()
+    begin
+        // The error should reach the user, so a bare call is correct.
+        TryImportDocument();
     end;
 
     [TryFunction]
@@ -13,6 +21,5 @@ codeunit 50300 "Try Return Good"
     end;
 
     var
-        ImportFailedErr: Label 'The document could not be imported.';
         SourceRejectedErr: Label 'The source document was rejected.';
 }

microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.md

@@ -1,30 +1,38 @@
 ---
 bc-version: [13..]
 domain: error-handling
-keywords: [tryfunction, try-method, boolean-return, ignored-return-value, error-propagation]
+keywords: [tryfunction, try-method, boolean-return, ignored-return-value, error-propagation, dead-failure-branch]
 technologies: [al]
 countries: [w1]
 application-area: [all]
 ---
 
-# Consume a TryFunction return value to enable try semantics
+# Consume a TryFunction return value when the caller handles the failure
 
 ## Description
 
-A procedure marked `[TryFunction]` catches errors only when the caller uses its Boolean return value. An assignment or conditional makes the invocation a try-method call; a bare call is treated as an ordinary procedure call and exposes errors as usual. The attribute alone does not make every invocation non-throwing.
+A `[TryFunction]` catches errors only when the caller uses its Boolean return value. An assignment or conditional makes the invocation a try-method call; a bare call is treated as an ordinary procedure call and exposes errors as usual. The attribute alone does not make every invocation non-throwing, so code that handles the failure of a bare call never sees that failure: the error leaves the procedure before the handling runs.
 
 ## Best Practice
 
-Consume the result directly: assign it to a Boolean or use the call in an `if` condition. Handle `false` immediately while the last-error state still describes that failure.
+When the caller must continue, log, count, or translate the failure, consume the result directly: assign it to a Boolean or use the call in an `if` condition. Handle `false` immediately while the last-error state still describes that failure.
+
+When the error should simply propagate, a bare call is correct and needs no change; see `microsoft/knowledge/error-handling/bare-tryfunction-call-propagates-errors.md`.
 
 See sample: [`ignored-tryfunction-return-disables-try-semantics.good.al`](ignored-tryfunction-return-disables-try-semantics.good.al).
 
 ## Anti Pattern
 
-Calling a `[TryFunction]` procedure as a standalone statement and assuming the attribute suppresses its errors. The call has ordinary error semantics because its Boolean result is ignored.
+Calling a `[TryFunction]` procedure as a standalone statement while the surrounding code expects the failure to be caught. Detect it from code, not from the call shape alone: the bare call is followed by a `GetLastErrorText`, `GetLastErrorCode`, or `GetLastErrorObject` check, a failure branch, failure logging, or a `false`/failure result; or it sits in a loop that is meant to continue past failed items. None of that handling sees the call's error, because the error stops the procedure first.
+
+A bare call with no such handling is intended propagation, not this anti-pattern.
 
 See sample: [`ignored-tryfunction-return-disables-try-semantics.bad.al`](ignored-tryfunction-return-disables-try-semantics.bad.al).
 
 ## See also
 
 `microsoft/knowledge/performance/use-tryfunction-for-error-catching-not-rollback.md` owns transaction rollback expectations after a try method has actually caught an error.
+
+## References
+
+- [Handling errors using try methods](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-handling-errors-using-try-methods): "If a try method call uses the return value in an `OK:=` statement or a conditional statement such as `if-then`, errors are caught."

microsoft/knowledge/events/avoid-raising-events-inside-try-functions.md

@@ -11,7 +11,7 @@ application-area: [all]
 
 ## Description
 
-A `TryFunction` catches all errors — including errors thrown by event subscribers. When an `[IntegrationEvent]` is raised inside a `TryFunction` body, any error a subscriber raises is silently swallowed by the TryFunction's error boundary. The subscriber's logic fails, the caller sees no error, and the calling code continues as if nothing happened. Subscribers have no way to signal failure to the caller.
+A `TryFunction` whose caller consumes its Boolean result catches all errors raised during its execution — including errors thrown by event subscribers. When an `[IntegrationEvent]` is raised inside a `TryFunction` body, any error a subscriber raises is silently swallowed by the TryFunction's error boundary. The subscriber's logic fails, the caller sees no error, and the calling code continues as if nothing happened. Subscribers have no way to signal failure to the caller.
 
 ## Best Practice
 

microsoft/knowledge/performance/use-tryfunction-for-error-catching-not-rollback.md

@@ -29,4 +29,4 @@ See sample: [`use-tryfunction-for-error-catching-not-rollback.bad.al`](use-tryfu
 
 ## See also
 
-`microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.md` owns the separate call-site rule that a try method's Boolean result must be consumed.
+`microsoft/knowledge/error-handling/ignored-tryfunction-return-disables-try-semantics.md` owns the separate call-site rule that only a call that consumes the Boolean result catches errors; `microsoft/knowledge/error-handling/bare-tryfunction-call-propagates-errors.md` records that a bare call propagates them as intended.

microsoft/skills/review/al-error-handling-review.md

@@ -48,7 +48,7 @@ Narrow the relevant files to the subset that applies to the changes under review
 - The changed procedures and triggers, weighted toward `OnValidate`/`OnInsert`/`OnModify` triggers, posting and validation routines, and procedures attributed with `[ErrorBehavior(...)]` or `[TryFunction]`.
 - Tokens extracted from the diff that relate to error surfacing and diagnostics (`Error`, `ErrorInfo`, `FieldError`, `TestField`, `Title`, `Message`, `DetailedMessage`, `AddAction`, `AddNavigationAction`, `RecordId`, `PageNo`, `ErrorBehavior`, `Collect`, `HasCollectedErrors`, `GetCollectedErrors`, `ClearCollectedErrors`, `ErrorType`, `Internal`, `Client`, `TryFunction`, `GetLastErrorText`, Boolean assignment).
 - For the outbound HTTP call paths identified in Source, include `HttpClient`, `Get`, `Post`, `HttpResponseMessage`, response use, and caller failure handling (including `[TryFunction]` call sites) in keyword and topic matching.
-- Resolve changed standalone call targets; when the target declaration has `[TryFunction]`, worklist the ignored-return rule even if the declaration itself is unchanged. Only assignment and conditional use activate try semantics.
+- Resolve changed standalone call targets; when the target declaration has `[TryFunction]`, worklist both `ignored-tryfunction-return-disables-try-semantics` and `bare-tryfunction-call-propagates-errors` even if the declaration itself is unchanged. Only assignment and conditional use activate try semantics. A `Try` name prefix does not establish `[TryFunction]`; resolve the declaration.
 
 A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
 
@@ -60,7 +60,8 @@ The following targeted checks cover every current `error-handling` article:
 - Developer-only invariant text is raised with default client visibility, or a user-actionable validation is hidden as `ErrorType::Internal` — `errortype-internal-vs-client-for-diagnostics`.
 - `FieldError` receives a complete capitalized sentence, repeats the field caption/value, or ends the predicate with punctuation — `fielderror-default-message-logic`.
 - An unguarded `FieldError` is used as though it performed a comparison, or `TestField` is forced onto a complex rule needing a tailored predicate — `fielderror-vs-testfield`.
-- A resolved call target is marked `[TryFunction]` but the call is a standalone statement whose Boolean result is ignored — `ignored-tryfunction-return-disables-try-semantics`. This call-site rule supersedes the performance TryFunction article unless writes and rollback expectations are also visible.
+- A resolved call target is marked `[TryFunction]`, the call is a standalone statement whose Boolean result is ignored, and the surrounding code expects the failure to be caught (a later last-error check, failure branch or result, or a loop meant to continue past failures) — `ignored-tryfunction-return-disables-try-semantics`. This call-site rule supersedes the performance TryFunction article unless writes and rollback expectations are also visible.
+- A bare `[TryFunction]` call without such handling is intended propagation — `bare-tryfunction-call-propagates-errors`. It suppresses findings, including agent findings, that call the failure swallowed or ask for the result to be captured and re-raised.
 - A plain `Error` represents a known actionable correction that can be expressed through `ErrorInfo` actions/navigation, or an `ErrorInfo` omits the context needed for that action — `prefer-errorinfo-for-actionable-errors`.
 
 Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.