feat(pc0038,fc0007,lc0089,lc0090) Improve flow-terminating built-in analysis

[MODUSCarstenScholling]

Summary

This PR strengthens PC0038's return-value flow analysis and establishes a shared, semantic definition of AL calls that terminate control flow.

It fixes the FieldError false positive reported in #463, prevents future false negatives caused by overly broad built-in name matching (#468), and migrates the related FC0007 and LC0089/LC0090 flow-aware behavior to the same shared classifier.

Issues addressed

Fixes #463
Fixes #468

PC0038: return-value flow analysis

• Extends named-return assignment tracking beyond ordinary statements:
  • var arguments in conditions
  • invocations using the named return as receiver
  • case selectors
  • while and repeat until conditions
  • for start/end bounds
  • foreach collection expressions
• Handles short-circuit evaluation correctly:
  • the left side of and / or is always evaluated
  • the right side is considered optional
  • conditional expressions remain conservative because either branch can be skipped
  • xor evaluates both operands and therefore does not use short-circuit handling
• Recognizes exhaustive case statements without else when all enum or option values visible in the current compilation are covered.
• Preserves the distinction between an explicit exit(<value>), a bare exit, and a fallthrough path with a definitely assigned named return.
• Removes the previous ThrowError special case because it is not part of the verified shared flow-termination contract.

Shared flow-terminator classification

Introduces ALCops.Common.FlowTerminatingBuiltIns as the single semantic classifier for built-in calls that unconditionally terminate normal flow.

The classifier accepts only these exact clean bindings:

• Dialog.Error
• Table.FieldError
• FieldRef.FieldError

The implementation intentionally validates both the built-in method and its containing built-in class. Checking only MethodKind.BuiltInMethod plus a method name would incorrectly classify a future unrelated built-in named Error or FieldError as terminating.

For incomplete code in the editor, the classifier supports invalid bindings only when the synthesized receiver has the matching semantic type:

• Dialog.Error
• Record.FieldError
• FieldRef.FieldError

This prevents diagnostic flicker while an argument or expression is incomplete, without treating user-defined methods named Error or FieldError as terminators.

The implementation uses centralized SDK name constants and direct comparer-based checks. The NAV SDK does not expose a stable public enum or identifier for individual built-in methods; direct comparisons avoid reflection and collection lookup overhead in analyzer hot paths.

FC0007: statement spacing

• Migrates ErrorOnly and ExitAndError classification to FlowTerminatingBuiltIns.
• Adds support for Record.FieldError(...) and FieldRef.FieldError(...) as scope-leaving statements.
• Keeps user-defined Error and FieldError procedures out of scope-leaving detection.
• Supports invalid editor-time FieldError bindings.
• Updates the diagnostic requirement to name the actual terminating call, such as FieldError().
• Updates the diagnostic resource description and internal rule documentation.

LC0089 / LC0090: cognitive complexity

• Migrates built-in Error and FieldError guard-clause detection to FlowTerminatingBuiltIns.
• Treats Record.FieldError(...) and FieldRef.FieldError(...) guards as complexity-neutral flow simplifications.
• Keeps user-defined Error and FieldError calls as normal calls that continue to add cognitive complexity.
• Preserves the existing syntax-specific handling for:
  • CurrReport.Break(), CurrReport.Skip(), and CurrReport.Quit()
  • equivalent CurrXMLport calls
  • loop-control statements such as break and continue

CurrReport and CurrXMLport calls are deliberately not part of FlowTerminatingBuiltIns: they may be valid guard exits for cognitive complexity, but they are not general procedure terminators.

Tests

Adds regression coverage for:

• named and unnamed PC0038 return values
• if / else, guard-clause, and case else paths
• Record.FieldError and FieldRef.FieldError
• incomplete Error and FieldError calls in error-tolerant editor fixtures
• exact clean built-in class/method pairs
• crossed and unrelated built-in class/method pairs
• FC0007 missing and valid spacing around FieldError
• FC0007 diagnostic wording for FieldError()
• LC0089/LC0090 FieldError guard clauses
• user-defined Error and FieldError name collisions
• existing CurrReport and CurrXMLport guard behavior

Known limitation

Error(ErrorInfo) is currently classified as flow-terminating even when ErrorInfo.Collectible = true and execution occurs in an ErrorBehavior::Collect scope. AL can continue after such a call.

