Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
add44a9
feat(common): add GetReceiverTableType helper for all four receiver f…
Arthurvdv Sep 3, 2026
4eada8c
fix(AC0030): flag bare implicit-self database reads (#348)
Arthurvdv Sep 3, 2026
d5fff0f
fix(DC0002): flag bare implicit-self Validate on flow fields (#348)
Arthurvdv Sep 3, 2026
8ae238e
fix(LC0040): flag bare/this implicit-self trigger invocations (#348)
Arthurvdv Sep 3, 2026
55d264d
fix(LC0081): flag bare/this implicit-self Count comparisons (#348)
Arthurvdv Sep 3, 2026
0c96ca2
fix(PC0003): flag bare implicit-self SetRange with filter operators (…
Arthurvdv Sep 3, 2026
a22f3f3
fix(PC0013): flag bare implicit-self Get argument mismatches (#348)
Arthurvdv Sep 3, 2026
ecc9db5
fix(PC0022): flag bare implicit-self Get argument overflow (#348)
Arthurvdv Sep 3, 2026
ca32eff
test(PC0027): pin receiver-form matrix as by-design (#348)
Arthurvdv Sep 3, 2026
f1114c8
fix(PC0029): flag bare/this CreateGuid in field and Validate paths (#…
Arthurvdv Sep 3, 2026
a56b3d0
fix(PC0020): resolve bare/this TransferFields target in table extensi…
Arthurvdv Sep 3, 2026
f2a89bc
fix(LC0086): suppress false positives for bare/this field writes and …
Arthurvdv Sep 3, 2026
b924101
test(LC0096): pin this.MyProc(this) as known limitation (#348)
Arthurvdv Sep 3, 2026
f7c7e45
test(AC0031): pin bare call in tableextension as regression fixture (…
Arthurvdv Sep 3, 2026
d7a009a
test(AC0032): pin Rec self-access receiver form (#348)
Arthurvdv Sep 3, 2026
c5880ab
test(PC0037): pin this receiver form as HasDiagnostic (#348)
Arthurvdv Sep 3, 2026
24a1770
test(LC0063): pin bare implicit-with source expression as known limit…
Arthurvdv Sep 3, 2026
2a2c7bc
docs: record the four receiver forms in the development guide, skills…
Arthurvdv Sep 3, 2026
d45036c
fix(LC0086): resolve this-receiver via the operation tree on pre-14.2…
Arthurvdv Sep 4, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .claude/rules/analyzer-development.md
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,16 @@ if (receiverExpression is not null && receiverExpression is not IdentifierNameSy

This is the same mechanism AC0031 (`RequiredPermissionDetector.TryGetFromInvocation`) uses via `invocation.Instance.Type`. Keep the variable-map fast path first so `GetOperation` (the ~0.3ms call) only runs for the rare non-identifier receivers.

#### Verified SDK facts (issue #348)

These were confirmed by auditing 18 receiver-relevant analyzers across all cops:

- **`Rec` vs `this` symbol binding**: `Rec` binds to a synthesized global VARIABLE symbol named `"Rec"`, while `this` binds to the record TYPE symbol named after the table (e.g. `"Sales Line"`). Name-keyed maps and symbol equality see different keys for the same instance; `GetReceiverTableType` normalizes both.
- **Pages/reports/xmlports**: `this` on a page binds to the page object symbol, not a record. The receiver-form matrix applies only inside tables and tableextensions.
- **Tableextensions**: `this`/`Rec`/bare all bind to the TARGET table's record. Containing-symbol fallbacks must unwrap the extension via `IApplicationObjectExtensionTypeSymbol.Target`.
- **Canonical resolution**: `GetReceiverTableType` in `ALCops.Common/Extensions/OperationExtensions.cs` is the canonical helper for resolving `IInvocationExpression.Instance` / `IFieldAccess.Instance` (including the null-Instance bare form) to the backing `IRecordTypeSymbol`.
- **`GetSymbolInfo` on a `this` receiver returns no symbol before AL 14.2**: `BoundThisReference` only gained its `ExpressionSymbol => Type` override in SDK 14.2.19, so on 14.0-14.1 `GetSymbolInfo(thisExpr).Symbol` is null while `GetOperation(thisExpr)?.Type` resolves the record type on all versions — so a `GetSymbolInfo`-based receiver fast path must fall back to the operation tree for non-identifier receivers (see LC0086).

### Detecting `this`/self at the operation level (`OperationKind.ThisReference`)

When you already hold the bound `IOperation` (e.g. `IFieldAccess.Instance` inside a `RegisterOperationAction`) rather than syntax, detect a `this`/self reference via the **`OperationKind` enum**, not the `IInstanceReferenceOperation` type:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
paths:
- "src/ALCops.ApplicationCop/**/UseReturnValueForDatabaseReadMethods*"
---

# AC0030: UseReturnValueForDatabaseReadMethods

## Purpose

Detects database read method calls (Find, FindFirst, FindLast, FindSet, Get, GetBySystemId) whose boolean return value is discarded.

## Design decisions

| Decision | Rationale |
|---|---|
| Receiver forms: all four handled via `GetReceiverTableType` (#348) | Phase-0 helper adoption fixed the bare-call gap; no form-specific logic needed |

## Known issues

- None specific to receiver forms after #348.
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ Maps AL built-in record methods to `DatabaseOperation`:
## Known issues

- **IntegerTable** test case is skipped (commented out)
- **Bare implicit calls** (`Modify()` without `Rec.`) inside table objects may not be detected as invocations; use `Rec.Modify()` pattern
- ~~**Bare implicit calls** (`Modify()` without `Rec.`) inside table objects may not be detected~~ Fixed by `GetReceiverTableType` helper (#348); all four receiver forms now resolved correctly, including bare calls in tableextensions
- **CalcFields/CalcSums** are not yet covered (out of scope for initial implementation)
- **CodeFix: blank line formatting** When creating a new Permissions property on an object that has no properties, no blank line is inserted between the new property and the first member (trigger/procedure)
- **CodeFix: cross-namespace test** The single-file test framework cannot test qualified table name resolution; both objects must be in the same file
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,8 @@ Each `SyntaxNodeAction` callback is self-contained with no shared mutable state.
4. **Cross-object calls**: If codeunit A calls codeunit B, and B accesses a table, A's permission for that table appears unused (correct, because permissions don't flow through the call stack). The reverse is not a limitation: when A iterates a set that B positioned (`Rec.Next()` in A), A's `r` is counted as used, because `Next` itself reads the database in A
5. **RecordRef bailout hides true positives**: When the whole-object bailout triggers, genuinely unused permissions in that object are no longer reported (accepted trade-off; see design decisions)
6. **DataTransfer bailout hides true positives**: a `CopyFields`/`CopyRows` that no `SetTables` reaches in the same body, or whose table arguments are not `Database::X` literals, silences AC0032 for the whole object — genuinely unused entries there go unreported (same trade-off as the RecordRef bailout). One accepted imprecision remains in the flow walk: `exit` does not terminate a path, so state from before an early exit still flows forward and an executor can be attributed a table its path never reaches. That over-attribution is conservative for AC0032 — it can only make a permission look used — but the same walk backs AC0031, where it can produce a spurious "missing permission" report
7. **FieldRef access is not a DB operation**: `FieldRef.Value`/`Field`/`Caption` operate on the in-memory current row and neither consume a permission nor trigger the RecordRef bailout (verified: only mapped DB methods on the RecordRef itself count). `FieldRef.CalcField` does read the database but is not traced, consistent with limitation 2
7. **Receiver forms fully covered** (#348): All four forms (named variable, Rec, bare, this) plus tableextension variants are resolved; the `this` form false positive from #343 is fixed
8. **FieldRef access is not a DB operation**: `FieldRef.Value`/`Field`/`Caption` operate on the in-memory current row and neither consume a permission nor trigger the RecordRef bailout (verified: only mapped DB methods on the RecordRef itself count). `FieldRef.CalcField` does read the database but is not traced, consistent with limitation 2

## CodeFix: TableDataAccessUnusedPermissionsCodeFixProvider

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
paths:
- "src/ALCops.DocumentationCop/**/WriteToFlowFieldRequiresComment*"
---

# DC0002: WriteToFlowFieldRequiresComment

## Purpose

Detects `Validate` calls on FlowFields without a preceding comment explaining why.

## Design decisions

| Decision | Rationale |
|---|---|
| Receiver forms: bare implicit self fixed (#348) | The analyzer previously gated on non-null invocation instance; bare `Validate()` in a table was missed |

## Known issues

- None specific to receiver forms after #348.
20 changes: 20 additions & 0 deletions .claude/rules/diagnostics/lc0040-explicitly-set-run-trigger.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
paths:
- "src/ALCops.LinterCop/**/ExplicitlySetRunTrigger*"
---

# LC0040: ExplicitlySetRunTrigger

## Purpose

Detects Insert/Modify/Delete/DeleteAll calls where the RunTrigger parameter is not explicitly set.

## Design decisions

| Decision | Rationale |
|---|---|
| Receiver forms: bare and this fixed (#348) | Both were missed because the analyzer only checked `MemberAccessExpressionSyntax` receivers |

## Known issues

- None specific to receiver forms after #348.
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
---
paths:
- "src/ALCops.LinterCop/**/ApiPageCanonicalFieldNameGuide*"
---

# LC0063: ApiPageCanonicalFieldNameGuide

## Purpose

Detects API page field names that do not follow the canonical naming convention (e.g. `no` instead of `number` for a `"No."` source field).

## Design decisions

| Decision | Rationale |
|---|---|
| Only checks fields with `Rec.FieldName` source expressions | `IsIdentifierValueTextRec` requires `MemberAccessExpressionSyntax` with an identifier receiver named "Rec" |

## Known issues

| Issue | Status |
|---|---|
| Bare implicit-with source expression bypasses check (#348) | Known limitation, pinned by `NoDiagnostic/BareImplicitWithSourceExpression.al`. A field like `field(no; "No.")` (without `Rec.`) is not analyzed because the expression is not a `MemberAccessExpressionSyntax`. Non-trivial to fix (requires semantic resolution of the page field source). |
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
paths:
- "src/ALCops.LinterCop/**/AnalyzeCountMethod*"
---

# LC0081 / LC0082: UseIsEmptyMethodInsteadOfCount / UseQueryOrFindWithNextInsteadOfCount

## Purpose

LC0081 flags `Count() = 0` / `Count() > 0` patterns that should use `IsEmpty`. LC0082 flags `Count() > 1` / `Count() = N` patterns that should use `FindSet`+`Next`.

## Design decisions

| Decision | Rationale |
|---|---|
| Receiver forms: bare and this fixed (#348) | Both share the same `AnalyzeCountMethod` analyzer; the invocation-instance null check skipped bare self-calls |

## Known issues

- None specific to receiver forms after #348.
2 changes: 2 additions & 0 deletions .claude/rules/diagnostics/lc0086-page-style-string-literal.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ Detects string literals that match `PageStyle` enum value names (e.g., `'Unfavor
| Unlocked labels: skip (no diagnostic) | Unlocked labels are translatable text, not style constants |
| StyleExpr direct: skip `StyleExpr = 'Standard'` | Already using the string in the correct property; the fix is to change the property value type, not the location |
| Table field writes: skip `MyRecord.MyField := 'Standard'` | Writing data to a record field, not styling |
| Receiver forms: bare/this field writes suppressed (#348) | The suppression checks `IFieldAccess.Instance` type; bare self-reference (null instance) and `this` both resolve via `GetReceiverTableType` to the record type, correctly triggering the field-write suppression |
| Data-access args: skip arguments to Record/RecordRef/FieldRef/Query methods | Data operations, not styling |
| Data-access receiver: `GetSymbolInfo` fast path + `GetOperation` fallback for non-identifier receivers | On AL 14.0-14.1, `GetSymbolInfo` returns no symbol for a `this` receiver: `SemanticModel.GetSymbolInfo` reads `BoundExpression.ExpressionSymbol`, and `BoundThisReference` only gained its `ExpressionSymbol => Type` override in SDK 14.2.19 (verified by diffing decompiled SDK v14.1.18.1238..v14.2.19.4832). `GetOperation(receiver)?.Type` binds `this` to the record type on all versions. The fallback runs only when the fast path resolves nothing AND the receiver is not a plain identifier, keeping the ~300us `GetOperation` off the common named-variable path. Note `IsWritingToTableField` needs no fallback: it resolves the member *name* (`this.MyField`), which binds through the field-access bound node, not `BoundThisReference`. |
| Flow analysis (Option C): rejected | Tracing `StyleExpr = myVar` -> `myVar := 'Standard'` across triggers/methods has PC0030-level complexity. BC.LinterCop also doesn't do this. |

## Architecture
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ The SDK's `OperationExtensions.GetSymbol()` throws `InvalidCastException` when t

The analyzer uses `GetSymbolSafe()` (from `ALCops.Common.Extensions.OperationSafeExtensions`) on all `GetSymbol()` call sites. This method handles the bug without exception handling: it checks `is IApplicationObjectAccess` first (returning the `ApplicationObjectTypeSymbol`), then guards any remaining `FieldAccess`-kind operations that don't implement `IFieldAccess` by returning null. The `DatabaseObjectReference` NoDiagnostic test case covers this pattern (`DATABASE::MyTable` as a method argument).

### `this.MyProc(this)` false negative (#348)

`GetSymbolSafe()` returns null for `this` references (no `OperationKind` case in the SDK's `GetSymbol` switch for `BoundThisReference`). When both the instance and the argument are `this`, the symbol comparison fails because neither resolves. Pinned by `NoDiagnostic/ExternalThisSelfMethodCall.al`. Fixing requires detecting `ThisReference` kind on both sides.

### IConversionExpression wrapping

Arguments may be wrapped in `IConversionExpression` by the SDK. When `argument.Value.GetSymbolSafe()` returns null, the analyzer unwraps through the conversion and calls `GetSymbolSafe()` on the operand.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
paths:
- "src/ALCops.PlatformCop/**/SetRangeWithFilterOperators*"
---

# PC0003: SetRangeWithFilterOperators

## Purpose

Detects `SetRange` calls that use filter operators (e.g. `*`, `..`) which should use `SetFilter` instead.

## Design decisions

| Decision | Rationale |
|---|---|
| Receiver forms: bare implicit self fixed (#348) | The analyzer gated on non-null `IInvocationExpression.Instance`; bare `SetRange()` in a table was missed |

## Known issues

- None specific to receiver forms after #348.
20 changes: 20 additions & 0 deletions .claude/rules/diagnostics/pc0013-record-get-procedure-arguments.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
paths:
- "src/ALCops.PlatformCop/**/RecordGetProcedureArguments*"
---

# PC0013: RecordGetProcedureArguments

## Purpose

Detects `Get` calls where the argument count does not match the primary key field count.

## Design decisions

| Decision | Rationale |
|---|---|
| Receiver forms: bare implicit self fixed (#348) | The analyzer gated on non-null invocation instance; bare `Get()` in a table was missed |

## Known issues

- None specific to receiver forms after #348.
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ PK fields; a constant-`true` third argument (`SkipFieldsNotMatchingType`) suppre

- Affix fixtures (`Affix_*`) inject an `AppSourceCop.json` via `MemoryFileSystem`; this requires `Microsoft.Dynamics.Nav.Analyzers.Common.dll` as a `Private=True` reference in the test csproj (ALCops.Common references it with `Private=False`).

- **Receiver forms**: bare/this TransferFields target in tableextensions resolved via `GetReceiverTableType` (#348); all four receiver forms now work correctly for target table resolution.
- `TransferFieldsRelations.TableRelations` is a curated static list with BC version ranges
(`MinVersion`/`MaxVersion`); relation-path coverage only applies to listed pairs.
- No CodeFix.
20 changes: 20 additions & 0 deletions .claude/rules/diagnostics/pc0022-possible-overflow-assigning.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
paths:
- "src/ALCops.PlatformCop/**/PossibleOverflowAssigning*"
---

# PC0022: PossibleOverflowAssigning

## Purpose

Detects possible data loss when a longer Text/Code value is assigned or passed into a shorter destination — including `Get(...)` arguments checked against the primary-key field lengths.

## Design decisions

| Decision | Rationale |
|---|---|
| Receiver forms: bare implicit self fixed (#348) | The analyzer gated on non-null invocation instance; bare `Get()` in a table was missed |

## Known issues

- None specific to receiver forms after #348.
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
paths:
- "src/ALCops.PlatformCop/**/TemporaryRecordTriggerInvocation*"
---

# PC0027: TemporaryRecordTriggerInvocation

## Purpose

Detects invocations of trigger-executing methods (Insert, Modify, Delete) on temporary record variables, where the triggers have no effect.

## Design decisions

| Decision | Rationale |
|---|---|
| Bare and this self-forms: by-design no diagnostic (#348) | Self-reference forms inside a table/tableextension target the object's own record, which is not a local temporary variable. No realistic use case for triggering this rule on self-calls. Pinned by `test(PC0027)` commit. |

## Known issues

- None specific to receiver forms; by-design verdicts pinned by fixture matrix.
4 changes: 4 additions & 0 deletions .claude/rules/diagnostics/pc0029-use-sequential-guid.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ The BC SDK's `OperationWalker` does NOT preserve `IOperation` reference identity

## Known issues and workarounds

### Receiver forms (#348)

Bare and `this` CreateGuid flows (assignment to key field, Validate call) are correctly detected via the `IFieldAccess.Instance` / `IInvocationExpression.Instance` operation tree. The `CheckFieldInKey` and `CheckValidateTarget` paths resolve the record type through the instance operation, which handles all four receiver forms without explicit form detection.

### IConversionExpression wrapping

Arguments and assignment values may be wrapped in `IConversionExpression` by the SDK. The `UnwrapConversion()` helper strips this layer before checking for `CreateGuid()` or resolving symbols.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ These decisions were made during the initial design and should be preserved unle
| Exit statement escape — Retroactive-only clear of `UncoveredReads` on the current path; NO forward flow flag | `exit(Rec)` escapes the procedure scope, so the caller might need the full record. `exit` terminates the path, so forward state is unreachable; setting a flow flag would leak suppression past the enclosing branch via OR-merge, causing false negatives on reads after an early-exit guard (`if Cond then exit(Rec); Rec.Get(X);` must still fire). Only bare variable references suppress (`exit(Rec."No.")` does not). Sets method-level `EverPassedToFunction` for RecordRef SetTable suppression. See [#429](https://github.com/ALCops/Analyzers/issues/429). |
| Version gate — `Spring2021OrGreater` (runtime 6.0, BC17) · Full netstandard2.1 support | `SetLoadFields` was introduced with runtime 6.0 |

## Receiver forms (#348)

Bare/this/Rec self-forms are by-design out of scope: PC0030/PC0031 tracks local variables only, and Rec/this/bare are object-scope globals that are never entered into the name-keyed tracking map. No realistic use case for `SetLoadFields` on the object's own record from inside a table's methods (maintainer decision, #348 — documentation only, no fixtures). Known theoretical edge: `this.FindSet()` next to a local variable named like the table shares a key in the name-keyed tracking map; not fixed, no real-world occurrence found.

## Architecture

### Registration strategy
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ The insertion target must be an element of a statement list (`BlockSyntax` or `R

## Known limitations

- **Bare/this/Rec self-forms: by-design no diagnostic (#348)**: Self-reference forms inside a table call CalcFields on the object's own record, which is always the loop variable in that context. No realistic use case warrants a separate fixture; the existing variable-based fixtures cover the analysis path.
- Fixtures using the `this` self-reference keyword must be gated with `SkipTestIfVersionIsTooLow("14.0")` (runtime 14.0, BC 2024 wave 2).

- Cross-method CalcFields calls (passed record variable) are not detected
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ Detects direct field assignments on non-temporary record variables and recommend

- `IRecordTypeSymbol.Temporary` is keyword-only (SDK `RecordTypeSymbol` ctor; `Rec` in a table object is hard-coded non-temporary even for `TableType = Temporary`), so the TableType branch must be explicit. `TableType = Temporary` tables are extensible (`SemanticFacts.IsTableTypeExtensible`), so the "extensions may add OnBefore/OnAfterValidate" argument applies to them too.

- **Receiver forms (#348)**: `this.Field := value` correctly fires (detected via `OperationKind.ThisReference`); `Rec.Field := value` correctly fires (Rec is a normal identifier). Bare `"Field" := value` inside a table does NOT fire — it binds to `ITableTypeSymbol` (not `IRecordTypeSymbol`), a known limitation pinned by `NoDiagnostic/InsideOnValidateTrigger.al`. Page bare references use implicit-with and resolve to `Rec`, so they do fire.
- CompoundAssignmentStatement OperationKind does not exist in netstandard2.1 SDK. Guarded with `!= default` check.
- The CodeFix does not handle compound assignments (`+=`, `-=`) — only simple `:=` is auto-fixable.
- `this`/self detection uses the `OperationKind.ThisReference` enum (via `EnumProvider`, guarded `!= default`), **not** the `IInstanceReferenceOperation` type. That type is absent from the netstandard2.1 compile floor (AL 12.0.13), and referencing it would force an `#if !NETSTANDARD2_1` guard that silently drops `this.` suppression on the netstandard2.1 binary serving AL 14.0–15.2. The enum approach works on every TFM with no guard. See AC0032 / PR #353.
Expand Down
Loading