diff --git a/.github/instructions/fc0004-permission-declaration-order.instructions.md b/.github/instructions/fc0004-permission-declaration-order.instructions.md index 7fa499d..221f337 100644 --- a/.github/instructions/fc0004-permission-declaration-order.instructions.md +++ b/.github/instructions/fc0004-permission-declaration-order.instructions.md @@ -22,7 +22,8 @@ Detects `Permissions` property entries that are not sorted alphabetically by (ty ``` src/ALCops.Common/ └── Permissions/ - └── PermissionSyntaxHelper.cs # Shared sort logic, multi-line builder + ├── NaturalNameComparer.cs # Natural/alphanumeric name comparison (AZ-compatible) + └── PermissionSyntaxHelper.cs # Shared sort logic, multi-line builder src/ALCops.FormattingCop/ ├── Analyzers/ @@ -33,11 +34,21 @@ src/ALCops.FormattingCop/ ### Sort order -Entries are sorted by two keys: -1. **Type keyword** (alphabetical, case-insensitive): `codeunit` < `page` < `query` < `record` < `report` < `table` < `tabledata` < `xmlport` -2. **Object name** (alphabetical, case-insensitive) within the same type +Entries are sorted using AZ Dev Tools-compatible ordering (matches `PermissionComparer` from al-code-outline): -Uses `StringComparison.OrdinalIgnoreCase` for both comparisons. Note that ordinal comparison means `(` < `+` < `0` < `A`, which may differ from culture-specific ordering. +1. **Table types first**: `table` and `tabledata` entries are grouped before all other types + - Within table types, entries are sorted by **object name** first (natural/alphanumeric comparison) + - For the same object name, `table` sorts before `tabledata` +2. **Remaining types alphabetically**: `codeunit` < `page` < `query` < `report` < `xmlport` + - Within the same type, sorted by **object name** (natural/alphanumeric comparison) + +**Name comparison** uses `NaturalNameComparer` (natural/alphanumeric sort): +- Splits names into text and numeric chunks +- Text chunks: spaces stripped, compared with `StringComparison.InvariantCultureIgnoreCase` +- Numeric chunks: compared as integers (`"Item 2"` < `"Item 10"`) +- Tiebreaker: shorter string first + +Uses `StringComparison.OrdinalIgnoreCase` for type keyword comparison only (fixed ASCII vocabulary). ### Analysis flow @@ -59,7 +70,11 @@ Uses `StringComparison.OrdinalIgnoreCase` for both comparisons. Note that ordina | `RegisterCompilationAction` | Same pattern as AC0032; iterates all objects in one pass | | One diagnostic per object, not per entry | The fix reorders the entire list; per-entry diagnostics would be noise | | Diagnostic on `PropertySyntax` node | Ensures CodeFix can find the node via `FindNode` + ancestor/descendant traversal | -| Type+name sorting applied globally (affects AC0031/AC0032) | `ArePermissionsSorted` was refactored from name-only to type+name; consistent behavior across all permission rules | +| AZ Dev Tools-compatible sort order | Prevents false positives on code formatted by AZ Dev Tools' "Sort Permissions" | +| table/tabledata first, rest alphabetical | Matches AZ's fixed priority map; more future-proof for new permission types | +| Natural/alphanumeric name comparison | Matches AZ's `AlphanumComparatorFast`; handles numeric suffixes correctly | +| `InvariantCultureIgnoreCase` for names | Handles accented chars and punctuation weight; matches AZ behavior | +| Space stripping in text chunks | Matches AZ behavior; prevents spaces from affecting sort order | | CodeFix always outputs multi-line for 2+ entries | Consistent formatting; single-line permissions are hard to scan | | Single entry stays single-line | No formatting benefit from multi-line with one entry | | Permission chars casing preserved | Each entry keeps its original `r`/`R`/`rimd`/`RIMD`/`X` casing | @@ -93,13 +108,18 @@ var propertySyntax = node as PropertySyntax ## Test coverage -**HasDiagnostic (4 cases):** UnsortedTabledata, UnsortedMixedTypes, UnsortedSingleType, UnsortedCodeunit. -**NoDiagnostic (4 cases):** AlreadySorted, SingleEntry, NoPermissionsProperty, SortedTabledata. +**HasDiagnostic (6 cases):** UnsortedTabledata, UnsortedMixedTypes, UnsortedSingleType, UnsortedCodeunit, UnsortedNumeric, UnsortedTableNotFirst. +**NoDiagnostic (7 cases):** AlreadySorted, SingleEntry, NoPermissionsProperty, SortedTabledata, DotsBeforeLetters, NaturalNumericSort, TableTypesFirst. **HasFix (4 cases):** ReorderTabledata, ReorderMixedTypes, SingleLineToMultiLine, PreserveCasing. ## Refactoring impact -The `PermissionSyntaxHelper.ArePermissionsSorted` method was changed from name-only to type+name comparison. This affects AC0031's `FindInsertionIndex` (used by its CodeFix to decide alphabetical vs append insertion). The behavioral change is: -- A list sorted by name but not by type is now detected as "unsorted" -- AC0031's CodeFix will append (instead of inserting alphabetically) for such lists -- This is the correct behavior since type+name is the canonical sort order +The `PermissionSyntaxHelper` sort logic was changed from simple `OrdinalIgnoreCase` to AZ Dev Tools-compatible ordering. This affects: +- **FC0004**: Uses the new sort order for detection and code fix +- **AC0031's `FindInsertionIndex`**: Uses `ComparePermissionEntries` for insertion point calculation +- **AC0032**: Uses `ArePermissionsSorted` to determine if a list is sorted + +The behavioral changes: +- table/tabledata types now sort before other types (previously alphabetical: `codeunit` came first) +- Names use natural/alphanumeric comparison (previously ordinal: "Item 10" came before "Item 2") +- Spaces in names are ignored during comparison (previously significant) diff --git a/src/ALCops.Common/Permissions/NaturalNameComparer.cs b/src/ALCops.Common/Permissions/NaturalNameComparer.cs new file mode 100644 index 0000000..eba1707 --- /dev/null +++ b/src/ALCops.Common/Permissions/NaturalNameComparer.cs @@ -0,0 +1,108 @@ +namespace ALCops.Common.Permissions; + +/// +/// Natural/alphanumeric string comparer for AL object names. +/// Splits strings into text and numeric chunks: +/// - Text chunks are compared with spaces stripped and InvariantCultureIgnoreCase. +/// - Numeric chunks are compared as integers. +/// - Shorter string wins as tiebreaker when all chunks are equal. +/// Functionally equivalent to AZ AL Dev Tools' AlphanumComparatorFast. +/// +public static class NaturalNameComparer +{ + /// + /// Compares two object names using natural/alphanumeric sorting. + /// Null or empty strings sort after non-empty strings. + /// + public static int Compare(string? x, string? y) + { + bool xEmpty = string.IsNullOrWhiteSpace(x); + bool yEmpty = string.IsNullOrWhiteSpace(y); + + if (xEmpty && yEmpty) + return 0; + if (xEmpty) + return 1; + if (yEmpty) + return -1; + + int len1 = x!.Length; + int len2 = y!.Length; + int marker1 = 0; + int marker2 = 0; + + while (marker1 < len1 && marker2 < len2) + { + char ch1 = x[marker1]; + char ch2 = y[marker2]; + + // Collect chunk from x + int chunkStart1 = marker1; + bool isDigit1 = char.IsDigit(ch1); + while (marker1 < len1 && char.IsDigit(x[marker1]) == isDigit1) + marker1++; + + // Collect chunk from y + int chunkStart2 = marker2; + bool isDigit2 = char.IsDigit(ch2); + while (marker2 < len2 && char.IsDigit(y[marker2]) == isDigit2) + marker2++; + + int result; + if (isDigit1 && isDigit2) + { + // Both numeric: compare as integers + long num1 = ParseLong(x, chunkStart1, marker1); + long num2 = ParseLong(y, chunkStart2, marker2); + result = num1.CompareTo(num2); + } + else + { + // At least one is text: compare with spaces stripped, InvariantCultureIgnoreCase + var str1 = StripSpaces(x, chunkStart1, marker1); + var str2 = StripSpaces(y, chunkStart2, marker2); + result = string.Compare(str1, str2, StringComparison.InvariantCultureIgnoreCase); + } + + if (result != 0) + return result; + } + + return len1 - len2; + } + + private static long ParseLong(string s, int start, int end) + { + long result = 0; + for (int i = start; i < end; i++) + result = result * 10 + (s[i] - '0'); + return result; + } + + private static string StripSpaces(string s, int start, int end) + { + // Fast path: if no spaces, return substring directly + bool hasSpace = false; + for (int i = start; i < end; i++) + { + if (s[i] == ' ') + { + hasSpace = true; + break; + } + } + + if (!hasSpace) + return s.Substring(start, end - start); + + var chars = new char[end - start]; + int count = 0; + for (int i = start; i < end; i++) + { + if (s[i] != ' ') + chars[count++] = s[i]; + } + + return new string(chars, 0, count); + } +} diff --git a/src/ALCops.Common/Permissions/PermissionSyntaxHelper.cs b/src/ALCops.Common/Permissions/PermissionSyntaxHelper.cs index da65d0b..8b96c4a 100644 --- a/src/ALCops.Common/Permissions/PermissionSyntaxHelper.cs +++ b/src/ALCops.Common/Permissions/PermissionSyntaxHelper.cs @@ -13,9 +13,9 @@ public static class PermissionSyntaxHelper private const string CanonicalOrder = MethodOperationMap.CanonicalOrder; /// - /// Checks whether the permission entries are sorted alphabetically by - /// (type keyword, object name), both case-insensitive. - /// Returns true if 0 or 1 entries (trivially sorted). + /// Checks whether the permission entries are sorted using AZ Dev Tools-compatible ordering: + /// table/tabledata types first (grouped by name), remaining types alphabetically by type, then by name. + /// Names use natural/alphanumeric comparison. Returns true if 0 or 1 entries (trivially sorted). /// public static bool ArePermissionsSorted(SeparatedSyntaxList permissions) { @@ -33,11 +33,8 @@ public static bool ArePermissionsSorted(SeparatedSyntaxList pe if (previousType is not null && previousName is not null) { - int typeCompare = string.Compare(previousType, type, StringComparison.OrdinalIgnoreCase); - if (typeCompare > 0) - return false; - if (typeCompare == 0 && - string.Compare(previousName, name, StringComparison.OrdinalIgnoreCase) > 0) + int cmp = ComparePermissionEntries(previousType, previousName, type, name); + if (cmp > 0) return false; } @@ -127,7 +124,7 @@ public static PermissionSyntax CreatePermissionSyntax(string tableName, string p /// /// Finds the insertion index for a new entry in a sorted permission list, - /// comparing by (type keyword, object name). The new entry type defaults to "tabledata" + /// using AZ Dev Tools-compatible ordering. The new entry type defaults to "tabledata" /// since AC0031 only inserts tabledata entries. /// If not sorted, returns the count (append). /// @@ -144,10 +141,8 @@ public static int FindInsertionIndex(SeparatedSyntaxList permi if (entryType is null || entryName is null) continue; - int typeCompare = string.Compare(newType, entryType, StringComparison.OrdinalIgnoreCase); - if (typeCompare < 0) - return i; - if (typeCompare == 0 && string.Compare(tableName, entryName, StringComparison.OrdinalIgnoreCase) < 0) + int cmp = ComparePermissionEntries(newType, tableName, entryType, entryName); + if (cmp < 0) return i; } @@ -317,7 +312,8 @@ private static bool HasNewlineTrivia(SyntaxTriviaList triviaList) } /// - /// Sorts the permission entries by (type keyword, object name), both case-insensitive. + /// Sorts the permission entries using AZ Dev Tools-compatible ordering: + /// table/tabledata first (grouped by name), remaining types alphabetically. /// Returns a new list with the entries in sorted order, stripping leading trivia from /// all entries (callers are responsible for applying formatting). /// @@ -331,18 +327,51 @@ public static List GetSortedPermissions(SeparatedSyntaxList + /// Compares two permission entries using AZ Dev Tools-compatible ordering: + /// - table/tabledata types sort first, grouped by name (table before tabledata for same name) + /// - remaining types sort alphabetically by type keyword (OrdinalIgnoreCase) + /// - within the same non-table type, sort by name using natural/alphanumeric comparison + /// + public static int ComparePermissionEntries(string typeA, string nameA, string typeB, string nameB) + { + bool aIsTable = IsTableType(typeA); + bool bIsTable = IsTableType(typeB); + + if (aIsTable && bIsTable) + { + // Both table types: sort by name first, then type (table before tabledata) + int nameCompare = NaturalNameComparer.Compare(nameA, nameB); + if (nameCompare != 0) + return nameCompare; + return string.Compare(typeA, typeB, StringComparison.OrdinalIgnoreCase); + } + + if (aIsTable != bIsTable) + { + // Table types come first + return aIsTable ? -1 : 1; + } + + // Both non-table: sort by type alphabetically, then by name + int typeCompare = string.Compare(typeA, typeB, StringComparison.OrdinalIgnoreCase); + if (typeCompare != 0) + return typeCompare; + return NaturalNameComparer.Compare(nameA, nameB); + } + + private static bool IsTableType(string type) => + string.Equals(type, "table", StringComparison.OrdinalIgnoreCase) || + string.Equals(type, "tabledata", StringComparison.OrdinalIgnoreCase); + /// /// Builds a multi-line PermissionPropertyValueSyntax from an ordered list of entries. /// The first entry has no leading trivia (it shares the line with "Permissions = "). diff --git a/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasDiagnostic/UnsortedMixedTypes.al b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasDiagnostic/UnsortedMixedTypes.al index 365cc57..11e914f 100644 --- a/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasDiagnostic/UnsortedMixedTypes.al +++ b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasDiagnostic/UnsortedMixedTypes.al @@ -24,8 +24,8 @@ permissionset 50100 "My Permission Set" Assignable = false; Access = Public; - [|Permissions = tabledata "My Table" = R, - codeunit "My Codeunit" = X, + [|Permissions = codeunit "My Codeunit" = X, + report "My Report" = X, page "My Page" = X, - report "My Report" = X|]; + tabledata "My Table" = R|]; } diff --git a/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasDiagnostic/UnsortedNumeric.al b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasDiagnostic/UnsortedNumeric.al new file mode 100644 index 0000000..a41a719 --- /dev/null +++ b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasDiagnostic/UnsortedNumeric.al @@ -0,0 +1,20 @@ +codeunit 50100 "Item 10" +{ +} + +codeunit 50101 "Item 2" +{ +} + +codeunit 50102 "Item 1" +{ +} + +permissionset 50100 "My Permission Set" +{ + Assignable = true; + [|Permissions = + codeunit "Item 1" = X, + codeunit "Item 10" = X, + codeunit "Item 2" = X|]; +} diff --git a/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasDiagnostic/UnsortedTableNotFirst.al b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasDiagnostic/UnsortedTableNotFirst.al new file mode 100644 index 0000000..6c9bb6e --- /dev/null +++ b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasDiagnostic/UnsortedTableNotFirst.al @@ -0,0 +1,20 @@ +table 50100 "My Table" +{ + Caption = '', Locked = true; + fields + { + field(1; MyField; Integer) { } + } +} + +codeunit 50100 "My Codeunit" +{ +} + +permissionset 50100 "My Permission Set" +{ + Assignable = true; + [|Permissions = + codeunit "My Codeunit" = X, + tabledata "My Table" = R|]; +} diff --git a/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasFix/ReorderMixedTypes/current.al b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasFix/ReorderMixedTypes/current.al index 365cc57..11e914f 100644 --- a/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasFix/ReorderMixedTypes/current.al +++ b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasFix/ReorderMixedTypes/current.al @@ -24,8 +24,8 @@ permissionset 50100 "My Permission Set" Assignable = false; Access = Public; - [|Permissions = tabledata "My Table" = R, - codeunit "My Codeunit" = X, + [|Permissions = codeunit "My Codeunit" = X, + report "My Report" = X, page "My Page" = X, - report "My Report" = X|]; + tabledata "My Table" = R|]; } diff --git a/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasFix/ReorderMixedTypes/expected.al b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasFix/ReorderMixedTypes/expected.al index 3de3a7f..e91ff28 100644 --- a/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasFix/ReorderMixedTypes/expected.al +++ b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasFix/ReorderMixedTypes/expected.al @@ -24,8 +24,8 @@ permissionset 50100 "My Permission Set" Assignable = false; Access = Public; - Permissions = codeunit "My Codeunit" = X, + Permissions = tabledata "My Table" = R, + codeunit "My Codeunit" = X, page "My Page" = X, - report "My Report" = X, - tabledata "My Table" = R; + report "My Report" = X; } diff --git a/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/NoDiagnostic/AlreadySorted.al b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/NoDiagnostic/AlreadySorted.al index 302c53d..565ebce 100644 --- a/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/NoDiagnostic/AlreadySorted.al +++ b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/NoDiagnostic/AlreadySorted.al @@ -24,8 +24,8 @@ permissionset 50100 "My Permission Set" Assignable = false; Access = Public; - Permissions = codeunit "My Codeunit" = X, + Permissions = tabledata Alpha = R, + codeunit "My Codeunit" = X, page "My Page" = X, - report "My Report" = X, - tabledata Alpha = R; + report "My Report" = X; } diff --git a/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/NoDiagnostic/DotsBeforeLetters.al b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/NoDiagnostic/DotsBeforeLetters.al new file mode 100644 index 0000000..5589491 --- /dev/null +++ b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/NoDiagnostic/DotsBeforeLetters.al @@ -0,0 +1,15 @@ +codeunit 50100 "Post. Appr. Setup Wiz Hlp" +{ +} + +codeunit 50101 "Post Inv. Appr. Setup Hlp" +{ +} + +[||]permissionset 50100 GeneratedPermission +{ + Assignable = true; + Permissions = + codeunit "Post. Appr. Setup Wiz Hlp" = X, + codeunit "Post Inv. Appr. Setup Hlp" = X; +} diff --git a/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/NoDiagnostic/NaturalNumericSort.al b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/NoDiagnostic/NaturalNumericSort.al new file mode 100644 index 0000000..dcaa9fe --- /dev/null +++ b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/NoDiagnostic/NaturalNumericSort.al @@ -0,0 +1,20 @@ +codeunit 50100 "Item 2" +{ +} + +codeunit 50101 "Item 10" +{ +} + +codeunit 50102 "Item 1" +{ +} + +[||]permissionset 50100 "My Permission Set" +{ + Assignable = true; + Permissions = + codeunit "Item 1" = X, + codeunit "Item 2" = X, + codeunit "Item 10" = X; +} diff --git a/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/NoDiagnostic/TableTypesFirst.al b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/NoDiagnostic/TableTypesFirst.al new file mode 100644 index 0000000..0b630db --- /dev/null +++ b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/NoDiagnostic/TableTypesFirst.al @@ -0,0 +1,26 @@ +[||]table 50100 "My Table" +{ + Caption = '', Locked = true; + fields + { + field(1; MyField; Integer) { } + } +} + +codeunit 50100 "My Codeunit" +{ +} + +page 50100 "My Page" +{ +} + +permissionset 50100 "My Permission Set" +{ + Assignable = true; + Permissions = + table "My Table" = X, + tabledata "My Table" = R, + codeunit "My Codeunit" = X, + page "My Page" = X; +} diff --git a/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/PermissionDeclarationOrder.cs b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/PermissionDeclarationOrder.cs index 17d5b67..b360615 100644 --- a/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/PermissionDeclarationOrder.cs +++ b/src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/PermissionDeclarationOrder.cs @@ -25,6 +25,8 @@ public void Setup() [TestCase("UnsortedMixedTypes")] [TestCase("UnsortedSingleType")] [TestCase("UnsortedCodeunit")] + [TestCase("UnsortedNumeric")] + [TestCase("UnsortedTableNotFirst")] public async Task HasDiagnostic(string testCase) { var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(HasDiagnostic), $"{testCase}.al")) @@ -38,6 +40,9 @@ public async Task HasDiagnostic(string testCase) [TestCase("SingleEntry")] [TestCase("NoPermissionsProperty")] [TestCase("SortedTabledata")] + [TestCase("DotsBeforeLetters")] + [TestCase("NaturalNumericSort")] + [TestCase("TableTypesFirst")] public async Task NoDiagnostic(string testCase) { var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(NoDiagnostic), $"{testCase}.al"))