Correctly distinguishing that behavior requires data-flow analysis of the ErrorInfo value and enclosing error-collection context, which is outside the current invocation-level classifier. This limitation is documented in the affected internal rule documentation.

[Arthurvdv]

Thanks for this one, the shared FlowTerminatingBuiltIns helper is a nice consolidation. I went through the PR manually and also ran a deeper review against the decompiled NAV SDK. Findings below, grouped by file, roughly in order of importance.

src/ALCops.PlatformCop/Analyzers/NotAllCodePathsReturnValue.cs

1. Exhaustive case detection misses enum extensions (new false negative). The check around line 374 uses IEnumBaseTypeSymbol.Values and ignores IsExtensible. In the SDK, SourceEnumTypeSymbol.Values is filled from the enum's own syntax only; the merge with EnumExtensionTypeSymbol.Values happens in the internal CompilationUtilities.GetEnumValues, which the analyzer never calls. So with enum E { A; B } Extensible = true plus enumextension EExt extends E { C }, a case X of X::A: ...; X::B: ...; end is treated as exhaustive and PC0038 stays silent although X::C leaves the return value unassigned. On main a case without else was always treated as fall-through. Minimal fix: treat IsExtensible enums as non-exhaustive, and add a HasDiagnostic fixture with an enumextension. The rule doc line "This includes enum extensions available at development time" is inaccurate as it stands.

2. repeat..until credits the until condition even when break bypasses it (false negative regression). Around line 248 the condition is analyzed as always evaluated, but IBreakStatement falls into default: return states; (~line 345). repeat if Done then break; ... until TryFetch(Result); therefore reports "assigned" although break exits before TryFetch runs. main returned only the body states for repeat and flagged this. IBreakStatement is already used in DataTransferTableResolver.cs, so break-exit states can be parked and unioned with the condition result.

