diff --git a/.github/instructions/analyzer-development.instructions.md b/.github/instructions/analyzer-development.instructions.md index 56941c63..b0e6aed0 100644 --- a/.github/instructions/analyzer-development.instructions.md +++ b/.github/instructions/analyzer-development.instructions.md @@ -380,6 +380,35 @@ This is the same mechanism AC0031 (`RequiredPermissionDetector.TryGetFromInvocat `invocation.Instance.Type`. Keep the variable-map fast path first so `GetOperation` (the ~0.3ms call) only runs for the rare non-identifier receivers. +### Detecting `this`/self at the operation level (`OperationKind.ThisReference`) + +When you already hold the bound `IOperation` (e.g. `IFieldAccess.Instance` inside a +`RegisterOperationAction`) rather than syntax, detect a `this`/self reference via the +**`OperationKind` enum**, not the `IInstanceReferenceOperation` type: + +```csharp +// Works on every TFM. Never names IInstanceReferenceOperation. +var thisReferenceKind = EnumProvider.OperationKind.ThisReference; +if (thisReferenceKind != default && instance.Kind == thisReferenceKind) + return true; // `this`/self +``` + +`IInstanceReferenceOperation`, `ThisExpressionSyntax`, and `SyntaxKind.ThisExpression` are all +absent from the netstandard2.1 compile floor (AL 12.0.13). Referencing the interface forces an +`#if !NETSTANDARD2_1` guard that silently drops `this.` detection on the netstandard2.1 binary that +serves AL 14.0–15.2. `EnumProvider.OperationKind.ThisReference` resolves to `default` (the enum's +`None`) on SDKs without the member — where no `this` code can exist anyway — so the `!= default` +guard makes it a no-op there and exact elsewhere. Add the member to `EnumProvider` with the +string-literal `ParseEnum<…>("ThisReference")` form (like `CompoundAssignmentStatement`), since +`nameof(OperationKind.ThisReference)` will not compile at the floor. + +Note that `this` yields a record-typed symbol (`BoundThisReference.ExpressionSymbol => Type`), so +`GetSymbolSafe().Name` is the table name, **not** `"Rec"`. To distinguish the current record from +the `xRec` before-image (both share the same record type), compare the global's name to the +reserved keyword `"Rec"` — the `IsThis`/`HasImplicitWith` flags that the compiler uses live on the +internal `SynthesizedGlobalVariableSymbol` and are not publicly reachable. See PC0037 +`UseValidateForFieldAssignment.IsCurrentRecordInstance`. + ## 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 15fd88a8..79c28c84 100644 --- a/.github/instructions/netstandard21-compatibility.instructions.md +++ b/.github/instructions/netstandard21-compatibility.instructions.md @@ -22,6 +22,7 @@ Some `Microsoft.Dynamics.Nav.CodeAnalysis` APIs only exist in newer SDK versions |---|---|---| | `IFieldSymbol.Type` | net8.0 only | `fieldSymbol.OriginalDefinition.GetTypeSymbol()` (requires `using Microsoft.Dynamics.Nav.CodeAnalysis.Symbols`) | | `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`. | +| `IInstanceReferenceOperation` (`this`/self when you hold the `IOperation`) | net8.0+ only (do not reference) | When you already have the bound instance operation, detect self via `instance.Kind == EnumProvider.OperationKind.ThisReference` (guarded `!= default`) instead of `instance is IInstanceReferenceOperation`. The `OperationKind` enum member is reachable through `EnumProvider` (string-literal `ParseEnum<…>("ThisReference")`) and resolves to `default` on SDKs without it, so no `#if` is needed and it works on every TFM. See PC0037 `UseValidateForFieldAssignment.IsCurrentRecordInstance` and the this/self note in `analyzer-development.instructions.md`. | ### Pattern for `IFieldSymbol.Type` diff --git a/.github/instructions/pc0037-use-validate-for-field-assignment.instructions.md b/.github/instructions/pc0037-use-validate-for-field-assignment.instructions.md index 0838a926..673ddad1 100644 --- a/.github/instructions/pc0037-use-validate-for-field-assignment.instructions.md +++ b/.github/instructions/pc0037-use-validate-for-field-assignment.instructions.md @@ -26,7 +26,8 @@ Detects direct field assignments on non-temporary record variables and recommend | Flag all fields (including PK, system fields) | Community consensus: no exceptions, use pragma for justified cases | | Exclude only `temporary` record variables | `IRecordTypeSymbol.Temporary == true` — temporary records don't persist and don't need subscriber execution | | Do NOT exclude non-Normal table types | Christian Hovenbitzer: inherently temporary tables should still validate | -| Do NOT exclude assignments inside OnValidate triggers | Even cascading field assignments should use Validate to trigger subscribers on the target field | +| Suppress same-field assignment to the current record inside its own validate trigger | Assigning a field to `Rec`/self inside that field's own `OnValidate`/`OnBeforeValidate`/`OnAfterValidate` is by design (default values, value transformation such as rounding); calling `Validate()` there is impossible/recursive. Issue #357 | +| Suppression is narrow: same field only, current record only | Cross-field cascades, `xRec`, and other same-table variables still fire (they are different records / different fields, so subscribers must run) | | Do NOT exclude assignments after Init() | The Init+assign+Insert pattern should still use Validate where possible | | No ChangeCompany handling | Use pragma for legacy ChangeCompany+write patterns | | Register for CompoundAssignmentStatement | Guard with `!= default` for netstandard2.1 where the OperationKind doesn't exist | @@ -36,19 +37,26 @@ Detects direct field assignments on non-temporary record variables and recommend - **Analyzer**: Registers for `OperationKind.AssignmentStatement` and `OperationKind.CompoundAssignmentStatement` - **Detection**: Checks if `IAssignmentStatement.Target` is `IFieldAccess` with a non-temporary `IRecordTypeSymbol` instance +- **Own-validate suppression** (`IsAssignmentToOwnValidateField`): walks to the nearest `TriggerDeclarationSyntax`, requires its name to be `OnValidate`/`OnBeforeValidate`/`OnAfterValidate`, requires the assigned instance to be the current record, then compares the assigned field name (`IsSameName`) to the trigger's owner field +- **Current-record detection** (`IsCurrentRecordInstance`): true when the instance is a `this`/self reference — detected via `instance.Kind == EnumProvider.OperationKind.ThisReference` (guarded `!= default`), **not** the `IInstanceReferenceOperation` type — or when its symbol is named `Rec` (covers explicit `Rec.` and a page's implicit-with bare reference). `Rec`/`xRec` are reserved AL keywords, so the name is the only public discriminator between the current record and the `xRec` before-image. See the this/self note in `analyzer-development.instructions.md`. +- **Owner field resolution** (`ResolveTriggerOwnerField`): the trigger symbol's `ContainingSymbol` is the owner — an `IFieldSymbol` (table field), an `IControlSymbol` (page control, resolved via `RelatedFieldSymbol`), or a change-modify symbol for `modify(...)` extensions whose modified base field/control is read via the internal `Target` property (`PropertyAccessor.GetPropertyIfExists`), then resolved recursively - **Location**: Reports on `fieldAccess.Syntax.GetIdentifierNameSyntax()` (the field identifier token) - **CodeFix**: Navigates from diagnostic span to parent `AssignmentStatementSyntax`, rewrites to `ExpressionStatement(InvocationExpression(MemberAccess(Rec, "Validate"), ArgumentList(FieldName, Value)))` ## Test coverage -**HasDiagnostic (5 cases):** SimpleAssignment, CompoundAssignment, InsideOnValidateTrigger, AfterInit, PrimaryKeyField. -**NoDiagnostic (3 cases):** TemporaryVariable, ValidateCall, NonRecordVariable. +**HasDiagnostic (7 cases):** SimpleAssignment, CompoundAssignment, AfterInit, PrimaryKeyField, OnValidateDifferentFieldOnRec, OnValidateXRecSameField, OnValidateOtherRecordSameField. +**NoDiagnostic (12 cases):** TemporaryVariable, ValidateCall, NonRecordVariable, InsideOnValidateTrigger, TableFieldOnValidateSameField, TableExtensionFieldOnBeforeValidateSameField, TableExtensionFieldOnAfterValidateSameField, PageControlOnValidateSameField, PageExtensionControlOnBeforeValidateSameField, PageExtensionControlOnAfterValidateSameField, OnValidateSameFieldThisReference, PageControlOnValidateSameFieldBareReference. **HasFix (1 case):** SimpleAssignment. ## Known issues - CompoundAssignmentStatement OperationKind does not exist in netstandard2.1 SDK. Guarded with `!= default` check. - The CodeFix does not handle compound assignments (`+=`, `-=`) — only simple `:=` is auto-fixable. +- `this`/self detection uses the `OperationKind.ThisReference` enum (via `EnumProvider`, guarded `!= default`), **not** the `IInstanceReferenceOperation` type. That type is absent from the netstandard2.1 compile floor (AL 12.0.13), and referencing it would force an `#if !NETSTANDARD2_1` guard that silently drops `this.` suppression on the netstandard2.1 binary serving AL 14.0–15.2. The enum approach works on every TFM with no guard. See AC0032 / PR #353. +- `OnBeforeValidate`/`OnAfterValidate` are only valid on **modified** fields/controls (`modify(...)`), not on newly-added extension fields (AL0162). The table/page extension fixtures therefore use `modify("Unit Price")` on a base field. +- A bare table self field reference (`"Field" := ...`, no `Rec.`) binds to the table object type (not `IRecordTypeSymbol`), so it never fires today and needs no suppression. Page bare references use implicit-with → `Rec`, so they fire and are suppressed. +- `xRec` and other same-table record variables keep firing — they are different records. ## Related diff --git a/src/ALCops.Common/Reflection/EnumProvider.cs b/src/ALCops.Common/Reflection/EnumProvider.cs index 6f27aca0..eda786df 100644 --- a/src/ALCops.Common/Reflection/EnumProvider.cs +++ b/src/ALCops.Common/Reflection/EnumProvider.cs @@ -484,6 +484,8 @@ public static class OperationKind new(() => ParseEnum(nameof(NavCodeAnalysis.OperationKind.AssignmentStatement))); private static readonly Lazy _binaryOperatorExpression = new(() => ParseEnum(nameof(NavCodeAnalysis.OperationKind.BinaryOperatorExpression))); + private static readonly Lazy _compoundAssignmentStatement = + new(() => ParseEnum("CompoundAssignmentStatement")); private static readonly Lazy _conversionExpression = new(() => ParseEnum(nameof(NavCodeAnalysis.OperationKind.ConversionExpression))); private static readonly Lazy _emptyStatement = @@ -506,17 +508,18 @@ public static class OperationKind new(() => ParseEnum(nameof(NavCodeAnalysis.OperationKind.ParameterReferenceExpression))); private static readonly Lazy _returnValueReferenceExpression = new(() => ParseEnum(nameof(NavCodeAnalysis.OperationKind.ReturnValueReferenceExpression))); + private static readonly Lazy _thisReference = + new(() => ParseEnum("ThisReference")); private static readonly Lazy _xmlPortDataItemAccess = new(() => ParseEnum(nameof(NavCodeAnalysis.OperationKind.XmlPortDataItemAccess))); - private static readonly Lazy _compoundAssignmentStatement = - new(() => ParseEnum("CompoundAssignmentStatement")); public static NavCodeAnalysis.OperationKind AssignmentStatement => _assignmentStatement.Value; public static NavCodeAnalysis.OperationKind BinaryOperatorExpression => _binaryOperatorExpression.Value; + public static NavCodeAnalysis.OperationKind CompoundAssignmentStatement => _compoundAssignmentStatement.Value; public static NavCodeAnalysis.OperationKind ConversionExpression => _conversionExpression.Value; public static NavCodeAnalysis.OperationKind EmptyStatement => _emptyStatement.Value; - public static NavCodeAnalysis.OperationKind ExpressionStatement => _expressionStatement.Value; public static NavCodeAnalysis.OperationKind ExitStatement => _exitStatement.Value; + public static NavCodeAnalysis.OperationKind ExpressionStatement => _expressionStatement.Value; public static NavCodeAnalysis.OperationKind FieldAccess => _fieldAccess.Value; public static NavCodeAnalysis.OperationKind GlobalReferenceExpression => _globalReferenceExpression.Value; public static NavCodeAnalysis.OperationKind InvocationExpression => _invocationExpression.Value; @@ -524,8 +527,8 @@ public static class OperationKind public static NavCodeAnalysis.OperationKind LocalReferenceExpression => _localReferenceExpression.Value; public static NavCodeAnalysis.OperationKind ParameterReferenceExpression => _parameterReferenceExpression.Value; public static NavCodeAnalysis.OperationKind ReturnValueReferenceExpression => _returnValueReferenceExpression.Value; + public static NavCodeAnalysis.OperationKind ThisReference => _thisReference.Value; public static NavCodeAnalysis.OperationKind XmlPortDataItemAccess => _xmlPortDataItemAccess.Value; - public static NavCodeAnalysis.OperationKind CompoundAssignmentStatement => _compoundAssignmentStatement.Value; } /// @@ -790,6 +793,8 @@ public static class SymbolKind { private static readonly Lazy _action = new(() => ParseEnum(nameof(NavCodeAnalysis.SymbolKind.Action))); + private static readonly Lazy _change = + new(() => ParseEnum(nameof(NavCodeAnalysis.SymbolKind.Change))); private static readonly Lazy _class = new(() => ParseEnum(nameof(NavCodeAnalysis.SymbolKind.Class))); private static readonly Lazy _codeunit = @@ -864,6 +869,7 @@ public static class SymbolKind #endif public static NavCodeAnalysis.SymbolKind Action => _action.Value; + public static NavCodeAnalysis.SymbolKind Change => _change.Value; public static NavCodeAnalysis.SymbolKind Class => _class.Value; public static NavCodeAnalysis.SymbolKind Codeunit => _codeunit.Value; public static NavCodeAnalysis.SymbolKind ControlAddIn => _controlAddIn.Value; diff --git a/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/HasDiagnostic/OnValidateDifferentFieldOnRec.al b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/HasDiagnostic/OnValidateDifferentFieldOnRec.al new file mode 100644 index 00000000..f2deeae0 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/HasDiagnostic/OnValidateDifferentFieldOnRec.al @@ -0,0 +1,15 @@ +table 50100 "Sales Line" +{ + fields + { + field(1; "Unit Price"; Decimal) + { + trigger OnValidate() + begin + Rec.[|"Line Amount"|] := Rec."Unit Price" * Rec.Quantity; + end; + } + field(2; Quantity; Decimal) { } + field(3; "Line Amount"; Decimal) { } + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/HasDiagnostic/OnValidateOtherRecordSameField.al b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/HasDiagnostic/OnValidateOtherRecordSameField.al new file mode 100644 index 00000000..1a3da236 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/HasDiagnostic/OnValidateOtherRecordSameField.al @@ -0,0 +1,15 @@ +table 50100 "Sales Line" +{ + fields + { + field(1; "Unit Price"; Decimal) + { + trigger OnValidate() + var + OtherSalesLine: Record "Sales Line"; + begin + OtherSalesLine.[|"Unit Price"|] := 100; + end; + } + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/HasDiagnostic/OnValidateXRecSameField.al b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/HasDiagnostic/OnValidateXRecSameField.al new file mode 100644 index 00000000..e318b7cf --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/HasDiagnostic/OnValidateXRecSameField.al @@ -0,0 +1,13 @@ +table 50100 "Sales Line" +{ + fields + { + field(1; "Unit Price"; Decimal) + { + trigger OnValidate() + begin + xRec.[|"Unit Price"|] := 100; + end; + } + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/OnValidateSameFieldThisReference.al b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/OnValidateSameFieldThisReference.al new file mode 100644 index 00000000..f6d29e8f --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/OnValidateSameFieldThisReference.al @@ -0,0 +1,13 @@ +table 50100 "Sales Line" +{ + fields + { + field(1; "Unit Price"; Decimal) + { + trigger OnValidate() + begin + this.[|"Unit Price"|] := 100; + end; + } + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/PageControlOnValidateSameField.al b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/PageControlOnValidateSameField.al new file mode 100644 index 00000000..71daf1aa --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/PageControlOnValidateSameField.al @@ -0,0 +1,27 @@ +table 50100 "Sales Line" +{ + fields + { + field(1; "Unit Price"; Decimal) { } + } +} + +page 50100 "Sales Line Card" +{ + PageType = Card; + SourceTable = "Sales Line"; + + layout + { + area(Content) + { + field(UnitPrice; Rec."Unit Price") + { + trigger OnValidate() + begin + Rec.[|"Unit Price"|] := 100; + end; + } + } + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/PageControlOnValidateSameFieldBareReference.al b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/PageControlOnValidateSameFieldBareReference.al new file mode 100644 index 00000000..1827dd4f --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/PageControlOnValidateSameFieldBareReference.al @@ -0,0 +1,27 @@ +table 50100 "Sales Line" +{ + fields + { + field(1; "Unit Price"; Decimal) { } + } +} + +page 50100 "Sales Line Card" +{ + PageType = Card; + SourceTable = "Sales Line"; + + layout + { + area(Content) + { + field(UnitPrice; Rec."Unit Price") + { + trigger OnValidate() + begin + [|"Unit Price"|] := 100; + end; + } + } + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/PageExtensionControlOnAfterValidateSameField.al b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/PageExtensionControlOnAfterValidateSameField.al new file mode 100644 index 00000000..44c58b20 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/PageExtensionControlOnAfterValidateSameField.al @@ -0,0 +1,35 @@ +table 50100 "Sales Line" +{ + fields + { + field(1; "Unit Price"; Decimal) { } + } +} + +page 50100 "Sales Line Card" +{ + PageType = Card; + SourceTable = "Sales Line"; + + layout + { + area(Content) + { + field(UnitPrice; Rec."Unit Price") { } + } + } +} + +pageextension 50101 "Sales Line Card Ext" extends "Sales Line Card" +{ + layout + { + modify(UnitPrice) + { + trigger OnAfterValidate() + begin + Rec.[|"Unit Price"|] := 100; + end; + } + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/PageExtensionControlOnBeforeValidateSameField.al b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/PageExtensionControlOnBeforeValidateSameField.al new file mode 100644 index 00000000..fc623ba5 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/PageExtensionControlOnBeforeValidateSameField.al @@ -0,0 +1,35 @@ +table 50100 "Sales Line" +{ + fields + { + field(1; "Unit Price"; Decimal) { } + } +} + +page 50100 "Sales Line Card" +{ + PageType = Card; + SourceTable = "Sales Line"; + + layout + { + area(Content) + { + field(UnitPrice; Rec."Unit Price") { } + } + } +} + +pageextension 50101 "Sales Line Card Ext" extends "Sales Line Card" +{ + layout + { + modify(UnitPrice) + { + trigger OnBeforeValidate() + begin + Rec.[|"Unit Price"|] := 100; + end; + } + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/TableExtensionFieldOnAfterValidateSameField.al b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/TableExtensionFieldOnAfterValidateSameField.al new file mode 100644 index 00000000..bd2f4572 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/TableExtensionFieldOnAfterValidateSameField.al @@ -0,0 +1,21 @@ +table 50100 "Sales Line" +{ + fields + { + field(1; "Unit Price"; Decimal) { } + } +} + +tableextension 50101 "Sales Line Ext" extends "Sales Line" +{ + fields + { + modify("Unit Price") + { + trigger OnAfterValidate() + begin + Rec.[|"Unit Price"|] := 100; + end; + } + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/TableExtensionFieldOnBeforeValidateSameField.al b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/TableExtensionFieldOnBeforeValidateSameField.al new file mode 100644 index 00000000..ae73d537 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/TableExtensionFieldOnBeforeValidateSameField.al @@ -0,0 +1,21 @@ +table 50100 "Sales Line" +{ + fields + { + field(1; "Unit Price"; Decimal) { } + } +} + +tableextension 50101 "Sales Line Ext" extends "Sales Line" +{ + fields + { + modify("Unit Price") + { + trigger OnBeforeValidate() + begin + Rec.[|"Unit Price"|] := 100; + end; + } + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/TableFieldOnValidateSameField.al b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/TableFieldOnValidateSameField.al new file mode 100644 index 00000000..cf802556 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/NoDiagnostic/TableFieldOnValidateSameField.al @@ -0,0 +1,14 @@ +table 50100 "Sales Line" +{ + fields + { + field(1; "Unit Price"; Decimal) + { + trigger OnValidate() + begin + Rec.[|"Unit Price"|] := Round(Rec."Unit Price", 0.01); + end; + } + field(2; Quantity; Decimal) { } + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/UseValidateForFieldAssignment.cs b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/UseValidateForFieldAssignment.cs index 3930a40f..d561e07d 100644 --- a/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/UseValidateForFieldAssignment.cs +++ b/src/ALCops.PlatformCop.Test/Rules/UseValidateForFieldAssignment/UseValidateForFieldAssignment.cs @@ -25,6 +25,9 @@ public void Setup() [TestCase("CompoundAssignment")] [TestCase("AfterInit")] [TestCase("PrimaryKeyField")] + [TestCase("OnValidateDifferentFieldOnRec")] + [TestCase("OnValidateXRecSameField")] + [TestCase("OnValidateOtherRecordSameField")] public async Task HasDiagnostic(string testCase) { var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(HasDiagnostic), $"{testCase}.al")) @@ -38,8 +41,33 @@ public async Task HasDiagnostic(string testCase) [TestCase("ValidateCall")] [TestCase("NonRecordVariable")] [TestCase("InsideOnValidateTrigger")] + [TestCase("TableFieldOnValidateSameField")] + [TestCase("TableExtensionFieldOnBeforeValidateSameField")] + [TestCase("TableExtensionFieldOnAfterValidateSameField")] + [TestCase("PageControlOnValidateSameField")] + [TestCase("PageExtensionControlOnBeforeValidateSameField")] + [TestCase("PageExtensionControlOnAfterValidateSameField")] + [TestCase("OnValidateSameFieldThisReference")] + [TestCase("PageControlOnValidateSameFieldBareReference")] public async Task NoDiagnostic(string testCase) { + SkipTestIfVersionIsTooLow( + [ + "TableExtensionFieldOnBeforeValidateSameField", + "TableExtensionFieldOnAfterValidateSameField", + "PageExtensionControlOnBeforeValidateSameField", + "PageExtensionControlOnAfterValidateSameField" + ], + testCase, + "13.0", + "No support for table/page extensions when target itself is already declared in the same module"); + + SkipTestIfVersionIsTooLow( + ["OnValidateSameFieldThisReference"], + 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.PlatformCop/Analyzers/UseValidateForFieldAssignment.cs b/src/ALCops.PlatformCop/Analyzers/UseValidateForFieldAssignment.cs index 534c7452..d3b99f6c 100644 --- a/src/ALCops.PlatformCop/Analyzers/UseValidateForFieldAssignment.cs +++ b/src/ALCops.PlatformCop/Analyzers/UseValidateForFieldAssignment.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Immutable; using ALCops.Common.Extensions; using ALCops.Common.Reflection; @@ -49,6 +50,9 @@ private static void AnalyzeAssignmentOperation(OperationAnalysisContext ctx) if (recordType.Temporary) return; + if (IsAssignmentToOwnValidateField(ctx, fieldAccess)) + return; + var location = fieldAccess.Syntax?.GetIdentifierNameSyntax()?.GetLocation() ?? fieldAccess.Syntax?.GetLocation(); @@ -61,4 +65,117 @@ private static void AnalyzeAssignmentOperation(OperationAnalysisContext ctx) location, fieldAccess.FieldSymbol.Name)); } + + private static readonly string[] ValidateTriggerNames = + { + "OnValidate", + "OnBeforeValidate", + "OnAfterValidate" + }; + + // Assigning a field to the current record (Rec/self) inside that same field's own + // OnValidate / OnBeforeValidate / OnAfterValidate trigger is by design (default + // values, value transformation such as rounding) and cannot use Validate(). + private static bool IsAssignmentToOwnValidateField(OperationAnalysisContext ctx, IFieldAccess fieldAccess) + { + var syntax = fieldAccess.Syntax; + if (syntax is null) + return false; + + var trigger = syntax.FirstAncestorOrSelf(); + if (trigger is null) + return false; + + if (!IsValidateTrigger(trigger)) + return false; + + if (!IsCurrentRecordInstance(fieldAccess.Instance)) + return false; + + var ownerField = ResolveTriggerOwnerField(ctx); + if (ownerField is null) + return false; + + return fieldAccess.FieldSymbol.Name.IsSameName(ownerField.Name); + } + + private static bool IsValidateTrigger(TriggerDeclarationSyntax trigger) + { + var name = trigger.Name?.Identifier.ValueText; + if (name is null) + return false; + + foreach (var validateName in ValidateTriggerNames) + { + if (string.Equals(name, validateName, StringComparison.OrdinalIgnoreCase)) + return true; + } + + return false; + } + + // The current record is referenced either as 'this'/self or through the synthesized 'Rec' + // global variable (including a page's implicit-with bare reference). 'xRec' (the before-image) + // and any user-declared record variable are different records and must keep firing. + private static bool IsCurrentRecordInstance(IOperation? instance) + { + if (instance is null) + return false; + + // 'this'/self reference (AL 14.0+). Detected via the OperationKind enum, NOT the + // IInstanceReferenceOperation type: that type (and SyntaxKind.ThisExpression) is absent + // from the netstandard2.1 compile floor (AL 12.0.13, predating the Fall 2024 'this' + // feature). Referencing it would force an #if guard that silently drops 'this' detection + // on the netstandard2.1 binary that serves AL 14.0-15.2. The enum resolves to default on + // SDKs without the member (where no 'this' code can exist anyway). See AC0032 / PR #353. + var thisReferenceKind = EnumProvider.OperationKind.ThisReference; + if (thisReferenceKind != default && instance.Kind == thisReferenceKind) + return true; + + // Current record global 'Rec'. 'Rec' and 'xRec' are reserved AL keywords the compiler + // synthesizes by these exact names (TableObjectMembers/PageObjectMembers), sharing the + // same record type, so the name is the only public, stable discriminator between the + // current record and the 'xRec' before-image (the distinguishing IsThis/HasImplicitWith + // flags live on the internal SynthesizedGlobalVariableSymbol and are not publicly reachable). + var symbol = instance.GetSymbolSafe(); + return symbol is not null + && string.Equals(symbol.Name, "Rec", StringComparison.OrdinalIgnoreCase); + } + + // The trigger symbol's containing symbol is the field (table/table extension) or the + // control (page/page extension) that owns the trigger. For a control, the owner field + // is the table field bound to its source expression. For OnBeforeValidate/OnAfterValidate + // the owner is a change-modify symbol whose modified base field/control is exposed via + // an internal 'Target' property. + private static IFieldSymbol? ResolveTriggerOwnerField(OperationAnalysisContext ctx) + => ResolveOwnerField(ctx.ContainingSymbol?.ContainingSymbol); + + // Name of the internal 'SourceChangeModifySymbol.Target' property. It is not exposed on + // the public IChangeSymbol interface (which only surfaces ChangeKind and Type), so the + // modified base field/control can only be reached via reflection. + private const string ChangeModifyTargetPropertyName = "Target"; + + private static IFieldSymbol? ResolveOwnerField(ISymbol? owner) + { + switch (owner) + { + case null: + return null; + case IFieldSymbol field: + return field; + case IControlSymbol control: + return control.RelatedFieldSymbol; + } + + // modify(field)/modify(control) in extensions: the owner is a change-modify symbol + // (SymbolKind.Change) whose modified base symbol is exposed via the internal 'Target'. + if (owner.Kind != EnumProvider.SymbolKind.Change) + return null; + + var target = owner.GetPropertyIfExists(ChangeModifyTargetPropertyName); + if (target is not null && !ReferenceEquals(target, owner)) + return ResolveOwnerField(target); + + return null; + } }