diff --git a/.claude/rules/diagnostics/pc0020-pc0021-transferfields-schema-compatibility.md b/.claude/rules/diagnostics/pc0020-pc0021-transferfields-schema-compatibility.md index 1618fdae..00b49576 100644 --- a/.claude/rules/diagnostics/pc0020-pc0021-transferfields-schema-compatibility.md +++ b/.claude/rules/diagnostics/pc0020-pc0021-transferfields-schema-compatibility.md @@ -18,7 +18,9 @@ Registers `RegisterOperationAction` on `InvocationExpression` and `RegisterSymbo | Decision | Rationale | |---|---| | Two analysis paths: the invocation path compares the tables of each `TransferFields` call, the relation path compares extension-added fields of curated table pairs (`TransferFieldsRelations.TableRelations`, e.g. Customer → Contact) | The relation path catches extension fields that collide on well-known BaseApp transfers even when the call site is not in the analyzed module | -| Invocation path reports field-level diagnostics only for pairs outside the curated list, plus one summary diagnostic at the invocation site | Curated pairs are already reported field-by-field on the extension fields themselves by the relation path | +| Invocation path reports field-level diagnostics only when the call's (source, target) pair is not a curated relation in that direction (source = argument record, target = receiver), plus one summary diagnostic at the invocation site | Curated pairs are already reported field-by-field on the extension fields themselves by the relation path. The lookup is by pair, not by either table alone: a table that appears somewhere in the list says nothing about an unrelated call | +| On a curated pair whose two tables are both Microsoft's, mismatches where both fields are Microsoft-owned are dropped before reporting (no summary, no field-level diagnostic). A field declared in the analyzed module is never Microsoft-owned; a dependency tableextension field is Microsoft-owned only when that extension is Microsoft's | Microsoft ships and supports these transfers with fields that differ by name or type (Purchase Header ↔ Purchase Header Archive field 151, Tracking Specification ↔ Reservation Entry fields 31 and 900; the 5043 difference is between FlowFields, which the rule ignores); the developer cannot rename Base App fields, and a pragma on the call would also hide collisions on their own extension fields ([#418](https://github.com/ALCops/Analyzers/issues/418)). Both sides must be Microsoft-owned: once one side was added by the developer or a third party the collision is new and outside Microsoft's support. This deliberately differs from the field pragma, which is an explicit user suppression and works from either side | +| Ownership test is `ISymbol.IsMicrosoftObject()` (Common): namespace root `Microsoft` or `System`, else module publisher `Microsoft` | Both namespace roots are reserved for Microsoft (AS0008/PTE0021) and the namespace is already read for the relation lookup; the publisher fallback covers pre-namespace Base App versions. Tables that only share a curated name in a non-Microsoft module are not the Base App tables and stay reported | | Skip via `IsRemoved()` (Removed/Moved), not `IsObsolete()` | `ObsoleteState = Pending` tables and fields still participate at runtime and must keep firing | | Removed fields, calls where either table is removed, and removed relation-path extensions (or extensions of a removed base table) are excluded | Removed fields do not participate in `TransferFields` at runtime ([#148](https://github.com/ALCops/Analyzers/issues/148)); upgrade code transfers from removed tables and a removed target makes the call dead code ([#435](https://github.com/ALCops/Analyzers/issues/435)) | | Field-level `#pragma warning disable` on either side suppresses the pair | Checked against both fields' syntax directives, so silencing one side is enough | @@ -35,11 +37,14 @@ Registers `RegisterOperationAction` on `InvocationExpression` and `RegisterSymbo - Type pairs the platform converts implicitly (Enum→Integer, Code→Text, Integer→BigInteger/Decimal). - PK fields when `InitPrimaryKeyFields` is `false`; any pair when `SkipFieldsNotMatchingType` is a constant `true`. - PC0021: names that differ only by a mandatory affix on a same-module extension field. +- Microsoft-owned field pairs on a curated pair of Microsoft tables. Not suppressed: a pair that is curated only in the reverse direction, and non-Microsoft tables that carry a curated name. ## Known issues - Platform-parity affix matching can over-strip coincidental substrings (`Customer` with affix `MER` → `Custo`); when the paired same-ID field's core genuinely collides, PC0021 stays silent. Accepted as a narrow false negative for SDK parity ([#436](https://github.com/ALCops/Analyzers/issues/436)). -- Relation-path coverage only applies to the curated `TransferFieldsRelations.TableRelations` list, which carries BC version ranges (`MinVersion`/`MaxVersion`). +- Relation-path coverage only applies to the curated `TransferFieldsRelations.TableRelations` list, which carries BC version ranges (`MinVersion`/`MaxVersion`). Those ranges are stored but never read: a relation counts as curated on every BC version. +- The suppression of a Microsoft dependency tableextension's field on a curated pair cannot be covered by a fixture (it needs a second module); it is verified by reasoning only. +- A third-party dependency's tableextension field that collides on a curated Microsoft pair is still reported in every app that calls `TransferFields` on that pair, although the consuming app cannot change it. ## SDK facts @@ -53,3 +58,4 @@ Registers `RegisterOperationAction` on `InvocationExpression` and `RegisterSymbo - Invocation-path fixtures for removed tables must be upgrade codeunits (see SDK facts). - Affix fixtures (`Affix_*`) inject an `AppSourceCop.json` via `MemoryFileSystem`; this requires `Microsoft.Dynamics.Nav.Analyzers.Common.dll` as a `Private=True` reference in the test csproj (ALCops.Common references it with `Private=False`). - Tableextension fixtures are gated on runtime 13.0 and `this` receiver fixtures on 14.0. +- Microsoft ownership is exercised two ways: a `namespace Microsoft.*` fixture for BC24+ tables, and namespace-less tables under a fixture whose `ProjectInfoCustomizer` sets the module publisher to `Microsoft` (`WithProjectDefinition`); the harness default publisher is `Default Publisher`. One AL file is one module with one namespace, so a fixture cannot mix a Microsoft table with a non-Microsoft extension; own-module extension fields are keyed on the declaring module instead. diff --git a/.claude/skills/fix-false-positive/references/regression-catalog.md b/.claude/skills/fix-false-positive/references/regression-catalog.md index 951478a7..2364f4a1 100644 --- a/.claude/skills/fix-false-positive/references/regression-catalog.md +++ b/.claude/skills/fix-false-positive/references/regression-catalog.md @@ -24,4 +24,5 @@ Recurring causes of false positives/negatives, mined from `fix(...)` commits. Wh | **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 | | **Version-scoped syntax** (`this`, newer keywords) | Fixtures need `SkipTestIfVersionIsTooLow("14.0")` or `RequireMinimumVersion(...)`; a rule may need a `VersionProvider` gate rather than a code change. | PC0035 fixtures, PC0029 | +| **Base App differences the developer cannot change** | A comparison between two Microsoft objects can flag differences Microsoft ships and supports (renamed or retyped fields on a curated pair). Suppress only where both sides are Microsoft-owned (`ISymbol.IsMicrosoftObject()`: namespace root `Microsoft`/`System`, else publisher `Microsoft`) and keep checking anything the analyzed module or a third party added. | PC0020/PC0021 #418 | | **Null `Instance` means bare self *or* static built-in class** | `BoundCall.Instance` is null for any `IsStatic` target (IsolatedStorage Get/Delete/Contains, NumberSequence Insert/Delete/Next, File Rename/Copy, bare Error/Format), not just bare self. `GetReceiverTableType` takes the invocation or field access itself (never its `Instance`) and returns null for static targets. | PC0013 #550 | diff --git a/src/ALCops.Common/Extensions/SymbolInterfaceExtensions.cs b/src/ALCops.Common/Extensions/SymbolInterfaceExtensions.cs index bf7676b3..9c2ce267 100644 --- a/src/ALCops.Common/Extensions/SymbolInterfaceExtensions.cs +++ b/src/ALCops.Common/Extensions/SymbolInterfaceExtensions.cs @@ -54,6 +54,25 @@ public static string GetFullyQualifiedObjectName(this ISymbol symbol, bool quote return $"{containingNamespace}.{symbolName}"; } + /// + /// True when the object is Microsoft-owned: its root namespace is "Microsoft" or "System" + /// (both reserved for Microsoft, see AS0008/PTE0021), or, when it has no namespace + /// (pre-namespace BC or a bare declaration), its module's publisher is "Microsoft". + /// + public static bool IsMicrosoftObject(this ISymbol symbol) + { + var ns = symbol.GetContainingNamespaceQualifiedNameWithReflection(); + if (!string.IsNullOrEmpty(ns)) + { + var dot = ns.IndexOf('.'); + var root = dot < 0 ? ns : ns.Substring(0, dot); + return SemanticFacts.IsSameName(root, "Microsoft") || SemanticFacts.IsSameName(root, "System"); + } + + var publisher = symbol.ContainingModule?.Publisher; + return publisher is not null && StringComparer.OrdinalIgnoreCase.Equals(publisher, "Microsoft"); + } + #region Obsolete Extension Methods // COMPAT(netstandard2.1, net8.0): IsObsoletePendingMove and IsObsoleteMoved may not exist in older SDK versions. // TODO: When netstandard2.1 and net8.0 are dropped, check if these properties are on ISymbol directly. diff --git a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/HasDiagnostic/CuratedPair_DefaultPublisher.al b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/HasDiagnostic/CuratedPair_DefaultPublisher.al new file mode 100644 index 00000000..3c514356 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/HasDiagnostic/CuratedPair_DefaultPublisher.al @@ -0,0 +1,32 @@ +// Same curated pair as CuratedPair_MicrosoftPublisher, but the module is not Microsoft's +// ("Default Publisher", no namespace): tables that merely share a curated name are not the +// Base App tables, so the mismatch is reported at the call. Field-level reporting stays off +// for curated pairs. +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + PurchaseHeader: Record "Purchase Header"; + PurchaseHeaderArchive: Record "Purchase Header Archive"; + begin + [|PurchaseHeader.TransferFields(PurchaseHeaderArchive)|]; + end; +} + +table 38 "Purchase Header" +{ + fields + { + field(1; "No."; Code[20]) { } + field(151; "Quote No."; Code[20]) { } + } +} + +table 5109 "Purchase Header Archive" +{ + fields + { + field(1; "No."; Code[20]) { } + field(151; "Purchase Quote No."; Code[20]) { } + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/HasDiagnostic/CuratedPair_OwnExtensionField.al b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/HasDiagnostic/CuratedPair_OwnExtensionField.al new file mode 100644 index 00000000..e257ca28 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/HasDiagnostic/CuratedPair_OwnExtensionField.al @@ -0,0 +1,51 @@ +// Curated Microsoft pair: the Microsoft-owned differences on fields 31 and 900 are not +// reported, but fields added by tableextensions of the analyzed module are always checked, +// so the collision on field 50100 is still reported at the call. +namespace Microsoft.Inventory.Tracking; + +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + TrackingSpecification: Record "Tracking Specification"; + ReservationEntry: Record "Reservation Entry"; + begin + [|ReservationEntry.TransferFields(TrackingSpecification)|]; + end; +} + +table 336 "Tracking Specification" +{ + fields + { + field(1; "Entry No."; Integer) { } + field(31; "Qty. Rounding Precision (Base)"; Decimal) { } + field(900; "Prohibit Cancellation"; Boolean) { } + } +} + +table 337 "Reservation Entry" +{ + fields + { + field(1; "Entry No."; Integer) { } + field(31; "Action Message Adjustment"; Decimal) { } + field(900; "Disallow Cancellation"; Boolean) { } + } +} + +tableextension 50100 MyTrackingSpecExt extends "Tracking Specification" +{ + fields + { + field(50100; "My Field A"; Integer) { } + } +} + +tableextension 50101 MyReservationEntryExt extends "Reservation Entry" +{ + fields + { + field(50100; "My Field B"; Integer) { } // Same ID (50100) as in MyTrackingSpecExt, different name + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/HasDiagnostic/NonCuratedPair_MicrosoftTables.al b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/HasDiagnostic/NonCuratedPair_MicrosoftTables.al new file mode 100644 index 00000000..746b64ca --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/HasDiagnostic/NonCuratedPair_MicrosoftTables.al @@ -0,0 +1,30 @@ +// Both tables are Microsoft's, but Purchase Header Archive -> Customer is not a curated +// TransferFields relation: every mismatch is reported, at the call and on both fields. +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + Customer: Record Customer; + PurchaseHeaderArchive: Record "Purchase Header Archive"; + begin + [|Customer.TransferFields(PurchaseHeaderArchive)|]; + end; +} + +table 18 Customer +{ + fields + { + field(1; "No."; Code[20]) { } + [|field(21; "Currency Code"; Code[10]) { }|] + } +} + +table 5109 "Purchase Header Archive" +{ + fields + { + field(1; "No."; Code[20]) { } + [|field(21; "Posting Date"; Date) { }|] + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/HasDiagnostic/ReverseOnlyPair_MicrosoftNamespace.al b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/HasDiagnostic/ReverseOnlyPair_MicrosoftNamespace.al new file mode 100644 index 00000000..3fb100dc --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/HasDiagnostic/ReverseOnlyPair_MicrosoftNamespace.al @@ -0,0 +1,34 @@ +// Bank Deposit Header -> Posted Bank Deposit Header is curated in that direction only. This call +// runs the other way (source = Posted Bank Deposit Header, target = Bank Deposit Header), so it is +// not a supported pair: the mismatch is reported at the call and on both fields, although both +// tables are Microsoft's. +namespace Microsoft.Bank.Deposit; + +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + BankDepositHeader: Record "Bank Deposit Header"; + PostedBankDepositHeader: Record "Posted Bank Deposit Header"; + begin + [|BankDepositHeader.TransferFields(PostedBankDepositHeader)|]; + end; +} + +table 1690 "Bank Deposit Header" +{ + fields + { + field(1; "No."; Code[20]) { } + [|field(2; "Bank Account No."; Code[20]) { }|] + } +} + +table 1695 "Posted Bank Deposit Header" +{ + fields + { + field(1; "No."; Code[20]) { } + [|field(2; "Posted Bank Account No."; Code[20]) { }|] // Same ID (2) as in Bank Deposit Header, different name + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/NoDiagnostic/CuratedPair_MicrosoftNamespace.al b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/NoDiagnostic/CuratedPair_MicrosoftNamespace.al new file mode 100644 index 00000000..6fb4a9e8 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/NoDiagnostic/CuratedPair_MicrosoftNamespace.al @@ -0,0 +1,35 @@ +// Tracking Specification -> Reservation Entry is a curated TransferFields relation and both +// tables live in the Microsoft namespace. Fields 31 and 900 differ by name in the Base App; +// the developer cannot rename them, so neither the call nor the fields are reported. +namespace Microsoft.Inventory.Tracking; + +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + TrackingSpecification: Record "Tracking Specification"; + ReservationEntry: Record "Reservation Entry"; + begin + [|ReservationEntry.TransferFields(TrackingSpecification)|]; + end; +} + +table 336 "Tracking Specification" +{ + fields + { + field(1; "Entry No."; Integer) { } + [|field(31; "Qty. Rounding Precision (Base)"; Decimal) { }|] + [|field(900; "Prohibit Cancellation"; Boolean) { }|] + } +} + +table 337 "Reservation Entry" +{ + fields + { + field(1; "Entry No."; Integer) { } + [|field(31; "Action Message Adjustment"; Decimal) { }|] + [|field(900; "Disallow Cancellation"; Boolean) { }|] + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/NoDiagnostic/CuratedPair_MicrosoftPublisher.al b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/NoDiagnostic/CuratedPair_MicrosoftPublisher.al new file mode 100644 index 00000000..466f7cf8 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/NoDiagnostic/CuratedPair_MicrosoftPublisher.al @@ -0,0 +1,31 @@ +// Purchase Header Archive -> Purchase Header is a curated TransferFields relation. The tables +// have no namespace (pre-namespace Base App), so ownership falls back to the module publisher, +// which the test fixture sets to "Microsoft". Field 151 differs by name in the Base App. +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + PurchaseHeader: Record "Purchase Header"; + PurchaseHeaderArchive: Record "Purchase Header Archive"; + begin + [|PurchaseHeader.TransferFields(PurchaseHeaderArchive)|]; + end; +} + +table 38 "Purchase Header" +{ + fields + { + field(1; "No."; Code[20]) { } + [|field(151; "Quote No."; Code[20]) { }|] + } +} + +table 5109 "Purchase Header Archive" +{ + fields + { + field(1; "No."; Code[20]) { } + [|field(151; "Purchase Quote No."; Code[20]) { }|] + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/TransferFieldsNameMismatch.cs b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/TransferFieldsNameMismatch.cs index e55990f9..0dfdd49e 100644 --- a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/TransferFieldsNameMismatch.cs +++ b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsNameMismatch/TransferFieldsNameMismatch.cs @@ -1,4 +1,5 @@ using Microsoft.Dynamics.Nav.CodeAnalysis; +using Microsoft.Dynamics.Nav.CodeAnalysis.Workspaces; using RoslynTestKit; namespace ALCops.PlatformCop.Test @@ -52,6 +53,21 @@ private static AnalyzerTestFixture CreateFixtureWithCoincidentalAffix() }); } + private static AnalyzerTestFixture CreateFixtureWithMicrosoftPublisher() + { + return RoslynFixtureFactory.Create( + new AnalyzerTestFixtureConfig + { + ProjectInfoCustomizer = info => info.WithProjectDefinition(new ProjectDefinition + { + Publisher = "Microsoft", + Name = "TestApp", + Version = new Version(1, 0, 0, 0), + AppId = Guid.NewGuid() + }) + }); + } + [SetUp] public void Setup() { @@ -64,6 +80,8 @@ public void Setup() } [Test] + [TestCase("CuratedPair_DefaultPublisher")] + [TestCase("CuratedPair_OwnExtensionField")] [TestCase("InvocationRecWithCodeunit")] [TestCase("InvocationRecWithPage")] [TestCase("InvocationRecWithTable")] @@ -82,10 +100,11 @@ public void Setup() [TestCase("TableExt_NamespaceCasingMismatch")] [TestCase("InvocationBareSelfInTableExtension")] [TestCase("InvocationThisSelfInTable")] + [TestCase("ReverseOnlyPair_MicrosoftNamespace")] public async Task HasDiagnostic(string testCase) { SkipTestIfVersionIsTooLow( - ["InvocationWithTableExtension", "InvocationBareSelfInTableExtension", "TableExt_Multiple_SameBase", "TableExtension", "TableExtensionTypeWithLength", "TableExt_NamespaceCasingMismatch"], + ["CuratedPair_OwnExtensionField", "InvocationWithTableExtension", "InvocationBareSelfInTableExtension", "TableExt_Multiple_SameBase", "TableExtension", "TableExtensionTypeWithLength", "TableExt_NamespaceCasingMismatch"], testCase, "13.0", "No support for tableextensions when target itself is already declared in the same module"); @@ -104,6 +123,7 @@ public async Task HasDiagnostic(string testCase) [Test] [TestCase("BuiltInInvocation")] + [TestCase("CuratedPair_MicrosoftNamespace")] [TestCase("Invocation_ObsoleteStateRemoved")] [TestCase("Invocation_Pragma")] [TestCase("Invocation_SourceTableObsoleteStateRemoved")] @@ -131,6 +151,26 @@ public async Task NoDiagnostic(string testCase) _fixture.NoDiagnosticAtAllMarkers(code, DiagnosticIds.TransferFieldsNameMismatch); } + [Test] + [TestCase("NonCuratedPair_MicrosoftTables")] + public async Task HasDiagnosticWithMicrosoftPublisher(string testCase) + { + var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(HasDiagnostic), $"{testCase}.al")) + .ConfigureAwait(false); + + CreateFixtureWithMicrosoftPublisher().HasDiagnosticAtAllMarkers(code, DiagnosticIds.TransferFieldsNameMismatch); + } + + [Test] + [TestCase("CuratedPair_MicrosoftPublisher")] + public async Task NoDiagnosticWithMicrosoftPublisher(string testCase) + { + var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(NoDiagnostic), $"{testCase}.al")) + .ConfigureAwait(false); + + CreateFixtureWithMicrosoftPublisher().NoDiagnosticAtAllMarkers(code, DiagnosticIds.TransferFieldsNameMismatch); + } + [Test] [TestCase("Affix_Invocation_CoreNameDiffers")] [TestCase("Affix_Invocation_OwnTableFieldsNotStripped")] diff --git a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/HasDiagnostic/CuratedPair_DefaultPublisher.al b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/HasDiagnostic/CuratedPair_DefaultPublisher.al new file mode 100644 index 00000000..26869840 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/HasDiagnostic/CuratedPair_DefaultPublisher.al @@ -0,0 +1,32 @@ +// Same curated pair as CuratedPair_MicrosoftPublisher, but the module is not Microsoft's +// ("Default Publisher", no namespace): tables that merely share a curated name are not the +// Base App tables, so the mismatch is reported at the call. Field-level reporting stays off +// for curated pairs. +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + PurchaseHeader: Record "Purchase Header"; + PurchaseHeaderArchive: Record "Purchase Header Archive"; + begin + [|PurchaseHeader.TransferFields(PurchaseHeaderArchive)|]; + end; +} + +table 38 "Purchase Header" +{ + fields + { + field(1; "No."; Code[20]) { } + field(5043; "No. of Archived Versions"; Integer) { } + } +} + +table 5109 "Purchase Header Archive" +{ + fields + { + field(1; "No."; Code[20]) { } + field(5043; "Interaction Exist"; Boolean) { } + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/HasDiagnostic/CuratedPair_OwnExtensionField.al b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/HasDiagnostic/CuratedPair_OwnExtensionField.al new file mode 100644 index 00000000..00eb40d8 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/HasDiagnostic/CuratedPair_OwnExtensionField.al @@ -0,0 +1,51 @@ +// Curated Microsoft pair: the Microsoft-owned differences on fields 31 and 900 are not +// reported, but fields added by tableextensions of the analyzed module are always checked, +// so the collision on field 50100 is still reported at the call. +namespace Microsoft.Inventory.Tracking; + +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + TrackingSpecification: Record "Tracking Specification"; + ReservationEntry: Record "Reservation Entry"; + begin + [|ReservationEntry.TransferFields(TrackingSpecification)|]; + end; +} + +table 336 "Tracking Specification" +{ + fields + { + field(1; "Entry No."; Integer) { } + field(31; "Qty. Rounding Precision (Base)"; Decimal) { } + field(900; "Prohibit Cancellation"; Boolean) { } + } +} + +table 337 "Reservation Entry" +{ + fields + { + field(1; "Entry No."; Integer) { } + field(31; "Action Message Adjustment"; Decimal) { } + field(900; "Disallow Cancellation"; Integer) { } + } +} + +tableextension 50100 MyTrackingSpecExt extends "Tracking Specification" +{ + fields + { + field(50100; "My Field"; Integer) { } + } +} + +tableextension 50101 MyReservationEntryExt extends "Reservation Entry" +{ + fields + { + field(50100; "My Field"; Boolean) { } // Same ID (50100) as in MyTrackingSpecExt, different type + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/HasDiagnostic/NonCuratedPair_MicrosoftTables.al b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/HasDiagnostic/NonCuratedPair_MicrosoftTables.al new file mode 100644 index 00000000..746b64ca --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/HasDiagnostic/NonCuratedPair_MicrosoftTables.al @@ -0,0 +1,30 @@ +// Both tables are Microsoft's, but Purchase Header Archive -> Customer is not a curated +// TransferFields relation: every mismatch is reported, at the call and on both fields. +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + Customer: Record Customer; + PurchaseHeaderArchive: Record "Purchase Header Archive"; + begin + [|Customer.TransferFields(PurchaseHeaderArchive)|]; + end; +} + +table 18 Customer +{ + fields + { + field(1; "No."; Code[20]) { } + [|field(21; "Currency Code"; Code[10]) { }|] + } +} + +table 5109 "Purchase Header Archive" +{ + fields + { + field(1; "No."; Code[20]) { } + [|field(21; "Posting Date"; Date) { }|] + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/HasDiagnostic/ReverseOnlyPair_MicrosoftNamespace.al b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/HasDiagnostic/ReverseOnlyPair_MicrosoftNamespace.al new file mode 100644 index 00000000..5f4d70c1 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/HasDiagnostic/ReverseOnlyPair_MicrosoftNamespace.al @@ -0,0 +1,34 @@ +// Bank Deposit Header -> Posted Bank Deposit Header is curated in that direction only. This call +// runs the other way (source = Posted Bank Deposit Header, target = Bank Deposit Header), so it is +// not a supported pair: the mismatch is reported at the call and on both fields, although both +// tables are Microsoft's. +namespace Microsoft.Bank.Deposit; + +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + BankDepositHeader: Record "Bank Deposit Header"; + PostedBankDepositHeader: Record "Posted Bank Deposit Header"; + begin + [|BankDepositHeader.TransferFields(PostedBankDepositHeader)|]; + end; +} + +table 1690 "Bank Deposit Header" +{ + fields + { + field(1; "No."; Code[20]) { } + [|field(2; "Bank Account No."; Integer) { }|] + } +} + +table 1695 "Posted Bank Deposit Header" +{ + fields + { + field(1; "No."; Code[20]) { } + [|field(2; "Bank Account No."; Boolean) { }|] // Same ID (2) as in Bank Deposit Header, different type + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/NoDiagnostic/CuratedPair_MicrosoftNamespace.al b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/NoDiagnostic/CuratedPair_MicrosoftNamespace.al new file mode 100644 index 00000000..5b7145b1 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/NoDiagnostic/CuratedPair_MicrosoftNamespace.al @@ -0,0 +1,36 @@ +// Tracking Specification -> Reservation Entry is a curated TransferFields relation and both +// tables live in the Microsoft namespace. The real pair differs by name only (fields 31 and +// 900); the type difference on field 900 is synthetic, to prove Microsoft-owned type +// mismatches on a supported pair are not reported either. +namespace Microsoft.Inventory.Tracking; + +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + TrackingSpecification: Record "Tracking Specification"; + ReservationEntry: Record "Reservation Entry"; + begin + [|ReservationEntry.TransferFields(TrackingSpecification)|]; + end; +} + +table 336 "Tracking Specification" +{ + fields + { + field(1; "Entry No."; Integer) { } + [|field(31; "Qty. Rounding Precision (Base)"; Decimal) { }|] + [|field(900; "Prohibit Cancellation"; Boolean) { }|] + } +} + +table 337 "Reservation Entry" +{ + fields + { + field(1; "Entry No."; Integer) { } + [|field(31; "Action Message Adjustment"; Decimal) { }|] + [|field(900; "Disallow Cancellation"; Integer) { }|] + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/NoDiagnostic/CuratedPair_MicrosoftPublisher.al b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/NoDiagnostic/CuratedPair_MicrosoftPublisher.al new file mode 100644 index 00000000..3991a910 --- /dev/null +++ b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/NoDiagnostic/CuratedPair_MicrosoftPublisher.al @@ -0,0 +1,32 @@ +// Purchase Header Archive -> Purchase Header is a curated TransferFields relation. The tables +// have no namespace (pre-namespace Base App), so ownership falls back to the module publisher, +// which the test fixture sets to "Microsoft". In the Base App field 5043 is a FlowField, which the +// rule skips; the fixture models it as Normal fields to exercise the type path. +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + PurchaseHeader: Record "Purchase Header"; + PurchaseHeaderArchive: Record "Purchase Header Archive"; + begin + [|PurchaseHeader.TransferFields(PurchaseHeaderArchive)|]; + end; +} + +table 38 "Purchase Header" +{ + fields + { + field(1; "No."; Code[20]) { } + [|field(5043; "No. of Archived Versions"; Integer) { }|] + } +} + +table 5109 "Purchase Header Archive" +{ + fields + { + field(1; "No."; Code[20]) { } + [|field(5043; "Interaction Exist"; Boolean) { }|] + } +} diff --git a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/TransferFieldsTypeMismatch.cs b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/TransferFieldsTypeMismatch.cs index 6084e15d..d5b63210 100644 --- a/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/TransferFieldsTypeMismatch.cs +++ b/src/ALCops.PlatformCop.Test/Rules/TransferFieldsTypeMismatch/TransferFieldsTypeMismatch.cs @@ -1,3 +1,4 @@ +using Microsoft.Dynamics.Nav.CodeAnalysis.Workspaces; using RoslynTestKit; namespace ALCops.PlatformCop.Test @@ -7,6 +8,21 @@ public class TransferFieldsTypeMismatch : NavCodeAnalysisBase private AnalyzerTestFixture _fixture; private string _testCasePath; + private static AnalyzerTestFixture CreateFixtureWithMicrosoftPublisher() + { + return RoslynFixtureFactory.Create( + new AnalyzerTestFixtureConfig + { + ProjectInfoCustomizer = info => info.WithProjectDefinition(new ProjectDefinition + { + Publisher = "Microsoft", + Name = "TestApp", + Version = new Version(1, 0, 0, 0), + AppId = Guid.NewGuid() + }) + }); + } + [SetUp] public void Setup() { @@ -19,6 +35,8 @@ public void Setup() } [Test] + [TestCase("CuratedPair_DefaultPublisher")] + [TestCase("CuratedPair_OwnExtensionField")] [TestCase("InvocationRecWithCodeunit")] [TestCase("InvocationRecWithPage")] [TestCase("InvocationRecWithTable")] @@ -38,10 +56,11 @@ public void Setup() [TestCase("TableExtensionTypeWithTypeLength")] [TestCase("InvocationBareSelfInTableExtension")] [TestCase("InvocationThisSelfInTable")] + [TestCase("ReverseOnlyPair_MicrosoftNamespace")] public async Task HasDiagnostic(string testCase) { SkipTestIfVersionIsTooLow( - ["InvocationWithTableExtension", "InvocationBareSelfInTableExtension", "TableExt_Multiple_SameBase", "TableExtension", "TableExtensionTypeWithType", "TableExtensionTypeWithTypeLength"], + ["CuratedPair_OwnExtensionField", "InvocationWithTableExtension", "InvocationBareSelfInTableExtension", "TableExt_Multiple_SameBase", "TableExtension", "TableExtensionTypeWithType", "TableExtensionTypeWithTypeLength"], testCase, "13.0", "No support for tableextensions when target itself is already declared in the same module"); @@ -60,6 +79,7 @@ public async Task HasDiagnostic(string testCase) [Test] [TestCase("BuiltInInvocation")] + [TestCase("CuratedPair_MicrosoftNamespace")] [TestCase("Invocation_ObsoleteStateRemoved")] [TestCase("Invocation_Pragma")] [TestCase("Invocation_SourceTableObsoleteStateRemoved")] @@ -90,5 +110,25 @@ public async Task NoDiagnostic(string testCase) _fixture.NoDiagnosticAtAllMarkers(code, DiagnosticIds.TransferFieldsTypeMismatch); } + + [Test] + [TestCase("NonCuratedPair_MicrosoftTables")] + public async Task HasDiagnosticWithMicrosoftPublisher(string testCase) + { + var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(HasDiagnostic), $"{testCase}.al")) + .ConfigureAwait(false); + + CreateFixtureWithMicrosoftPublisher().HasDiagnosticAtAllMarkers(code, DiagnosticIds.TransferFieldsTypeMismatch); + } + + [Test] + [TestCase("CuratedPair_MicrosoftPublisher")] + public async Task NoDiagnosticWithMicrosoftPublisher(string testCase) + { + var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(NoDiagnostic), $"{testCase}.al")) + .ConfigureAwait(false); + + CreateFixtureWithMicrosoftPublisher().NoDiagnosticAtAllMarkers(code, DiagnosticIds.TransferFieldsTypeMismatch); + } } } \ No newline at end of file diff --git a/src/ALCops.PlatformCop/Analyzers/TransferFieldsRelations.cs b/src/ALCops.PlatformCop/Analyzers/TransferFieldsRelations.cs index 566a147e..6c63c4e9 100644 --- a/src/ALCops.PlatformCop/Analyzers/TransferFieldsRelations.cs +++ b/src/ALCops.PlatformCop/Analyzers/TransferFieldsRelations.cs @@ -21,23 +21,32 @@ public static IEnumerable TryFindBySource(ITableTypeSymbol table) } /// - /// Determines whether a table relation exists where the given table - /// matches either Source or Target. + /// Determines whether a curated relation exists for exactly this direction: + /// source = the record passed to TransferFields, target = the receiver. /// - /// The table symbol to match. + /// The table passed to TransferFields. + /// The table calling TransferFields. /// true if a matching relation exists; otherwise, false. - public static bool HasTableRelation(ITableTypeSymbol table) + public static bool HasTableRelation(ITableTypeSymbol source, ITableTypeSymbol target) { + var sourceNs = source.GetContainingNamespaceQualifiedNameWithReflection() ?? string.Empty; + var sourceName = source.Name ?? string.Empty; + var targetNs = target.GetContainingNamespaceQualifiedNameWithReflection() ?? string.Empty; + var targetName = target.Name ?? string.Empty; + return TableRelations.Any(item => - Matches(item.Source, table) || - Matches(item.Target, table)); + Matches(item.Source, sourceNs, sourceName) && + Matches(item.Target, targetNs, targetName)); } - private static bool Matches(ObjectName configured, ITableTypeSymbol table) - { - var ns = table.GetContainingNamespaceQualifiedNameWithReflection() ?? string.Empty; - var name = table.Name ?? string.Empty; + private static bool Matches(ObjectName configured, ITableTypeSymbol table) => + Matches( + configured, + table.GetContainingNamespaceQualifiedNameWithReflection() ?? string.Empty, + table.Name ?? string.Empty); + private static bool Matches(ObjectName configured, string ns, string name) + { // AL namespaces and object identifiers are case-insensitive, but the runtime-resolved // namespace casing (symbol.ContainingNamespace.QualifiedName) is not stable across // compilations, so both comparisons must be case-insensitive. diff --git a/src/ALCops.PlatformCop/Analyzers/TransferFieldsSchemaCompatibility.cs b/src/ALCops.PlatformCop/Analyzers/TransferFieldsSchemaCompatibility.cs index 61ca7ff4..c3c3bc20 100644 --- a/src/ALCops.PlatformCop/Analyzers/TransferFieldsSchemaCompatibility.cs +++ b/src/ALCops.PlatformCop/Analyzers/TransferFieldsSchemaCompatibility.cs @@ -165,19 +165,24 @@ private static void AnalyzeInvocation(OperationAnalysisContext ctx) if (sourceById.Count == 0 || targetById.Count == 0) return; - var hasTableRelationEntry = HasTableRelation(sourceTable); + var isCuratedPair = HasTableRelation(sourceTable, targetTable); + // Microsoft ships and supports this transfer; differences between fields Microsoft owns on + // both sides are theirs to keep. Fields the developer or a third party added stay checked. + var isMicrosoftSupportedPair = isCuratedPair && sourceTable.IsMicrosoftObject() && targetTable.IsMicrosoftObject(); var targetDisplay = targetTable.GetFullyQualifiedObjectName(quoteIdentifierIfNeeded: true); var sourceDisplay = sourceTable.GetFullyQualifiedObjectName(quoteIdentifierIfNeeded: true); var mismatches = FindFieldMismatches(sourceById, targetById, ctx.Compilation); + if (isMicrosoftSupportedPair) + mismatches.RemoveAll(m => IsMicrosoftOwnedField(m.Source, ctx.Compilation) && IsMicrosoftOwnedField(m.Target, ctx.Compilation)); var result = ReportMismatches( mismatches, ctx.Compilation, sourceDisplay, targetDisplay, - reportAtFieldLevel: !hasTableRelationEntry, + reportAtFieldLevel: !isCuratedPair, swapDisplayForTarget: false, getLocation: static field => field.Location, reportDiagnostic: ctx.ReportDiagnostic); @@ -529,6 +534,21 @@ private static bool IsDeclaredInCurrentModule(IFieldSymbol field, Compilation co return location is not null && location.IsInSource && IsLocationInCompilation(location, compilation); } + // Only meaningful once the caller has established that both tables are Microsoft's. + private static bool IsMicrosoftOwnedField(IFieldSymbol field, Compilation compilation) + { + // Declared on the (Microsoft) table itself + if (field.ContainingSymbol is not ITableExtensionTypeSymbol extension) + return true; + + // The developer controls fields of the analyzed module + if (IsDeclaredInCurrentModule(field, compilation)) + return false; + + // A dependency's tableextension: only Microsoft's own extensions are Microsoft-owned + return extension.IsMicrosoftObject(); + } + private static MismatchResult ReportMismatches( List mismatches, Compilation compilation,