diff --git a/.claude/rules/diagnostics/lc0031-record-instance-isolation-level.md b/.claude/rules/diagnostics/lc0031-record-instance-isolation-level.md new file mode 100644 index 00000000..d17aaaeb --- /dev/null +++ b/.claude/rules/diagnostics/lc0031-record-instance-isolation-level.md @@ -0,0 +1,72 @@ +--- +paths: + - "src/ALCops.LinterCop/**/RecordInstanceIsolationLevel*" + - "src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/**" +--- + +# LC0031: RecordInstanceIsolationLevel + +## Purpose + +Flags `LockTable()` on `Record` and `RecordRef` instances and suggests `ReadIsolation(IsolationLevel::UpdLock)`. `LockTable()` sets transaction-wide table state: every subsequent read of that table, on any variable, uses UPDLOCK until commit, and tri-state optimistic reads are disabled for it. `ReadIsolation` is local to one record instance. + +Registers `RegisterOperationAction` on `InvocationExpression`; matches built-in methods named `LockTable`. + +**References:** +- [Record instance isolation level](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-read-isolation), [Record.LockTable](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-locktable-method), [Tri-state locking](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-tri-state-locking), [Performance for developers](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/performance/performance-developer) (Microsoft Learn) +- microsoft/BCQuality [prefer-readisolation-over-locktable-for-reads.md](https://github.com/microsoft/BCQuality/blob/main/microsoft/knowledge/performance/prefer-readisolation-over-locktable-for-reads.md) and [do-not-locktable-in-read-only-procedure.md](https://github.com/microsoft/BCQuality/blob/main/microsoft/knowledge/performance/do-not-locktable-in-read-only-procedure.md) +- [#530](https://github.com/ALCops/Analyzers/issues/530) (LockTable in table triggers), [#545](https://github.com/ALCops/Analyzers/issues/545) (receiver-form audit) + +## Design decisions + +| Decision | Rationale | +|---|---| +| Severity Info, category Design | The call is legal and not deprecated; the replacement narrows lock scope, so it is a suggestion, not a defect | +| Version gate `Spring2023OrGreater` (runtime 11.0) | `ReadIsolation` is a runtime 11.0 built-in (BC22) | +| Self `LockTable()` in table and tableextension triggers is reported on purpose (#530) | The compiler binds bare, `Rec.`, `this.` and named-variable receivers to one `TableClassTypeSymbol` built-in with no self-receiver special case, and the transaction-wide effect is identical inside a trigger. Microsoft's own new code does not write it: W1 app corpus trigger `LockTable()` calls went 80 to 78 from BC 23.5 to 28.4 (two removals, zero additions); Business Foundation, E-Document Core, Subscription Billing and Excise Taxes contain no `LockTable` at all; no Microsoft guidance or community source treats triggers as a special case. The Base Application is legacy here, not the oracle | +| Built-in matched by name only, so `RecordRef.LockTable()` is reported too | `RecordRef` has both `LockTable` and `ReadIsolation` with the same scope difference | +| No dead-call detection | Whether a following read depends on the lock is a judgement the developer makes; the alcops.dev page explains convert versus delete | + +### Receiver-form verdicts (#545) + +| Form | Analyzer | CodeFix | +|---|---|---| +| Named variable | ok, pinned | ok, pinned | +| `Rec.` (table, tableextension, page, `TableNo` OnRun) | ok, pinned | ok, pinned (table); same path elsewhere, not pinned | +| Bare self in table and tableextension | ok, pinned | fixed (#530), pinned | +| Bare self on page and in `TableNo` OnRun | ok, pinned | `IdentifierNameSyntax` path; page pinned, OnRun not pinned | +| `this.` (table, tableextension) | ok, pinned | ok, pinned (table); tableextension same path, not pinned | +| Namespaced fully qualified variable | ok, pinned | same path as named variable, not pinned | +| `RecordRef` variable | ok, pinned | ok, pinned | +| `LockTable(true)` (arguments) | ok, pinned | arguments dropped, see Known issues | + +## Deliberate non-reports + +- `ReadIsolation` itself, in method and property form: the binder rewrites `ReadIsolation := X` into the same `BoundCall` as `ReadIsolation(X)`, and the rule matches the method name only. +- Obsolete code (`IsObsolete()`). + +## Known issues + +- The fix drops `LockTable(Wait, VersionCheck)` arguments because `ReadIsolation` has no equivalent. Accepted at Info severity; the developer reviews the edit. +- The fix always converts, even when the call is dead. A Remove action was considered and rejected: deleting is a second semantics-changing edit the developer has to judge anyway, and the docs page carries the choice. + +## SDK facts + +- `TableClassTypeSymbol` declares `LockTable(Wait?, VersionCheck?)` with no version gate and no deprecation, while the same class deprecates `FindSet(ForUpdate, UpdateKey)` with a message; the absence is deliberate. `ReadIsolation` is a property-style built-in (`isProperty: true`) gated on runtime 11.0. `RecordRefClassTypeSymbol` mirrors both (verified against SDK 18.0.41 in `../nav-sdk-source`). +- `Rec` inside a table is synthesized as a plain record of the table's own type (`TableObjectMembers`); all receiver forms bind to one singleton built-in symbol. `Binder.BindAssignmentStatement` rewrites the property form of `ReadIsolation` into the one-argument call. + +## Test notes + +- `this` fixtures are gated on runtime 14.0 with `SkipTestIfVersionIsTooLow` in both `HasDiagnostic` and `HasFix`. +- Tableextension fixtures are gated on SDK 13.0: older compilers reject a tableextension whose target is declared in the same module (AL0334). +- `HasFix/BareSelfWithLeadingComment` pins trivia preservation; it fails when `WithTriviaFrom` is removed from the fix. + +## CodeFix: RecordInstanceIsolationLevelCodeFixProvider + +| Decision | Rationale | +|---|---| +| Single Replace action | Converting never widens locking; deletion is documented, not automated (Known issues) | +| Keep the author's receiver form: `memberAccess.Expression` reused verbatim, bare stays bare | A `Rec.` prefix on the bare form was rejected; `this.` needs no `ThisExpressionSyntax` reference this way (`netstandard21-compatibility.md`) | +| `WithTriviaFrom(invocationExpression)` on the replacement | A fresh `SyntaxFactory` identifier carries only elastic trivia, so the bare form would lose the indentation and comments attached to the `LockTable` token; the member-access form kept them only because the receiver node was reused | +| Any other expression shape returns the unchanged document | Never throw from a fix | +| `WellKnownFixAllProviders.BatchFixer` | Each diagnostic replaces its own invocation node; no shared ancestor | diff --git a/.claude/skills/fix-false-positive/references/regression-catalog.md b/.claude/skills/fix-false-positive/references/regression-catalog.md index 64393e52..8d323f14 100644 --- a/.claude/skills/fix-false-positive/references/regression-catalog.md +++ b/.claude/skills/fix-false-positive/references/regression-catalog.md @@ -19,7 +19,7 @@ Recurring causes of false positives/negatives, mined from `fix(...)` commits. Wh | **Non-record DB access types (`DataTransfer`)** | The table is an argument, not the receiver: `SetTables(Database::X, Database::Y)` names the tables and `CopyFields`/`CopyRows` executes. Receiver-keyed maps (`MethodOperationMap`, record variable maps) see nothing, so the access is invisible to both permission rules. Resolve from the `SetTables` that reaches the executor in flow order (strict reset, branch union); bail out when unresolvable or none reaches it. | AC0031/AC0032 #465 | | **Built-in method names are not identities** | `MethodKind.BuiltInMethod` plus a method name can match a future built-in on the wrong class. Anchor semantic classification to the exact containing built-in class and method pair; use receiver `NavTypeKind` only for invalid editor-time bindings. | PC0038 #468 | | **Flow-analysis operation wrappers and bypasses** | Parenthesized expressions must be unwrapped before applying short-circuit rules; `break` is a loop exit rather than body fallthrough; and enum exhaustiveness must use the compiler's complete enum-value helper because public enum value lists omit enum-extension values. Use `OperationKind` plus reflective operands for operation interfaces that differ by target framework. | PC0038 #471 | -| **Record fields, record methods and user procedures reach their receiver in four forms** | The receiver may be a named variable (`MyTable.M()`, `MyTable.F`), the implicit `Rec`, bare implicit self (`M()`, `F`), or `this`. Instance-null gates skip bare self inside tables and tableextensions; name-keyed maps mis-key `this` (table name instead of "Rec") and bare (null). In tableextensions all self forms redirect to the target table. Resolve with `GetReceiverTableType`; fixture set in `testing.md` rule 6. | AC0032 #343, batch #348, PC0029 #544 | +| **Record fields, record methods and user procedures reach their receiver in four forms** | The receiver may be a named variable (`MyTable.M()`, `MyTable.F`), the implicit `Rec`, bare implicit self (`M()`, `F`), or `this`. Instance-null gates skip bare self inside tables and tableextensions; name-keyed maps mis-key `this` (table name instead of "Rec") and bare (null). In tableextensions all self forms redirect to the target table. CodeFixes share the gap: a fix that requires `MemberAccessExpressionSyntax` silently returns the unchanged document on the bare form. Resolve with `GetReceiverTableType`; fixture set in `testing.md` rule 6. | AC0032 #343, batch #348, PC0029 #544, LC0031 #530 | | **`Rec` has several origins** | Tables and tableextensions: a synthesized global marked as the object's own instance, bare access binds with a null instance. Pages, page extensions, request pages, reports and xmlports: a synthesized global for the (request page's) `SourceTable`, bare access is rewritten to an explicit `Rec` receiver. A codeunit with `TableNo`: `Rec` is a synthesized **local** of `trigger OnRun` only, no `xRec`, `this` is the codeunit. Report and query dataitems: no `Rec`, the instance is a dataitem access. A `SymbolKind.GlobalVariable` test or a non-null instance gate misses some of these. | PC0029 #544 | | **Table shape: implicit primary key** | A table without a `keys` section has a synthesized primary key over its first valid field. `ITableTypeSymbol.Keys` never lists it (also for referenced `.app` tables); only `PrimaryKey` does. Key-membership logic must read `PrimaryKey`, and every rule reading keys needs a fixture without a `keys` section. | PC0029 #544 | | **One `SymbolKind` covers several AL constructs** | `SymbolKind.Action` spans `area`, `group`, `action`, `separator`, `actionref`, `customaction`, `systemaction` and `fileuploadaction`; `SymbolKind.Control` spans `area`, `group`, `field`, `part` and the rest. Read `IActionSymbol.ActionKind` / `IControlSymbol.ControlKind` before treating a name, caption or property as developer-chosen. | LC0092 #537, AC0011 | diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/BareSelfInProcedure.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/BareSelfInProcedure.al new file mode 100644 index 00000000..373dff4f --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/BareSelfInProcedure.al @@ -0,0 +1,12 @@ +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } + + procedure MyProcedure() + begin + [|LockTable();|] + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/BareSelfInTableExtension.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/BareSelfInTableExtension.al new file mode 100644 index 00000000..f74f14cc --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/BareSelfInTableExtension.al @@ -0,0 +1,15 @@ +tableextension 50001 MyTableExtension extends MyTable +{ + procedure MyProcedure() + begin + [|LockTable();|] + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/BareSelfInTrigger.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/BareSelfInTrigger.al new file mode 100644 index 00000000..e26db066 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/BareSelfInTrigger.al @@ -0,0 +1,12 @@ +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } + + trigger OnInsert() + begin + [|LockTable();|] + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/LockTableWithArguments.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/LockTableWithArguments.al new file mode 100644 index 00000000..4e0f009d --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/LockTableWithArguments.al @@ -0,0 +1,12 @@ +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } + + procedure MyProcedure() + begin + [|Rec.LockTable(true);|] + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/NamedVariable.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/NamedVariable.al new file mode 100644 index 00000000..98f62461 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/NamedVariable.al @@ -0,0 +1,17 @@ +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + MyTable: Record MyTable; + begin + [|MyTable.LockTable();|] + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/NamespacedQualifiedRecordVariable.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/NamespacedQualifiedRecordVariable.al new file mode 100644 index 00000000..ee960177 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/NamespacedQualifiedRecordVariable.al @@ -0,0 +1,19 @@ +namespace MyPublisher.MyExtension.MyAppDomain; + +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + MyTable: Record MyPublisher.MyExtension.MyAppDomain.MyTable; + begin + [|MyTable.LockTable();|] + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/OnRunBareSelf.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/OnRunBareSelf.al new file mode 100644 index 00000000..166718df --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/OnRunBareSelf.al @@ -0,0 +1,17 @@ +codeunit 50100 MyCodeunit +{ + TableNo = MyTable; + + trigger OnRun() + begin + [|LockTable();|] + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/OnRunRecSelf.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/OnRunRecSelf.al new file mode 100644 index 00000000..0a34015f --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/OnRunRecSelf.al @@ -0,0 +1,17 @@ +codeunit 50100 MyCodeunit +{ + TableNo = MyTable; + + trigger OnRun() + begin + [|Rec.LockTable();|] + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/PageBareSelf.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/PageBareSelf.al new file mode 100644 index 00000000..0e71e369 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/PageBareSelf.al @@ -0,0 +1,17 @@ +page 50100 MyPage +{ + SourceTable = MyTable; + + trigger OnOpenPage() + begin + [|LockTable();|] + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/PageRecSelf.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/PageRecSelf.al new file mode 100644 index 00000000..0723cccd --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/PageRecSelf.al @@ -0,0 +1,17 @@ +page 50100 MyPage +{ + SourceTable = MyTable; + + trigger OnOpenPage() + begin + [|Rec.LockTable();|] + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/RecSelfInTableExtension.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/RecSelfInTableExtension.al new file mode 100644 index 00000000..9ff85f16 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/RecSelfInTableExtension.al @@ -0,0 +1,15 @@ +tableextension 50001 MyTableExtension extends MyTable +{ + procedure MyProcedure() + begin + [|Rec.LockTable();|] + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/RecSelfInTrigger.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/RecSelfInTrigger.al new file mode 100644 index 00000000..9df7ad59 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/RecSelfInTrigger.al @@ -0,0 +1,12 @@ +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } + + trigger OnDelete() + begin + [|Rec.LockTable();|] + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/RecordRefVariable.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/RecordRefVariable.al new file mode 100644 index 00000000..322a8796 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/RecordRefVariable.al @@ -0,0 +1,9 @@ +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + MyRecordRef: RecordRef; + begin + [|MyRecordRef.LockTable();|] + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/ThisSelfInProcedure.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/ThisSelfInProcedure.al new file mode 100644 index 00000000..b841bd30 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/ThisSelfInProcedure.al @@ -0,0 +1,12 @@ +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } + + procedure MyProcedure() + begin + [|this.LockTable();|] + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/ThisSelfInTableExtension.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/ThisSelfInTableExtension.al new file mode 100644 index 00000000..ab13a4a4 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/ThisSelfInTableExtension.al @@ -0,0 +1,15 @@ +tableextension 50001 MyTableExtension extends MyTable +{ + procedure MyProcedure() + begin + [|this.LockTable();|] + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/ThisSelfInTrigger.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/ThisSelfInTrigger.al new file mode 100644 index 00000000..c3088069 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasDiagnostic/ThisSelfInTrigger.al @@ -0,0 +1,12 @@ +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } + + trigger OnInsert() + begin + [|this.LockTable();|] + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfInTableExtension/current.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfInTableExtension/current.al new file mode 100644 index 00000000..cfca9dab --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfInTableExtension/current.al @@ -0,0 +1,15 @@ +tableextension 50001 MyTableExtension extends MyTable +{ + procedure MyProcedure() + begin + [|LockTable()|]; + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfInTableExtension/expected.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfInTableExtension/expected.al new file mode 100644 index 00000000..d333b11c --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfInTableExtension/expected.al @@ -0,0 +1,15 @@ +tableextension 50001 MyTableExtension extends MyTable +{ + procedure MyProcedure() + begin + ReadIsolation(IsolationLevel::UpdLock); + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfInTrigger/current.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfInTrigger/current.al new file mode 100644 index 00000000..9915843a --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfInTrigger/current.al @@ -0,0 +1,12 @@ +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } + + trigger OnInsert() + begin + [|LockTable()|]; + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfInTrigger/expected.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfInTrigger/expected.al new file mode 100644 index 00000000..5388c430 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfInTrigger/expected.al @@ -0,0 +1,12 @@ +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } + + trigger OnInsert() + begin + ReadIsolation(IsolationLevel::UpdLock); + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfWithLeadingComment/current.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfWithLeadingComment/current.al new file mode 100644 index 00000000..704065a6 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfWithLeadingComment/current.al @@ -0,0 +1,13 @@ +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } + + trigger OnInsert() + begin + // Serialize inserts on this table + [|LockTable()|]; + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfWithLeadingComment/expected.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfWithLeadingComment/expected.al new file mode 100644 index 00000000..40712b8a --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/BareSelfWithLeadingComment/expected.al @@ -0,0 +1,13 @@ +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } + + trigger OnInsert() + begin + // Serialize inserts on this table + ReadIsolation(IsolationLevel::UpdLock); + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/PageBareSelf/current.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/PageBareSelf/current.al new file mode 100644 index 00000000..77f19984 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/PageBareSelf/current.al @@ -0,0 +1,17 @@ +page 50100 MyPage +{ + SourceTable = MyTable; + + trigger OnOpenPage() + begin + [|LockTable()|]; + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/PageBareSelf/expected.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/PageBareSelf/expected.al new file mode 100644 index 00000000..4d4b0954 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/PageBareSelf/expected.al @@ -0,0 +1,17 @@ +page 50100 MyPage +{ + SourceTable = MyTable; + + trigger OnOpenPage() + begin + ReadIsolation(IsolationLevel::UpdLock); + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/RecordRefVariable/current.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/RecordRefVariable/current.al new file mode 100644 index 00000000..f618c3b0 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/RecordRefVariable/current.al @@ -0,0 +1,9 @@ +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + MyRecordRef: RecordRef; + begin + [|MyRecordRef.LockTable()|]; + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/RecordRefVariable/expected.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/RecordRefVariable/expected.al new file mode 100644 index 00000000..10144cf6 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/RecordRefVariable/expected.al @@ -0,0 +1,9 @@ +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + MyRecordRef: RecordRef; + begin + MyRecordRef.ReadIsolation(IsolationLevel::UpdLock); + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/ThisSelf/current.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/ThisSelf/current.al new file mode 100644 index 00000000..a5ec3bc7 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/ThisSelf/current.al @@ -0,0 +1,12 @@ +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } + + procedure MyProcedure() + begin + [|this.LockTable()|]; + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/ThisSelf/expected.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/ThisSelf/expected.al new file mode 100644 index 00000000..335337db --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/HasFix/ThisSelf/expected.al @@ -0,0 +1,12 @@ +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } + + procedure MyProcedure() + begin + this.ReadIsolation(IsolationLevel::UpdLock); + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/NoDiagnostic/ObsoleteProcedure.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/NoDiagnostic/ObsoleteProcedure.al new file mode 100644 index 00000000..7de2f808 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/NoDiagnostic/ObsoleteProcedure.al @@ -0,0 +1,13 @@ +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } + + [Obsolete('Replaced by ReadIsolation.', '25.0')] + procedure MyProcedure() + begin + [|Rec.LockTable();|] + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/NoDiagnostic/ReadIsolationMethodForm.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/NoDiagnostic/ReadIsolationMethodForm.al new file mode 100644 index 00000000..86bc9422 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/NoDiagnostic/ReadIsolationMethodForm.al @@ -0,0 +1,12 @@ +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } + + procedure MyProcedure() + begin + [|Rec.ReadIsolation(IsolationLevel::UpdLock);|] + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/NoDiagnostic/ReadIsolationPropertyForm.al b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/NoDiagnostic/ReadIsolationPropertyForm.al new file mode 100644 index 00000000..33a6ae88 --- /dev/null +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/NoDiagnostic/ReadIsolationPropertyForm.al @@ -0,0 +1,12 @@ +table 50100 MyTable +{ + fields + { + field(1; MyField; Integer) { } + } + + procedure MyProcedure() + begin + [|ReadIsolation := IsolationLevel::UpdLock;|] + end; +} diff --git a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/RecordInstanceIsolationLevel.cs b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/RecordInstanceIsolationLevel.cs index 41090241..c2cf32dc 100644 --- a/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/RecordInstanceIsolationLevel.cs +++ b/src/ALCops.LinterCop.Test/Rules/RecordInstanceIsolationLevel/RecordInstanceIsolationLevel.cs @@ -22,28 +22,77 @@ public void Setup() [Test] [TestCase("LockTable")] + [TestCase("NamedVariable")] + [TestCase("BareSelfInProcedure")] + [TestCase("BareSelfInTrigger")] + [TestCase("RecSelfInTrigger")] + [TestCase("ThisSelfInProcedure")] + [TestCase("ThisSelfInTrigger")] + [TestCase("BareSelfInTableExtension")] + [TestCase("RecSelfInTableExtension")] + [TestCase("ThisSelfInTableExtension")] + [TestCase("PageRecSelf")] + [TestCase("PageBareSelf")] + [TestCase("OnRunRecSelf")] + [TestCase("OnRunBareSelf")] + [TestCase("NamespacedQualifiedRecordVariable")] + [TestCase("RecordRefVariable")] + [TestCase("LockTableWithArguments")] public async Task HasDiagnostic(string testCase) { + SkipTestIfVersionIsTooLow( + ["ThisSelfInProcedure", "ThisSelfInTrigger", "ThisSelfInTableExtension"], + testCase, + "14.0", + "The 'this' self-reference keyword requires runtime version 14.0 (BC 2024 wave 2)."); + + SkipTestIfVersionIsTooLow( + ["BareSelfInTableExtension", "RecSelfInTableExtension"], + testCase, + "13.0", + "No support for tableextensions when target itself is already declared in the same module"); + var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(HasDiagnostic), $"{testCase}.al")) .ConfigureAwait(false); _fixture.HasDiagnosticAtAllMarkers(code, DiagnosticIds.RecordInstanceIsolationLevel); } - // [Test] - // public async Task NoDiagnostic(string testCase) - // { - // var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(NoDiagnostic), $"{testCase}.al")) - // .ConfigureAwait(false); + [Test] + [TestCase("ObsoleteProcedure")] + [TestCase("ReadIsolationMethodForm")] + [TestCase("ReadIsolationPropertyForm")] + public async Task NoDiagnostic(string testCase) + { + var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(NoDiagnostic), $"{testCase}.al")) + .ConfigureAwait(false); - // _fixture.NoDiagnosticAtAllMarkers(code, DiagnosticIds.RecordInstanceIsolationLevel); - // } + _fixture.NoDiagnosticAtAllMarkers(code, DiagnosticIds.RecordInstanceIsolationLevel); + } [Test] [TestCase("ReplaceLockTableWithReadIsolation")] [TestCase("ReplaceLockTableWithReadIsolationUsingRec")] + [TestCase("BareSelfInTrigger")] + [TestCase("BareSelfInTableExtension")] + [TestCase("ThisSelf")] + [TestCase("RecordRefVariable")] + [TestCase("PageBareSelf")] + [TestCase("BareSelfWithLeadingComment")] public async Task HasFix(string testCase) { + SkipTestIfVersionIsTooLow( + ["ThisSelf"], + testCase, + "14.0", + "The 'this' self-reference keyword requires runtime version 14.0 (BC 2024 wave 2)."); + + SkipTestIfVersionIsTooLow( + ["BareSelfInTableExtension"], + testCase, + "13.0", + "No support for tableextensions when target itself is already declared in the same module"); + var currentCode = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(HasFix), testCase, "current.al")) .ConfigureAwait(false); @@ -59,4 +108,4 @@ public async Task HasFix(string testCase) fixture.TestCodeFix(currentCode, expectedCode, DiagnosticDescriptors.RecordInstanceIsolationLevel); } } -} \ No newline at end of file +} diff --git a/src/ALCops.LinterCop/CodeFixes/RecordInstanceIsolationLevel.cs b/src/ALCops.LinterCop/CodeFixes/RecordInstanceIsolationLevel.cs index 7be3284f..db97c290 100644 --- a/src/ALCops.LinterCop/CodeFixes/RecordInstanceIsolationLevel.cs +++ b/src/ALCops.LinterCop/CodeFixes/RecordInstanceIsolationLevel.cs @@ -69,8 +69,22 @@ private static async Task ReplaceLockTableWithReadIsolation(Document d if (node is not InvocationExpressionSyntax invocationExpression) return document; - if (invocationExpression.Expression is not MemberAccessExpressionSyntax memberAccess) - return document; + CodeExpressionSyntax target; + switch (invocationExpression.Expression) + { + case MemberAccessExpressionSyntax memberAccess: + target = SyntaxFactory.MemberAccessExpression( + memberAccess.Expression, + SyntaxFactory.Token(EnumProvider.SyntaxKind.DotToken), + SyntaxFactory.IdentifierName(ReadIsolationMethodName)); + break; + case IdentifierNameSyntax: + // A bare LockTable() inside a table, tableextension or page keeps its bare form. + target = SyntaxFactory.IdentifierName(ReadIsolationMethodName); + break; + default: + return document; + } var enumMemberAccess = SyntaxFactory.OptionAccessExpression( SyntaxFactory.IdentifierName(IsolationLevelEnumName), @@ -80,12 +94,9 @@ private static async Task ReplaceLockTableWithReadIsolation(Document d var argumentList = SyntaxFactory.ArgumentList( new SeparatedSyntaxList().Add(enumMemberAccess)); - var newMemberAccess = SyntaxFactory.MemberAccessExpression( - memberAccess.Expression, - SyntaxFactory.Token(EnumProvider.SyntaxKind.DotToken), - SyntaxFactory.IdentifierName(ReadIsolationMethodName)); - - var newInvocation = SyntaxFactory.InvocationExpression(newMemberAccess, argumentList); + // A fresh identifier carries no trivia, so copy the indentation and comments of the original call. + var newInvocation = SyntaxFactory.InvocationExpression(target, argumentList) + .WithTriviaFrom(invocationExpression); var root = await syntaxRootTask.ConfigureAwait(false); if (root is null)