3. Parenthesized conditions skip the short-circuit branch (false positive). AnalyzeCondition (~line 472) only unwraps IConversionExpression, but the binder binds ( ... ) to BoundParenthesizedExpression : IParenthesizedExpression. So if (TryFetch(Result) and Enabled) then hits ContainsBranchingExpression and fires PC0038, while the unparenthesized form (fixture NamedIfConditionGuaranteedLeft.al) passes. Same for if not (TryFetch(Result) and X) then. Peeling IParenthesizedExpression.Operand in UnwrapConversions (Common OperationExtensions.cs) or at the top of AnalyzeCondition fixes it. No fixture currently contains if ( or not (.

4. Ternary branch differs per TFM. The IConditionalOperatorExpression branch (~line 493) is compiled out under #if !NETSTANDARD2_1, so if (Flag ? Customer.Get(A) : Customer.Get(B)) then exit; gives no diagnostic on net8.0/net10.0 but fires on netstandard2.1. The interface type is indeed absent in the netstandard2.1 SDK, but OperationKind.ConditionalExpression and the Condition/WhenTrue/WhenFalse properties exist at runtime on every SDK that parses ? :. The repo pattern for this (see .claude/rules/analyzer-development.md and GetPropertyIfExists, already used further down in this file) is EnumProvider.OperationKind.ConditionalExpression + GetPropertyIfExists<IOperation>(...) without the #if. A ternary fixture would pin the behaviour on all TFMs.

5. Smaller: GetCaseSelectorType (~line 396) hand-rolls what the SDK's public ISymbol.GetTypeSymbol() already does; GetCaseLabelSymbol (~line 440) is a pure pass-through; and the (op as IOptionAccess)?.OptionSymbol patch (~line 448) belongs in Common's GetSymbolSafe (RecordGetProcedureArguments.cs special-cases it too).

src/ALCops.Common/FlowTerminatingBuiltIns.cs

6. The private IsSameName(left, right) => SemanticFacts.NameEqualityComparer.Equals(left, right) re-implements SemanticFacts.IsSameName(string, string), whose SDK body is exactly that. The repo already calls SemanticFacts.IsSameName directly in ~15 places and has a null-tolerant string?.IsSameName extension. Please replace the six call sites and drop the helper. While there: IsKnownInvalidBinding evaluates GetNavTypeKindSafe() three times and relies on A && B || C && (D || E) precedence; binding the kind once and parenthesizing would help. IsErrorOnDialog / IsFieldErrorOn each have a single caller and can be inlined so the file reads as the three-row table the remarks describe.

src/ALCops.Common.Test/FlowTerminatingBuiltInsTests.cs and ALCops.Common.Test.csproj

7. I'd like to drop the SymbolProxy : DispatchProxy tests and the System.Collections.Immutable <Reference> hack together. The csproj reference came in with "Hopefully repair 2 failing test runs" after the v18 CI jobs failed exactly these 7 tests with FileNotFoundException: System.Collections.Immutable, Version=9.0.0.0 at RuntimeMethodInfo.GetParameters: DispatchProxy reflects over every member of IInvocationExpression / IMethodSymbol (ImmutableArray-typed), which forces the SDK's SCI version to be loadable. No other test project needs that reference. What the tests actually verify is that the classifier's constant table matches itself: they mirror the assumption about Binder.CreateBadCall / ErrorMethodSymbol.ContainingSymbol instead of testing the binder, and the negative rows (Error on class Table, FieldError on Codeunit) are inputs no AL compilation can produce. Every reachable case already has a binder-driven .al fixture under PlatformCop.Test/Rules/NotAllCodePathsReturnValue. The only uncovered proxy case is an invalid-binding FieldRef.FieldError; one extra fixture in NoDiagnosticInDocumentWithErrors covers it. Then the test file (and its CA1852 [SuppressMessage], the repo's first; code-analysis.md wants #pragma or .editorconfig) and the csproj hunk can go.

src/ALCops.LinterCop/Analyzers/CognitiveComplexity.cs

8. IsGuardInvocation calls SemanticModel.GetOperation too early (~line 256). It runs before the existing cheap syntactic checks and without a callee-name pre-filter, so every if <cond> then AnyCall(); pays a semantic lookup even when the callee cannot be Error / FieldError. In the SDK, GetOperation is a read-lock cache hit if the enclosing statement is already bound, otherwise it binds the enclosing statement under a write lock. When other operation-registering analyzers are active the block is already bound, but with LC0089 enabled alone (or in trigger blocks, where the recursion pass is gated on MethodDeclaration) this becomes a real per-statement bind. Suggested order: run the syntactic switch (CurrReport.Break/Skip/Quit, identifier set) first and return early, then pre-filter the callee name (IdentifierNameSyntax / MemberAccessExpressionSyntax.Name via SemanticFacts.IsSameName against Error / FieldError), and only then call GetOperation + FlowTerminatingBuiltIns.IsFlowTerminatingCall. GetSymbolInfo is not a cheaper substitute (same GetBoundNodes path, and IsInvalid is needed anyway).

src/ALCops.FormattingCop/Analyzers/StatementBlocksSeparatedByBlankLine.cs

9. FC0007 message text varies with user casing on the invalid-binding path (~line 217). The message now interpolates targetMethod.Name. Table.FieldError and FieldRef.FieldError each have two overloads, so Binder.CreateBadCall produces an ErrorMethodSymbol whose Name is the identifier text as typed. Typing rec.fielderror( reports 'fielderror()' and flips to 'FieldError()' once bound. The fixture FieldErrorSpacingMissingInvalid.al uses canonical casing and the only message assertion runs against the valid fixture, so this is untested. Suggestion: have FlowTerminatingBuiltIns return which built-in matched (canonical name) and format from that.

Minor

• src/ALCops.Common/Settings/alcops.schema.json (~line 122): the ScopeLeavingMode description still says 'ErrorOnly' checks only built-in 'Error(...)' while the analyzer now gates FieldError under the same mode. The parity test compares enum member names only, so it won't catch this.
• EnumProvider.cs (~line 354): ParseEnum<NavTypeKind>("Dialog") uses a string literal although NavTypeKind.Dialog exists in all three shipped SDKs (12.0.13 / 16.0.27 / 18.0.36). The file's convention reserves literals for members missing on some TFM, so nameof fits here.
• Docs: alcops.dev#164 (and the overlapping Bump gittools/actions from 4.3.3 to 4.4.2 #153) cover the FC0007 / PC0038 / LC0089 pages. Let's make sure one lands with this PR and the other is closed.

Checked and fine: EnumProvider.NavTypeKind.Dialog semantics, the bare incomplete Error( path resolving to the Dialog class, no fixtures or tests removed, ThrowError removal (no such built-in), and FlowAnalyzer holding no shared analyzer state.

[Arthurvdv]

I’ve taken the liberty of making a small commit to fix the failing tests (version-gate the tests that fail on older AL versions), so we can get this into the main branch.

@MODUSCarstenScholling, are you okay with me merging this?

[CarstenScholling]

That's great @Arthurvdv
Thank you.