[MODUSCarstenScholling]

Summary

PC0038 reported named Record return values without exit(...) as "not all code paths return a value", although the procedure had initialized the return variable through its fields. For records the named form is the idiomatic one: exit(LocalRecord) is a record assignment that drops filters, the current key and the rows of a temporary table.

Fixes #596

Changes

• Assigning a field of the named return (Result."No." := No;) counts as assignment.
• Conversions around var arguments are unwrapped before matching the named return. This covers a temporary record passed to a non-temporary var Record parameter, and Clear(Result).
• Init() and Clear() count as explicit initialization, even as the only statement. An empty body is still reported.
• Rule doc and regression catalog: new design decisions, plus runtime facts verified on BC 29.

Tests

• New NoDiagnostic fixtures:
  • Only these fail without the fix: field assignments (including in both branches), Clear() (alone and followed by a field assignment), and the non-temporary var argument.
  • The others pass without the fix and guard against regressions: view-state calls, ReadIsolation, Init(), the temporary var argument, and field assignment followed by Insert().
• New HasDiagnostic fixture: empty body.
• All PlatformCop tests pass; build for netstandard2.1, net8.0 and net10.0 succeeds; dotnet format is clean.

Docs: ALCops/alcops.dev (PR #215).

[Arthurvdv]

Thanks @MODUSCarstenScholling, this is a clean fix and the conversion unwrap for temporary records passed to a non-temporary var parameter is a nice catch. Tests pass locally (68/68 for NotAllCodePathsReturnValue) and the rule doc and regression catalog are updated. A few points from our review:

1. A field of the return used as a var argument or receiver is still reported
AssignmentInitializesNamedReturn accepts Result."No." := ..., but InvocationAssignsNamedReturn only accepts Result itself as receiver or var argument. A field access on the return isn't recognised in either spot, so these still produce the original false positive:

procedure Build(Txt: Text) Result: Record MyTable
begin
    Evaluate(Result."Entry No.", Txt);   // var argument is Result."Entry No."
end;

procedure Build2() Result: Record MyTable
var
    OutS: OutStream;
begin
    Result.Blob.CreateOutStream(OutS);   // receiver is Result.Blob
end;

Following the same reasoning as the PR (a field is part of the returned instance), reusing the field-of-return check for the receiver and the var argument would cover both. If you'd rather keep the diff small, please list it under the rule doc's known issues instead.

2. HasDiagnostic guards for the new acceptance path
The only new HasDiagnostic fixture is NamedRecordEmptyBody. If the IFieldAccess match were loosened by accident (for example, dropping the Instance check), the suite would still pass. Could you add fixtures that must still report?

• a field of the return assigned in only one branch (if C then Result."No." := 'A'; with no else)
• a field of a different record assigned while Result is untouched (Other."No." := 'A';)
• a field of the return that is only read (x := Result."No.";)

3. Fixtures and code that the change doesn't exercise
NamedRecordFilteredOnly, NamedRecordInitOnly, NamedRecordViewConfiguredOnly and NamedRecordReadIsolationAssignment were already accepted on main through the receiver rule. They're fine as regression pins, so no need to remove them. The receiver unwrap (invocation.Instance?.UnwrapConversions()) is described as added "for symmetry", and the doc notes no fixture covers it. Please either add a fixture that needs it or drop it, to keep the change minimal.

Nits

• The regression-catalog.md row includes runtime commentary ("exit(LocalRec) would drop filters ...") that belongs in the rule doc rather than the catalog's cause/fix column.
• The new skip block for NamedRecordReadIsolationAssignment sits above the existing skip blocks, so the order differs from the other gated cases.

[Arthurvdv]

Wow, you were fast 🤯 and the last commit this covers everything from the review it seems. LGTM, so let's merge!

[MODUSCarstenScholling]

Wow, you were fast 🤯

Because the force is with him! And the test framework 😄