From 5d1cdc815c589b9c74b1dd71c16ab6e52da37b22 Mon Sep 17 00:00:00 2001 From: Arthur van de Vondervoort Date: Thu, 18 Jun 2026 11:23:46 +0200 Subject: [PATCH 1/4] fix(AC0032): detect self-access via `this` keyword and bare implicit self MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AC0032 (unused permissions) resolves table data access at the syntax level. It recognised variable receivers (`MyTable.Modify()`, `Rec.Modify()`) but missed two self-access forms: - `this.Modify()` — the AL `this` self-reference keyword (BC 2024 wave 2) - `Modify()` — bare implicit self Both made a table's self-permission appear unused, producing a false positive (issue #343). Root cause: the `this` receiver is a `ThisExpressionSyntax` (neither the identifier fast path nor the `GetSymbolInfo` fallback resolved it), and the implicit-self branch checked `containingObject is IRecordTypeSymbol` — always false, since a table object's symbol is an `ITableTypeSymbol`, not a record. Changes: - Resolve `this` via `SemanticModel.GetOperation(thisNode)?.Type` (binds to the record type); guarded with `#if !NETSTANDARD2_1` (type absent in that SDK). - Resolve bare implicit self from the containing `ITableTypeSymbol`. - New `TryGetSelfPermission` accepts either a record type or a table type. - Tests: AC0032 `ThisKeywordSelfAccess`, `ImplicitSelfBareCall` (NoDiagnostic), `ThisKeywordPartialUnused` (HasDiagnostic); AC0031 future-proof `ThisKeywordSelfCallInTable` (HasDiagnostic). `this` cases guarded at runtime 14.0. - Docs: AC0031/AC0032 instruction files, analyzer-development guide (four receiver forms), netstandard21-compatibility guide (`ThisExpressionSyntax`). - Follow-up audit tracked in #348. Fixes #343 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ...ccess-requires-permissions.instructions.md | 2 +- ...-access-unused-permissions.instructions.md | 11 ++-- .../analyzer-development.instructions.md | 22 +++++++- ...etstandard21-compatibility.instructions.md | 1 + .../ThisKeywordSelfCallInTable.al | 18 +++++++ .../TableDataAccessRequiresPermissions.cs | 7 +++ .../HasDiagnostic/ThisKeywordPartialUnused.al | 19 +++++++ .../NoDiagnostic/ImplicitSelfBareCall.al | 20 +++++++ .../NoDiagnostic/ThisKeywordSelfAccess.al | 20 +++++++ .../TableDataAccessUnusedPermissions.cs | 15 ++++++ .../TableDataAccessUnusedPermissions.cs | 52 +++++++++++++++---- 11 files changed, 172 insertions(+), 15 deletions(-) create mode 100644 src/ALCops.ApplicationCop.Test/Rules/TableDataAccessRequiresPermissions/HasDiagnostic/ThisKeywordSelfCallInTable.al create mode 100644 src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/HasDiagnostic/ThisKeywordPartialUnused.al create mode 100644 src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/NoDiagnostic/ImplicitSelfBareCall.al create mode 100644 src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/NoDiagnostic/ThisKeywordSelfAccess.al diff --git a/.github/instructions/ac0031-table-data-access-requires-permissions.instructions.md b/.github/instructions/ac0031-table-data-access-requires-permissions.instructions.md index 9b0440bb..f45d1f38 100644 --- a/.github/instructions/ac0031-table-data-access-requires-permissions.instructions.md +++ b/.github/instructions/ac0031-table-data-access-requires-permissions.instructions.md @@ -117,7 +117,7 @@ Uses C#-like namespace resolution (`PermissionTableNameResolver`): ## Test coverage -**HasDiagnostic (8 cases):** ProcedureCalls, ProcedureCallsExtended, GetBySystemId, Count, ImplicitSelfCallInTable, XmlPorts, Queries, Reports. +**HasDiagnostic (10 cases):** ProcedureCalls, ProcedureCallsExtended, GetBySystemId, Count, ImplicitSelfCallInTable, ThisKeywordSelfCallInTable, XmlPorts, Queries, Reports, DottedTableName. **NoDiagnostic (21 cases):** ProcedureCallsPermissionsProperty, ProcedureCallsPermissionsPropertyFullyQualified, ProcedureCallsInherentPermissionsProperty, ProcedureCallsInherentPermissionsAttribute, PageSourceTable, PageExtensionSourceTable, XmlPortPermissionsProperty, XmlPortInherentPermissions, QueryPermissionsProperty, QueryInherentPermissions, ReportPermissionsProperty, ReportInherentPermissions, XMLPortWithTableElementProps, PermissionsAsObjectId, PermissionPropertyWithPragma, PermissionPropertyWithComment, MultiplePermissionsDifferentType, TestPermissionsDisabled, GetBySystemIdWithPermissions, CountWithPermissions, ImplicitSelfCallWithInherentPermissions. **HasFix (9 cases):** AddNewPermissionsProperty, AddNewTableEntry, MergePermissionChar, MergeCanonicalOrder, AddEntryMultiLine, AddEntrySingleLine, AddEntryAlphabetical, AddEntryAppend, AddEntryAlphabeticalFirst. diff --git a/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md b/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md index a5e5a75e..9c4f391c 100644 --- a/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md +++ b/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md @@ -66,7 +66,8 @@ src/ALCops.Common/ |---|---| | `AnalyzeApplicationObject` | Entry point; checks Permissions, orchestrates collection and reporting | | `CollectFromInvocations` | Builds object-scope record map (vars + data items), walks method bodies, resolves DB calls via unified handler | -| `TryGetPermissionFromDbAccess` | Unified resolution for both syntax forms (with/without parentheses): pattern-matches to extract method name + receiver, uses variable-map fast path, falls back to `TryGetPermissionViaSymbolInfo` | +| `TryGetPermissionFromDbAccess` | Unified resolution for both syntax forms (with/without parentheses). Handles all four receiver forms: `MyTable.Modify()` / `Rec.Modify()` (variable-map fast path), `Modify()` (bare implicit self), `this.Modify()` (AL `this` self-reference). Falls back to `TryGetPermissionViaSymbolInfo` for complex receivers | +| `TryGetSelfPermission` | Builds a self-access permission from a record type (resolved to its backing table) or a table type directly; returns null for non-record/table self types (e.g. `this` inside a codeunit) | | `TryGetPermissionViaSymbolInfo` | Fallback for complex receivers: uses GetSymbolInfo on the node and receiver expression to resolve method and receiver type | | `CollectFromDataItems` | Iterates report/query FlattenedDataItems and xmlport FlattenedXmlPortNodes (all via reflection) for implicit read permissions | | `AddXmlPortNodeToVarMap` | Adds an xmlport table element to the object-scope record map if it references a non-temporary table | @@ -88,7 +89,9 @@ Each `SyntaxNodeAction` callback is self-contained with no shared mutable state. | Data items in object-scope record map | Report data items and xmlport table elements act as implicit record variables in trigger code; added to the same map for fast-path resolution via `GetTypeSymbol()` | | XmlPort nested nodes via `GetFlattenedXmlPortNodes` | `GetMembers()` returns only top-level schema nodes; `IXmlPortNodeSymbol.FlattenedNodes` only returns immediate children (not recursive). Uses reflection on the internal `SourceXmlPortTypeSymbol.FlattenedNodes` property which truly flattens all depths | | `GetSymbolInfo` fallback for complex receivers | Handles function return values, array indexing, property access; only ~1% of invocations need this path | -| No `GetOperation` / `OperationWalker` | `GetOperation` in `SyntaxNodeAction` costs ~0.3ms/call (no pre-computed cache); variable map approach is 4.5x faster | +| No `GetOperation` / `OperationWalker` | `GetOperation` in `SyntaxNodeAction` costs ~0.3ms/call (no pre-computed cache); variable map approach is 4.5x faster. The only exception is the `this` self-reference, where a single `GetOperation` call on the `this` node resolves the record type (rare; no syntactic shortcut available) | +| Four receiver forms handled explicitly | Database access can target a record via four syntactic forms: `MyTable.Modify()`, `Rec.Modify()`, bare `Modify()` (implicit self), and `this.Modify()` (AL `this` keyword, `ThisExpressionSyntax`). Variable receivers use the fast path; bare self resolves the containing `ITableTypeSymbol`; `this` resolves via the operation's type. Missing the `this` form caused the AC0032 false positive in issue #343 | +| Self-table symbol shapes | The table object's declared symbol is an `ITableTypeSymbol` (not an `IRecordTypeSymbol`); `this`/`Rec` resolve to a separate `IRecordTypeSymbol` wrapper. `TryGetSelfPermission` accepts both: record types via `OriginalDefinition`, table types directly | | No cross-callback shared state | Eliminates the fragile two-phase accumulator pattern that caused false positives during incremental compilation | | Iterate `DescendantNodes()` for methods | Finds all MethodOrTriggerDeclarationSyntax in the object, skip obsolete ones | | Skip obsolete methods via symbol | `GetDeclaredSymbol` + `IsObsolete()` on the method symbol | @@ -139,8 +142,8 @@ When removing the first entry from a multi-entry list, `SeparatedSyntaxList.Remo ## Test coverage -**HasDiagnostic (10 cases):** EntireEntryUnused, PartialCharsUnused, MultipleUnusedEntries, NoCodeInCodeunit, UnusedOnReport, UnusedOnQuery, UnusedOnXmlPort, TemporaryRecord, ParameterPartialUnused, ReportDataItemPartialUnused. -**NoDiagnostic (25 cases):** AllPermissionsUsed, PageSourceTable, TestCodeunitDisabled, ReadUsed, ReportDataItemRead, QueryDataItemRead, PermissionSet, PermissionSetExtension, SystemTable, ParameterOperations, UppercasePermissions, ParameterAllOperations, LocalVarSpacedTable, GlobalVarSpacedTable, ReportDataItemModify, ReportDataItemAliasModify, XmlPortTableElementModify, XmlPortNestedTableElementModify, ReturnParameterRead, ReportNestedDataItemRead, QueryNestedDataItemRead, MethodWithoutParenthesesCount, MethodWithoutParenthesesFindFirst, MethodWithoutParenthesesIsEmpty, MethodWithoutParenthesesChained. +**HasDiagnostic (11 cases):** EntireEntryUnused, PartialCharsUnused, MultipleUnusedEntries, NoCodeInCodeunit, UnusedOnReport, UnusedOnQuery, UnusedOnXmlPort, TemporaryRecord, ParameterPartialUnused, ReportDataItemPartialUnused, ThisKeywordPartialUnused. +**NoDiagnostic (27 cases):** AllPermissionsUsed, PageSourceTable, TestCodeunitDisabled, ReadUsed, ReportDataItemRead, QueryDataItemRead, PermissionSet, PermissionSetExtension, SystemTable, ParameterOperations, UppercasePermissions, ParameterAllOperations, LocalVarSpacedTable, GlobalVarSpacedTable, ReportDataItemModify, ReportDataItemAliasModify, XmlPortTableElementModify, XmlPortNestedTableElementModify, ReturnParameterRead, ReportNestedDataItemRead, QueryNestedDataItemRead, MethodWithoutParenthesesCount, MethodWithoutParenthesesFindFirst, MethodWithoutParenthesesIsEmpty, MethodWithoutParenthesesChained, ThisKeywordSelfAccess, ImplicitSelfBareCall. **HasFix (4 cases):** RemoveEntireEntry, ReduceChars, RemoveEntireProperty, ReplaceChars. ## Known limitations diff --git a/.github/instructions/analyzer-development.instructions.md b/.github/instructions/analyzer-development.instructions.md index 623a4838..d4dcd175 100644 --- a/.github/instructions/analyzer-development.instructions.md +++ b/.github/instructions/analyzer-development.instructions.md @@ -328,8 +328,28 @@ private static void AnalyzeStringLiteral(SyntaxNodeAnalysisContext ctx) } ``` -## EnumProvider (Critical Pattern) +### Record method receiver forms (syntax level) + +When detecting AL record (table data) method calls at the **syntax level**, a call like +`Modify` can reach a record through **four** receiver forms. Analyzers that pattern-match +only some of them produce false positives/negatives (see issue #343, AC0032): + +| Form | Example | Syntax shape | How to resolve the record/table | +|---|---|---|---| +| Named variable | `MyTable.Modify()` | `MemberAccessExpressionSyntax` with `IdentifierNameSyntax` receiver | Variable map (locals/params/globals) or `GetSymbolInfo` on the receiver | +| Implicit `Rec` | `Rec.Modify()` | same as above (`Rec` is a normal identifier) | same as named variable | +| Bare implicit self | `Modify()` | `InvocationExpressionSyntax` whose `Expression` is `IdentifierNameSyntax` (no receiver) | The containing object: a table object's declared symbol is an `ITableTypeSymbol` | +| `this` self-reference | `this.Modify()` | `MemberAccessExpressionSyntax` with a `ThisExpressionSyntax` receiver (AL `this` keyword, runtime 14.0+) | `SemanticModel.GetOperation(thisNode)?.Type` (binds to a `BoundThisReference`/`IInstanceReferenceOperation`) | +Key symbol-shape gotcha: a **table object's declared symbol is `ITableTypeSymbol`, which is +NOT an `IRecordTypeSymbol`**. The record (`Rec`/`this`) is a separate `IRecordTypeSymbol` +wrapper whose `OriginalDefinition` is the `ITableTypeSymbol`. Guard with `is ITableTypeSymbol` +for the object/bare-self and `is IRecordTypeSymbol` for variable/`this` receivers. + +Tests that exercise the `this` form must guard on runtime version 14.0 (e.g. +`SkipTestIfVersionIsTooLow([...], testCase, "14.0", ...)`), since `this` is a Fall 2024 feature. + +## EnumProvider (Critical Pattern) Never reference `Microsoft.Dynamics.Nav.CodeAnalysis` enum values directly. Always use `EnumProvider` from `ALCops.Common.Reflection`. This provides backward compatibility across SDK versions via reflection-based caching. ```csharp diff --git a/.github/instructions/netstandard21-compatibility.instructions.md b/.github/instructions/netstandard21-compatibility.instructions.md index d6f1a70b..b8508e77 100644 --- a/.github/instructions/netstandard21-compatibility.instructions.md +++ b/.github/instructions/netstandard21-compatibility.instructions.md @@ -21,6 +21,7 @@ Some `Microsoft.Dynamics.Nav.CodeAnalysis` APIs only exist in newer SDK versions | API | Available in | netstandard2.1 workaround | |---|---|---| | `IFieldSymbol.Type` | net8.0 only | `fieldSymbol.OriginalDefinition.GetTypeSymbol()` (requires `using Microsoft.Dynamics.Nav.CodeAnalysis.Symbols`) | +| `ThisExpressionSyntax` (AL `this` keyword) | net8.0+ only | Feature is Fall 2024 (runtime 14.0); the type is absent in the Sep-2023 netstandard2.1 SDK. Wrap usages in `#if !NETSTANDARD2_1` (the older SDK can never parse `this`, so skipping its handling is correct). See AC0032 `TableDataAccessUnusedPermissions`. | ### Pattern for `IFieldSymbol.Type` diff --git a/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessRequiresPermissions/HasDiagnostic/ThisKeywordSelfCallInTable.al b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessRequiresPermissions/HasDiagnostic/ThisKeywordSelfCallInTable.al new file mode 100644 index 00000000..e9b0ad4c --- /dev/null +++ b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessRequiresPermissions/HasDiagnostic/ThisKeywordSelfCallInTable.al @@ -0,0 +1,18 @@ +table 50000 MyTable +{ + Caption = '', Locked = true; + + fields + { + field(1; MyField; Integer) + { + Caption = '', Locked = true; + DataClassification = ToBeClassified; + } + } + + procedure DoSomething() + begin + [|this.Modify();|] + end; +} diff --git a/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessRequiresPermissions/TableDataAccessRequiresPermissions.cs b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessRequiresPermissions/TableDataAccessRequiresPermissions.cs index ad5f2d6c..5609be05 100644 --- a/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessRequiresPermissions/TableDataAccessRequiresPermissions.cs +++ b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessRequiresPermissions/TableDataAccessRequiresPermissions.cs @@ -26,12 +26,19 @@ public void Setup() [TestCase("GetBySystemId")] [TestCase("Count")] [TestCase("ImplicitSelfCallInTable")] + [TestCase("ThisKeywordSelfCallInTable")] [TestCase("XmlPorts")] [TestCase("Queries")] [TestCase("Reports")] [TestCase("DottedTableName")] public async Task HasDiagnostic(string testCase) { + SkipTestIfVersionIsTooLow( + ["ThisKeywordSelfCallInTable"], + testCase, + "14.0", + "The 'this' self-reference keyword requires runtime version 14.0 (BC 2024 wave 2)."); + var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(HasDiagnostic), $"{testCase}.al")) .ConfigureAwait(false); diff --git a/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/HasDiagnostic/ThisKeywordPartialUnused.al b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/HasDiagnostic/ThisKeywordPartialUnused.al new file mode 100644 index 00000000..22b54d6b --- /dev/null +++ b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/HasDiagnostic/ThisKeywordPartialUnused.al @@ -0,0 +1,19 @@ +table 50000 MyTable +{ + Permissions = [|tabledata MyTable = rimd|]; + + fields + { + field(1; MyField; Integer) + { + Caption = '', Locked = true; + DataClassification = ToBeClassified; + } + } + + procedure SelfAccess() + begin + this.FindFirst(); + this.Modify(); + end; +} diff --git a/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/NoDiagnostic/ImplicitSelfBareCall.al b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/NoDiagnostic/ImplicitSelfBareCall.al new file mode 100644 index 00000000..6d057880 --- /dev/null +++ b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/NoDiagnostic/ImplicitSelfBareCall.al @@ -0,0 +1,20 @@ +table 50000 MyTable +{ + Permissions = [|tabledata MyTable = rim|]; + + fields + { + field(1; MyField; Integer) + { + Caption = '', Locked = true; + DataClassification = ToBeClassified; + } + } + + procedure SelfAccess() + begin + FindFirst(); + Insert(); + Modify(); + end; +} diff --git a/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/NoDiagnostic/ThisKeywordSelfAccess.al b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/NoDiagnostic/ThisKeywordSelfAccess.al new file mode 100644 index 00000000..297b2a92 --- /dev/null +++ b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/NoDiagnostic/ThisKeywordSelfAccess.al @@ -0,0 +1,20 @@ +table 50000 MyTable +{ + Permissions = [|tabledata MyTable = rim|]; + + fields + { + field(1; MyField; Integer) + { + Caption = '', Locked = true; + DataClassification = ToBeClassified; + } + } + + procedure SelfAccess() + begin + this.FindFirst(); + this.Insert(); + this.Modify(); + end; +} diff --git a/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs index 0faac8d9..be32bbd4 100644 --- a/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs +++ b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs @@ -31,8 +31,15 @@ public void Setup() [TestCase("TemporaryRecord")] [TestCase("ParameterPartialUnused")] [TestCase("ReportDataItemPartialUnused")] + [TestCase("ThisKeywordPartialUnused")] public async Task HasDiagnostic(string testCase) { + SkipTestIfVersionIsTooLow( + ["ThisKeywordPartialUnused"], + testCase, + "14.0", + "The 'this' self-reference keyword requires runtime version 14.0 (BC 2024 wave 2)."); + var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(HasDiagnostic), $"{testCase}.al")) .ConfigureAwait(false); @@ -65,6 +72,8 @@ public async Task HasDiagnostic(string testCase) [TestCase("MethodWithoutParenthesesFindFirst")] [TestCase("MethodWithoutParenthesesIsEmpty")] [TestCase("MethodWithoutParenthesesChained")] + [TestCase("ThisKeywordSelfAccess")] + [TestCase("ImplicitSelfBareCall")] public async Task NoDiagnostic(string testCase) { SkipTestIfVersionIsTooLow( @@ -73,6 +82,12 @@ public async Task NoDiagnostic(string testCase) "13.0", "No support for tableextensions when target itself is already declared in the same module"); + SkipTestIfVersionIsTooLow( + ["ThisKeywordSelfAccess"], + testCase, + "14.0", + "The 'this' self-reference keyword requires runtime version 14.0 (BC 2024 wave 2)."); + var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(NoDiagnostic), $"{testCase}.al")) .ConfigureAwait(false); diff --git a/src/ALCops.ApplicationCop/Analyzers/TableDataAccessUnusedPermissions.cs b/src/ALCops.ApplicationCop/Analyzers/TableDataAccessUnusedPermissions.cs index 4e605edb..512e7d38 100644 --- a/src/ALCops.ApplicationCop/Analyzers/TableDataAccessUnusedPermissions.cs +++ b/src/ALCops.ApplicationCop/Analyzers/TableDataAccessUnusedPermissions.cs @@ -224,17 +224,26 @@ private static void CollectFromInvocations( if (operation == DatabaseOperation.None) return null; - // Implicit self (no receiver, inside table/tableext) - if (hasImplicitSelf) + // `this.Method()` self-reference (AL 'this' keyword, represented as + // ThisExpressionSyntax). In a table/tableextension `this` is the record itself, + // equivalent to a bare `Method()` call. Resolve its type via the semantic model; + // in non-record objects (e.g. codeunits, where `this` is the codeunit instance) + // the type is not a record and is correctly ignored. + // Guarded out on netstandard2.1: the 'this' keyword (Fall 2024 feature) and + // ThisExpressionSyntax do not exist in that older SDK, so it can never be parsed. +#if !NETSTANDARD2_1 + if (receiverExpression is ThisExpressionSyntax thisExpression) { - if (containingObject is IRecordTypeSymbol selfRecord && !selfRecord.Temporary) - { - var tableType = selfRecord.OriginalDefinition as ITableTypeSymbol; - if (tableType is not null) - return new RequiredPermission(tableType, selfRecord, operation, node.GetLocation()); - } - return null; + var thisType = ctx.SemanticModel.GetOperation(thisExpression, ctx.CancellationToken)?.Type; + return TryGetSelfPermission(thisType, operation, node); } +#endif + + // Implicit self: bare `Method()` inside a table. The accessed table is the + // containing object itself (an ITableTypeSymbol). The operation model reports a + // null instance for bare self calls, so resolve directly from the object symbol. + if (hasImplicitSelf) + return TryGetSelfPermission(containingObject as ITypeSymbol, operation, node); // Fast path: resolve receiver via variable map lookup if (receiverExpression is IdentifierNameSyntax identifierName) @@ -264,6 +273,31 @@ private static void CollectFromInvocations( return TryGetPermissionViaSymbolInfo(node, receiverExpression, containingObject, ctx); } + /// + /// Builds a required permission for a self-access (bare `Method()` or `this.Method()`). + /// Accepts either a record type (resolved to its backing table) or a table type directly. + /// Returns null when the self type is not a non-temporary record/table (e.g. inside a + /// codeunit, where `this` is the codeunit instance). + /// + private static RequiredPermission? TryGetSelfPermission( + ITypeSymbol? selfType, + DatabaseOperation operation, + SyntaxNode node) + { + switch (selfType) + { + case IRecordTypeSymbol record when !record.Temporary + && record.OriginalDefinition is ITableTypeSymbol recordTable: + return new RequiredPermission(recordTable, record, operation, node.GetLocation()); + + case ITableTypeSymbol table: + return new RequiredPermission(table, table, operation, node.GetLocation()); + + default: + return null; + } + } + private static RequiredPermission? TryGetPermissionViaSymbolInfo( SyntaxNode node, ExpressionSyntax? receiverExpression, From 7e64079ad2d74b723528e378c94f4c2bc7104c03 Mon Sep 17 00:00:00 2001 From: Arthur van de Vondervoort Date: Thu, 18 Jun 2026 11:41:38 +0200 Subject: [PATCH 2/4] test(AC0032): raise ThisKeywordSelfAccess guard to runtime 16.0 The 'this' keyword parses from runtime 14.0, but the SDK only binds a table's `this` to its backing record type (via GetRecord) from runtime 16.0 (BC 2025 wave 2). On 14.0-15.2, GetOperation(this).Type does not resolve to a record, so `this.Modify()` self-access is not detected and the NoDiagnostic test fails. Bump the ThisKeywordSelfAccess guard from 14.0 to 16.0 and document the limitation in the AC0032 instruction file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ...c0032-table-data-access-unused-permissions.instructions.md | 1 + .../TableDataAccessUnusedPermissions.cs | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md b/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md index 9c4f391c..1521fd0a 100644 --- a/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md +++ b/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md @@ -152,6 +152,7 @@ When removing the first entry from a multi-entry list, `SeparatedSyntaxList.Remo 2. **CalcFields/CalcSums**: Indirect table access through FlowFields is not traced 3. **InherentPermissions overlap**: Table-level `InherentPermissions` may make an object-level entry redundant, but the analyzer does not flag this (different concern from unused) 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) +5. **`this` self-reference before runtime 16.0**: Resolving `this` to the record type inside a table relies on `GetOperation(this).Type` returning the backing record. The SDK only binds table `this` to a record from runtime 16.0 (BC 2025 wave 2). On 14.0-15.2 the `this` keyword parses but does not resolve to a record, so `this.Modify()` self-access is not detected and the unused-permission false positive can persist. The `ThisKeywordSelfAccess` test is guarded to 16.0 for this reason ## Design decisions (continued) diff --git a/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs index be32bbd4..c567ca91 100644 --- a/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs +++ b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs @@ -85,8 +85,8 @@ public async Task NoDiagnostic(string testCase) SkipTestIfVersionIsTooLow( ["ThisKeywordSelfAccess"], testCase, - "14.0", - "The 'this' self-reference keyword requires runtime version 14.0 (BC 2024 wave 2)."); + "16.0", + "Resolving 'this' to the record type inside a table requires runtime version 16.0 (BC 2025 wave 2); on 14.0-15.2 the SDK does not bind table 'this' to a record, so self-access cannot be detected."); var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(NoDiagnostic), $"{testCase}.al")) From 30f55b46d921ae71f6c2fc5f8cf81b1161a1560e Mon Sep 17 00:00:00 2001 From: Arthur van de Vondervoort Date: Thu, 18 Jun 2026 12:01:25 +0200 Subject: [PATCH 3/4] docs(AC0032): correct the reason `this.` self-access only works on AL 16.0+ The previous note attributed the 16.0 boundary to an SDK runtime binding difference. That is wrong: the binder resolves a table's `this` to its record identically on AL 14.0/15.2/16.0 (verified against the tagged NAV SDK source). The real cause is a netstandard2.1 compile-time limitation. The public `ThisExpressionSyntax` type (and `SyntaxKind.ThisExpression` / `IInstanceReferenceOperation`) is absent from the netstandard2.1 build floor (AL 12.0.13); these were added in AL 14.0. The `this` branch is therefore `#if !NETSTANDARD2_1`-excluded with no reflection/operation-tree fallback. AL 14.0-15.2 ship a netstandard2.0 SDK and run ALCops's netstandard2.1 binary, so `this.` self-access is undetected there and the #343 false positive persists on those versions; it is fixed only on the net8.0/net10.0 binaries (AL 16.0+). - Correct the analyzer comment, AC0032 known-limitation, analyzer-development guide, and netstandard21-compatibility guide. - Keep the `ThisKeywordSelfAccess` guard at 16.0 with an accurate reason. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ...-table-data-access-unused-permissions.instructions.md | 2 +- .../instructions/analyzer-development.instructions.md | 9 +++++++++ .../netstandard21-compatibility.instructions.md | 2 +- .../TableDataAccessUnusedPermissions.cs | 2 +- .../Analyzers/TableDataAccessUnusedPermissions.cs | 9 +++++++-- 5 files changed, 19 insertions(+), 5 deletions(-) diff --git a/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md b/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md index 1521fd0a..11f7bd10 100644 --- a/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md +++ b/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md @@ -152,7 +152,7 @@ When removing the first entry from a multi-entry list, `SeparatedSyntaxList.Remo 2. **CalcFields/CalcSums**: Indirect table access through FlowFields is not traced 3. **InherentPermissions overlap**: Table-level `InherentPermissions` may make an object-level entry redundant, but the analyzer does not flag this (different concern from unused) 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) -5. **`this` self-reference before runtime 16.0**: Resolving `this` to the record type inside a table relies on `GetOperation(this).Type` returning the backing record. The SDK only binds table `this` to a record from runtime 16.0 (BC 2025 wave 2). On 14.0-15.2 the `this` keyword parses but does not resolve to a record, so `this.Modify()` self-access is not detected and the unused-permission false positive can persist. The `ThisKeywordSelfAccess` test is guarded to 16.0 for this reason +5. **`this.` self-access on AL 14.0-15.2**: The `this` branch is `#if !NETSTANDARD2_1`-excluded because the public `ThisExpressionSyntax` type (plus `SyntaxKind.ThisExpression` and `IInstanceReferenceOperation`) is absent from the netstandard2.1 compile floor (AL 12.0.13, predating the Fall 2024 `this` feature). AL 14.0-15.2 ship a netstandard2.0 SDK and run ALCops's netstandard2.1 binary, so `this.Modify()` self-access is not detected there and the unused-permission false positive (#343) persists on those versions. It is fixed only on the net8.0/net10.0 binaries (AL 16.0+); the `ThisKeywordSelfAccess` test is guarded to 16.0 for this reason. A reflection-based fallback is not possible (the required SDK members do not exist at the floor) ## Design decisions (continued) diff --git a/.github/instructions/analyzer-development.instructions.md b/.github/instructions/analyzer-development.instructions.md index d4dcd175..573b4404 100644 --- a/.github/instructions/analyzer-development.instructions.md +++ b/.github/instructions/analyzer-development.instructions.md @@ -349,6 +349,15 @@ for the object/bare-self and `is IRecordTypeSymbol` for variable/`this` receiver Tests that exercise the `this` form must guard on runtime version 14.0 (e.g. `SkipTestIfVersionIsTooLow([...], testCase, "14.0", ...)`), since `this` is a Fall 2024 feature. +`ThisExpressionSyntax` requires a `#if !NETSTANDARD2_1` guard: the public type is **absent** +from the netstandard2.1 compile floor (AL 12.0.13, which predates the Fall 2024 `this` feature; +the type, the `SyntaxKind.ThisExpression` enum member, and `IInstanceReferenceOperation` were all +added in AL 14.0). There is therefore no type-free or `EnumProvider`-reflection fallback. The +netstandard2.1 binary serves AL 14.0-15.2 (those ship a netstandard2.0 SDK), so any analyzer that +relies on `ThisExpressionSyntax` cannot detect `this.` on 14.0-15.2 -- only on the net8.0/net10.0 +binaries (AL 16.0+). Guard the corresponding test to 16.0, not 14.0. Operation-tree based detection +(e.g. `IInvocationOperation.Instance`) has the same floor limitation and cannot work around it. + ## EnumProvider (Critical Pattern) Never reference `Microsoft.Dynamics.Nav.CodeAnalysis` enum values directly. Always use `EnumProvider` from `ALCops.Common.Reflection`. This provides backward compatibility across SDK versions via reflection-based caching. diff --git a/.github/instructions/netstandard21-compatibility.instructions.md b/.github/instructions/netstandard21-compatibility.instructions.md index b8508e77..905b1725 100644 --- a/.github/instructions/netstandard21-compatibility.instructions.md +++ b/.github/instructions/netstandard21-compatibility.instructions.md @@ -21,7 +21,7 @@ Some `Microsoft.Dynamics.Nav.CodeAnalysis` APIs only exist in newer SDK versions | API | Available in | netstandard2.1 workaround | |---|---|---| | `IFieldSymbol.Type` | net8.0 only | `fieldSymbol.OriginalDefinition.GetTypeSymbol()` (requires `using Microsoft.Dynamics.Nav.CodeAnalysis.Symbols`) | -| `ThisExpressionSyntax` (AL `this` keyword) | net8.0+ only | Feature is Fall 2024 (runtime 14.0); the type is absent in the Sep-2023 netstandard2.1 SDK. Wrap usages in `#if !NETSTANDARD2_1` (the older SDK can never parse `this`, so skipping its handling is correct). See AC0032 `TableDataAccessUnusedPermissions`. | +| `ThisExpressionSyntax` (AL `this` keyword) | net8.0+ only | The public type is absent from the netstandard2.1 compile floor (AL 12.0.13, predating the Fall 2024 `this` feature; the type was added in AL 14.0). `SyntaxKind.ThisExpression` and `IInstanceReferenceOperation` are likewise absent at the floor, so there is **no** `EnumProvider`/reflection or operation-tree fallback. Wrap usages in `#if !NETSTANDARD2_1`. Note: the netstandard2.1 binary serves AL 14.0-15.2 at runtime, so guarding it out means `this.` detection (and any fix depending on it) is unavailable there -- guard the corresponding tests to **16.0**, not 14.0. See AC0032 `TableDataAccessUnusedPermissions`. | ### Pattern for `IFieldSymbol.Type` diff --git a/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs index c567ca91..f862c429 100644 --- a/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs +++ b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs @@ -86,7 +86,7 @@ public async Task NoDiagnostic(string testCase) ["ThisKeywordSelfAccess"], testCase, "16.0", - "Resolving 'this' to the record type inside a table requires runtime version 16.0 (BC 2025 wave 2); on 14.0-15.2 the SDK does not bind table 'this' to a record, so self-access cannot be detected."); + "this. self-access is only detected by the net8.0/net10.0 analyzer builds; AL 14.0-15.2 ship a netstandard2.0 SDK and run ALCops's netstandard2.1 binary, where ThisExpressionSyntax is absent at the 12.0.13 compile floor and the 'this' branch is excluded."); var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(NoDiagnostic), $"{testCase}.al")) diff --git a/src/ALCops.ApplicationCop/Analyzers/TableDataAccessUnusedPermissions.cs b/src/ALCops.ApplicationCop/Analyzers/TableDataAccessUnusedPermissions.cs index 512e7d38..5e8c6b0e 100644 --- a/src/ALCops.ApplicationCop/Analyzers/TableDataAccessUnusedPermissions.cs +++ b/src/ALCops.ApplicationCop/Analyzers/TableDataAccessUnusedPermissions.cs @@ -229,8 +229,13 @@ private static void CollectFromInvocations( // equivalent to a bare `Method()` call. Resolve its type via the semantic model; // in non-record objects (e.g. codeunits, where `this` is the codeunit instance) // the type is not a record and is correctly ignored. - // Guarded out on netstandard2.1: the 'this' keyword (Fall 2024 feature) and - // ThisExpressionSyntax do not exist in that older SDK, so it can never be parsed. + // + // Guarded out on netstandard2.1: the public ThisExpressionSyntax type is absent + // from the netstandard2.1 compile floor (AL 12.0.13, which predates the Fall 2024 + // 'this' feature; the type was added in AL 14.0), so it cannot be referenced there. + // Consequence: AL 14.0-15.2 ship a netstandard2.0 SDK and therefore run ALCops's + // netstandard2.1 binary, which has no 'this' handling -- the self-access false + // positive (#343) is only fixed on the net8.0/net10.0 binaries (AL 16.0+). #if !NETSTANDARD2_1 if (receiverExpression is ThisExpressionSyntax thisExpression) { From f88aa7be917f0f3ec3108667cf1d0fe8a8b083a0 Mon Sep 17 00:00:00 2001 From: Arthur van de Vondervoort Date: Thu, 18 Jun 2026 12:27:59 +0200 Subject: [PATCH 4/4] fix(AC0032): resolve this. self-access via the operation tree Resolve non-identifier method receivers (the AL `this` self-reference and expression receivers) through the receiver's IOperation.Type instead of pattern-matching ThisExpressionSyntax. This mirrors AC0031 (RequiredPermissionDetector) and never references the ThisExpressionSyntax type, which is absent at the netstandard2.1 compile floor (AL 12.0.13). Because IOperation, SemanticModel.GetOperation and IOperation.Type all exist at that floor, the fix works on every TFM and AL version (14.0+), not just the net8.0/net10.0 builds (AL 16.0+). Removes the prior #if !NETSTANDARD2_1 guard and the associated known-limitation, and reverts the ThisKeywordSelfAccess test guard from 16.0 back to 14.0. Renames TryGetSelfPermission to TryGetPermissionForType to reflect that it now serves any resolved receiver/self type. Updates the AC0032, analyzer-development and netstandard21-compatibility instruction files accordingly. Fixes #343 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ...-access-unused-permissions.instructions.md | 13 +++-- .../analyzer-development.instructions.md | 30 ++++++++---- ...etstandard21-compatibility.instructions.md | 2 +- .../TableDataAccessUnusedPermissions.cs | 4 +- .../TableDataAccessUnusedPermissions.cs | 48 +++++++++---------- 5 files changed, 52 insertions(+), 45 deletions(-) diff --git a/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md b/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md index 11f7bd10..1cce9cd6 100644 --- a/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md +++ b/.github/instructions/ac0032-table-data-access-unused-permissions.instructions.md @@ -53,7 +53,7 @@ src/ALCops.Common/ - For each method body: syntax pre-filter (`HasPossibleDbInvocation`) - Build per-method record variable map from `IMethodSymbol.LocalVariables` + `.Parameters` - Walk method body for both `InvocationExpressionSyntax` and standalone `MemberAccessExpressionSyntax` (method calls without parentheses) - - Unified `TryGetPermissionFromDbAccess` extracts method name + receiver from either form, resolves via variable map (fast path), falls back to `GetSymbolInfo` for complex receivers + - Unified `TryGetPermissionFromDbAccess` extracts method name + receiver from either form, resolves via variable map (fast path), then via the receiver's `IOperation.Type` for non-identifier receivers (`this.`, expression receivers), and falls back to `GetSymbolInfo` for anything unresolved 5. **Collect data items** (`CollectFromDataItems`): - Iterate `containingObject.GetMembers()` for ReportDataItem, QueryDataItem, XmlPortNode symbols - For XmlPortNode: also iterate `FlattenedNodes` to reach nested table elements @@ -66,8 +66,8 @@ src/ALCops.Common/ |---|---| | `AnalyzeApplicationObject` | Entry point; checks Permissions, orchestrates collection and reporting | | `CollectFromInvocations` | Builds object-scope record map (vars + data items), walks method bodies, resolves DB calls via unified handler | -| `TryGetPermissionFromDbAccess` | Unified resolution for both syntax forms (with/without parentheses). Handles all four receiver forms: `MyTable.Modify()` / `Rec.Modify()` (variable-map fast path), `Modify()` (bare implicit self), `this.Modify()` (AL `this` self-reference). Falls back to `TryGetPermissionViaSymbolInfo` for complex receivers | -| `TryGetSelfPermission` | Builds a self-access permission from a record type (resolved to its backing table) or a table type directly; returns null for non-record/table self types (e.g. `this` inside a codeunit) | +| `TryGetPermissionFromDbAccess` | Unified resolution for both syntax forms (with/without parentheses). Handles all four receiver forms: `MyTable.Modify()` / `Rec.Modify()` (variable-map fast path), `Modify()` (bare implicit self), `this.Modify()` (AL `this` self-reference, resolved via `IOperation.Type`). Falls back to `TryGetPermissionViaSymbolInfo` for unresolved receivers | +| `TryGetPermissionForType` | Builds a required permission from a resolved receiver/self type (record resolved to its backing table, or a table type directly); returns null for non-record/table types (e.g. `this` inside a codeunit) | | `TryGetPermissionViaSymbolInfo` | Fallback for complex receivers: uses GetSymbolInfo on the node and receiver expression to resolve method and receiver type | | `CollectFromDataItems` | Iterates report/query FlattenedDataItems and xmlport FlattenedXmlPortNodes (all via reflection) for implicit read permissions | | `AddXmlPortNodeToVarMap` | Adds an xmlport table element to the object-scope record map if it references a non-temporary table | @@ -89,9 +89,9 @@ Each `SyntaxNodeAction` callback is self-contained with no shared mutable state. | Data items in object-scope record map | Report data items and xmlport table elements act as implicit record variables in trigger code; added to the same map for fast-path resolution via `GetTypeSymbol()` | | XmlPort nested nodes via `GetFlattenedXmlPortNodes` | `GetMembers()` returns only top-level schema nodes; `IXmlPortNodeSymbol.FlattenedNodes` only returns immediate children (not recursive). Uses reflection on the internal `SourceXmlPortTypeSymbol.FlattenedNodes` property which truly flattens all depths | | `GetSymbolInfo` fallback for complex receivers | Handles function return values, array indexing, property access; only ~1% of invocations need this path | -| No `GetOperation` / `OperationWalker` | `GetOperation` in `SyntaxNodeAction` costs ~0.3ms/call (no pre-computed cache); variable map approach is 4.5x faster. The only exception is the `this` self-reference, where a single `GetOperation` call on the `this` node resolves the record type (rare; no syntactic shortcut available) | -| Four receiver forms handled explicitly | Database access can target a record via four syntactic forms: `MyTable.Modify()`, `Rec.Modify()`, bare `Modify()` (implicit self), and `this.Modify()` (AL `this` keyword, `ThisExpressionSyntax`). Variable receivers use the fast path; bare self resolves the containing `ITableTypeSymbol`; `this` resolves via the operation's type. Missing the `this` form caused the AC0032 false positive in issue #343 | -| Self-table symbol shapes | The table object's declared symbol is an `ITableTypeSymbol` (not an `IRecordTypeSymbol`); `this`/`Rec` resolve to a separate `IRecordTypeSymbol` wrapper. `TryGetSelfPermission` accepts both: record types via `OriginalDefinition`, table types directly | +| Hot path avoids `GetOperation` / `OperationWalker` | `GetOperation` in `SyntaxNodeAction` costs ~0.3ms/call (no pre-computed cache); the variable-map fast path is 4.5x faster and handles the common identifier receivers. `GetOperation` is used only for the rare non-identifier receivers (`this.`, expression receivers), reading the receiver type off the base `IOperation.Type` | +| Four receiver forms handled explicitly | Database access can target a record via four syntactic forms: `MyTable.Modify()`, `Rec.Modify()`, bare `Modify()` (implicit self), and `this.Modify()` (AL `this` keyword). Variable receivers use the fast path; bare self resolves the containing `ITableTypeSymbol`; `this` (and other expression receivers) resolve via `IOperation.Type` -- the same operation-tree mechanism AC0031 uses, which works on all TFMs (it never references the `ThisExpressionSyntax` type, absent at the netstandard2.1 floor). Missing the `this` form caused the AC0032 false positive in issue #343 | +| Self-table symbol shapes | The table object's declared symbol is an `ITableTypeSymbol` (not an `IRecordTypeSymbol`); `this`/`Rec` resolve to a separate `IRecordTypeSymbol` wrapper. `TryGetPermissionForType` accepts both: record types via `OriginalDefinition`, table types directly | | No cross-callback shared state | Eliminates the fragile two-phase accumulator pattern that caused false positives during incremental compilation | | Iterate `DescendantNodes()` for methods | Finds all MethodOrTriggerDeclarationSyntax in the object, skip obsolete ones | | Skip obsolete methods via symbol | `GetDeclaredSymbol` + `IsObsolete()` on the method symbol | @@ -152,7 +152,6 @@ When removing the first entry from a multi-entry list, `SeparatedSyntaxList.Remo 2. **CalcFields/CalcSums**: Indirect table access through FlowFields is not traced 3. **InherentPermissions overlap**: Table-level `InherentPermissions` may make an object-level entry redundant, but the analyzer does not flag this (different concern from unused) 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) -5. **`this.` self-access on AL 14.0-15.2**: The `this` branch is `#if !NETSTANDARD2_1`-excluded because the public `ThisExpressionSyntax` type (plus `SyntaxKind.ThisExpression` and `IInstanceReferenceOperation`) is absent from the netstandard2.1 compile floor (AL 12.0.13, predating the Fall 2024 `this` feature). AL 14.0-15.2 ship a netstandard2.0 SDK and run ALCops's netstandard2.1 binary, so `this.Modify()` self-access is not detected there and the unused-permission false positive (#343) persists on those versions. It is fixed only on the net8.0/net10.0 binaries (AL 16.0+); the `ThisKeywordSelfAccess` test is guarded to 16.0 for this reason. A reflection-based fallback is not possible (the required SDK members do not exist at the floor) ## Design decisions (continued) diff --git a/.github/instructions/analyzer-development.instructions.md b/.github/instructions/analyzer-development.instructions.md index 573b4404..280f95ec 100644 --- a/.github/instructions/analyzer-development.instructions.md +++ b/.github/instructions/analyzer-development.instructions.md @@ -339,7 +339,7 @@ only some of them produce false positives/negatives (see issue #343, AC0032): | Named variable | `MyTable.Modify()` | `MemberAccessExpressionSyntax` with `IdentifierNameSyntax` receiver | Variable map (locals/params/globals) or `GetSymbolInfo` on the receiver | | Implicit `Rec` | `Rec.Modify()` | same as above (`Rec` is a normal identifier) | same as named variable | | Bare implicit self | `Modify()` | `InvocationExpressionSyntax` whose `Expression` is `IdentifierNameSyntax` (no receiver) | The containing object: a table object's declared symbol is an `ITableTypeSymbol` | -| `this` self-reference | `this.Modify()` | `MemberAccessExpressionSyntax` with a `ThisExpressionSyntax` receiver (AL `this` keyword, runtime 14.0+) | `SemanticModel.GetOperation(thisNode)?.Type` (binds to a `BoundThisReference`/`IInstanceReferenceOperation`) | +| `this` self-reference | `this.Modify()` | `MemberAccessExpressionSyntax` whose receiver is the AL `this` keyword (runtime 14.0+) | `SemanticModel.GetOperation(receiverNode)?.Type` (in a table, `this` binds to the record). Detect it as "any non-`IdentifierNameSyntax` receiver" -- do **not** pattern-match `ThisExpressionSyntax` (absent at the netstandard2.1 floor) | Key symbol-shape gotcha: a **table object's declared symbol is `ITableTypeSymbol`, which is NOT an `IRecordTypeSymbol`**. The record (`Rec`/`this`) is a separate `IRecordTypeSymbol` @@ -349,14 +349,26 @@ for the object/bare-self and `is IRecordTypeSymbol` for variable/`this` receiver Tests that exercise the `this` form must guard on runtime version 14.0 (e.g. `SkipTestIfVersionIsTooLow([...], testCase, "14.0", ...)`), since `this` is a Fall 2024 feature. -`ThisExpressionSyntax` requires a `#if !NETSTANDARD2_1` guard: the public type is **absent** -from the netstandard2.1 compile floor (AL 12.0.13, which predates the Fall 2024 `this` feature; -the type, the `SyntaxKind.ThisExpression` enum member, and `IInstanceReferenceOperation` were all -added in AL 14.0). There is therefore no type-free or `EnumProvider`-reflection fallback. The -netstandard2.1 binary serves AL 14.0-15.2 (those ship a netstandard2.0 SDK), so any analyzer that -relies on `ThisExpressionSyntax` cannot detect `this.` on 14.0-15.2 -- only on the net8.0/net10.0 -binaries (AL 16.0+). Guard the corresponding test to 16.0, not 14.0. Operation-tree based detection -(e.g. `IInvocationOperation.Instance`) has the same floor limitation and cannot work around it. +**Do not** reference `ThisExpressionSyntax` directly (and therefore do not `#if !NETSTANDARD2_1`-guard +`this` handling). The public `ThisExpressionSyntax` type, the `SyntaxKind.ThisExpression` enum member, +and `IInstanceReferenceOperation` are all **absent** from the netstandard2.1 compile floor (AL 12.0.13, +predating the Fall 2024 `this` feature), so a `ThisExpressionSyntax` pattern-match forces a guard that +silently drops `this` detection on the netstandard2.1 binary -- which is exactly what serves AL 14.0-15.2 +(they ship a netstandard2.0 SDK). Instead, resolve the receiver via the **operation tree**, which is +fully available at the floor (`GetOperation`, `IOperation`, `IOperation.Type`, `IInvocationExpression.Instance`): + +```csharp +// Works on every TFM and AL version; never names ThisExpressionSyntax. +if (receiverExpression is not null && receiverExpression is not IdentifierNameSyntax) +{ + var receiverType = ctx.SemanticModel.GetOperation(receiverExpression, ct)?.Type; // `this` binds to the record + // ... build the permission from receiverType +} +``` + +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. ## EnumProvider (Critical Pattern) Never reference `Microsoft.Dynamics.Nav.CodeAnalysis` enum values directly. Always use `EnumProvider` from `ALCops.Common.Reflection`. This provides backward compatibility across SDK versions via reflection-based caching. diff --git a/.github/instructions/netstandard21-compatibility.instructions.md b/.github/instructions/netstandard21-compatibility.instructions.md index 905b1725..15fd88a8 100644 --- a/.github/instructions/netstandard21-compatibility.instructions.md +++ b/.github/instructions/netstandard21-compatibility.instructions.md @@ -21,7 +21,7 @@ Some `Microsoft.Dynamics.Nav.CodeAnalysis` APIs only exist in newer SDK versions | API | Available in | netstandard2.1 workaround | |---|---|---| | `IFieldSymbol.Type` | net8.0 only | `fieldSymbol.OriginalDefinition.GetTypeSymbol()` (requires `using Microsoft.Dynamics.Nav.CodeAnalysis.Symbols`) | -| `ThisExpressionSyntax` (AL `this` keyword) | net8.0+ only | The public type is absent from the netstandard2.1 compile floor (AL 12.0.13, predating the Fall 2024 `this` feature; the type was added in AL 14.0). `SyntaxKind.ThisExpression` and `IInstanceReferenceOperation` are likewise absent at the floor, so there is **no** `EnumProvider`/reflection or operation-tree fallback. Wrap usages in `#if !NETSTANDARD2_1`. Note: the netstandard2.1 binary serves AL 14.0-15.2 at runtime, so guarding it out means `this.` detection (and any fix depending on it) is unavailable there -- guard the corresponding tests to **16.0**, not 14.0. See AC0032 `TableDataAccessUnusedPermissions`. | +| `ThisExpressionSyntax` (AL `this` keyword) | net8.0+ only (do not reference) | The public type, `SyntaxKind.ThisExpression`, and `IInstanceReferenceOperation` are absent from the netstandard2.1 compile floor (AL 12.0.13, predating the Fall 2024 `this` feature), so there is no type-name or `EnumProvider` workaround for the syntax type. Instead, **do not reference it at all**: resolve the receiver via the operation tree (`GetOperation(receiver)?.Type`, or `IInvocationExpression.Instance.Type`), which *is* available at the floor and binds a table's `this` to its record. This avoids the `#if` entirely and works on AL 14.0-15.2 (which run the netstandard2.1 binary). See AC0032 `TableDataAccessUnusedPermissions` and AC0031 `RequiredPermissionDetector`. | ### Pattern for `IFieldSymbol.Type` diff --git a/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs index f862c429..be32bbd4 100644 --- a/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs +++ b/src/ALCops.ApplicationCop.Test/Rules/TableDataAccessUnusedPermissions/TableDataAccessUnusedPermissions.cs @@ -85,8 +85,8 @@ public async Task NoDiagnostic(string testCase) SkipTestIfVersionIsTooLow( ["ThisKeywordSelfAccess"], testCase, - "16.0", - "this. self-access is only detected by the net8.0/net10.0 analyzer builds; AL 14.0-15.2 ship a netstandard2.0 SDK and run ALCops's netstandard2.1 binary, where ThisExpressionSyntax is absent at the 12.0.13 compile floor and the 'this' branch is excluded."); + "14.0", + "The 'this' self-reference keyword requires runtime version 14.0 (BC 2024 wave 2)."); var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(NoDiagnostic), $"{testCase}.al")) diff --git a/src/ALCops.ApplicationCop/Analyzers/TableDataAccessUnusedPermissions.cs b/src/ALCops.ApplicationCop/Analyzers/TableDataAccessUnusedPermissions.cs index 5e8c6b0e..af7888d9 100644 --- a/src/ALCops.ApplicationCop/Analyzers/TableDataAccessUnusedPermissions.cs +++ b/src/ALCops.ApplicationCop/Analyzers/TableDataAccessUnusedPermissions.cs @@ -224,31 +224,11 @@ private static void CollectFromInvocations( if (operation == DatabaseOperation.None) return null; - // `this.Method()` self-reference (AL 'this' keyword, represented as - // ThisExpressionSyntax). In a table/tableextension `this` is the record itself, - // equivalent to a bare `Method()` call. Resolve its type via the semantic model; - // in non-record objects (e.g. codeunits, where `this` is the codeunit instance) - // the type is not a record and is correctly ignored. - // - // Guarded out on netstandard2.1: the public ThisExpressionSyntax type is absent - // from the netstandard2.1 compile floor (AL 12.0.13, which predates the Fall 2024 - // 'this' feature; the type was added in AL 14.0), so it cannot be referenced there. - // Consequence: AL 14.0-15.2 ship a netstandard2.0 SDK and therefore run ALCops's - // netstandard2.1 binary, which has no 'this' handling -- the self-access false - // positive (#343) is only fixed on the net8.0/net10.0 binaries (AL 16.0+). -#if !NETSTANDARD2_1 - if (receiverExpression is ThisExpressionSyntax thisExpression) - { - var thisType = ctx.SemanticModel.GetOperation(thisExpression, ctx.CancellationToken)?.Type; - return TryGetSelfPermission(thisType, operation, node); - } -#endif - // Implicit self: bare `Method()` inside a table. The accessed table is the // containing object itself (an ITableTypeSymbol). The operation model reports a // null instance for bare self calls, so resolve directly from the object symbol. if (hasImplicitSelf) - return TryGetSelfPermission(containingObject as ITypeSymbol, operation, node); + return TryGetPermissionForType(containingObject as ITypeSymbol, operation, node); // Fast path: resolve receiver via variable map lookup if (receiverExpression is IdentifierNameSyntax identifierName) @@ -274,17 +254,33 @@ private static void CollectFromInvocations( } } + // Non-identifier receiver (e.g. `this.Method()`, or an expression receiver such as + // `GetRec().Method()`). Resolve the receiver's type off the base IOperation. This + // mirrors AC0031 (RequiredPermissionDetector) and deliberately avoids referencing + // ThisExpressionSyntax, which is absent from the netstandard2.1 compile floor + // (AL 12.0.13, predating the Fall 2024 `this` feature). IOperation/GetOperation and + // IOperation.Type all exist at that floor, so this works on every TFM and AL version: + // in a table `this` binds to the record; in non-record objects (e.g. a codeunit, + // where `this` is the codeunit instance) the type is not a record and is ignored. + if (receiverExpression is not null && receiverExpression is not IdentifierNameSyntax) + { + var receiverType = ctx.SemanticModel.GetOperation(receiverExpression, ctx.CancellationToken)?.Type; + var permission = TryGetPermissionForType(receiverType, operation, node); + if (permission is not null) + return permission; + } + // Fallback: complex receiver or unresolved name (use GetSymbolInfo) return TryGetPermissionViaSymbolInfo(node, receiverExpression, containingObject, ctx); } /// - /// Builds a required permission for a self-access (bare `Method()` or `this.Method()`). - /// Accepts either a record type (resolved to its backing table) or a table type directly. - /// Returns null when the self type is not a non-temporary record/table (e.g. inside a - /// codeunit, where `this` is the codeunit instance). + /// Builds a required permission from a resolved receiver/self type (bare `Method()`, + /// `this.Method()`, or an expression receiver). Accepts either a record type (resolved to + /// its backing table) or a table type directly. Returns null when the type is not a + /// non-temporary record/table (e.g. inside a codeunit, where `this` is the codeunit instance). /// - private static RequiredPermission? TryGetSelfPermission( + private static RequiredPermission? TryGetPermissionForType( ITypeSymbol? selfType, DatabaseOperation operation, SyntaxNode node)