New Rule0088: Avoid option types

BusinessCentral.LinterCop.Test/Rule0088.cs

@@ -0,0 +1,49 @@
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+
+namespace BusinessCentral.LinterCop.Test
+{
+    internal class Rule0088
+    {
+        private string _testCaseDir = "";
+
+        [SetUp]
+        public void Setup()
+        {
+            _testCaseDir = Path.Combine(Directory.GetParent(Environment.CurrentDirectory)!.Parent!.Parent!.FullName,
+                "TestCases", "Rule0088");
+        }
+
+        [Test]
+        [TestCase("OptionField")]
+        [TestCase("OptionParameterGlobalVar")]
+        [TestCase("OptionParameterLocalVar")]
+        [TestCase("OptionReturnValue")]
+        [TestCase("OptionVariable")]
+        public async Task HasDiagnostic(string testCase)
+        {
+            var code = await File.ReadAllTextAsync(Path.Combine(_testCaseDir, "HasDiagnostic", $"{testCase}.al"))
+                .ConfigureAwait(false);
+
+            var fixture = RoslynFixtureFactory.Create<Rule0088AvoidOptionTypes>();
+            fixture.HasDiagnostic(code, DiagnosticDescriptors.Rule0088AvoidOptionTypes.Id);
+        }
+
+        [Test]
+        [TestCase("EventSubscriberOption")]
+        [TestCase("ObsoleteFieldOption")]
+        [TestCase("CDSDocument")]
+        // [TestCase("OptionParameter")] //TODO: See remarks in the test file
+        public async Task NoDiagnostic(string testCase)
+        {
+            var code = await File.ReadAllTextAsync(Path.Combine(_testCaseDir, "NoDiagnostic", $"{testCase}.al"))
+                .ConfigureAwait(false);
+
+            var fixture = RoslynFixtureFactory.Create<Rule0088AvoidOptionTypes>();
+            fixture.NoDiagnosticAtMarker(code, DiagnosticDescriptors.Rule0088AvoidOptionTypes.Id);
+        }
+    }
+}

BusinessCentral.LinterCop.Test/TestCases/Rule0088/HasDiagnostic/OptionField.al

@@ -0,0 +1,13 @@
+table 50100 "My Table"
+{
+    fields
+    {
+        field(1; "Test Option"; [|Option|])
+        {
+            Caption = 'Test Option';
+            DataClassification = CustomerContent;
+            OptionCaption = '0,1,2,3,4,5';
+            OptionMembers = "0","1","2","3","4","5";
+        }    
+    }
+}

BusinessCentral.LinterCop.Test/TestCases/Rule0088/HasDiagnostic/OptionParameterGlobalVar.al

@@ -0,0 +1,19 @@
+codeunit 50100 MyCodeunit
+{
+    var
+    [|Mode|]: Option "None","Allow deletion",Match;
+
+    procedure MyProcedure()
+    var
+        ReservationManagement: Codeunit "Reservation Management PTE";
+    begin
+        ReservationManagement.SetItemTrackingHandling(Mode);
+    end;
+}
+
+codeunit 50101 "Reservation Management PTE"
+{
+    procedure SetItemTrackingHandling(Mode: Option "None","Allow deletion",Match)
+    begin
+    end;
+}

BusinessCentral.LinterCop.Test/TestCases/Rule0088/HasDiagnostic/OptionParameterLocalVar.al

@@ -0,0 +1,17 @@
+codeunit 50100 MyCodeunit
+{
+    procedure MyProcedure()
+    var
+        ReservationManagement: Codeunit "Reservation Management PTE";
+        [|Mode|]: Option "None","Allow deletion",Match;
+    begin
+        ReservationManagement.SetItemTrackingHandling(Mode);
+    end;
+}
+
+codeunit 50101 "Reservation Management PTE"
+{
+    procedure SetItemTrackingHandling(Mode: Option "None","Allow deletion",Match)
+    begin
+    end;
+}

BusinessCentral.LinterCop.Test/TestCases/Rule0088/HasDiagnostic/OptionReturnValue.al

@@ -0,0 +1,7 @@
+codeunit 50100 MyCodeunit
+{
+    procedure MyProcedure() Mode: [|Option "None","Allow deletion",Match|]
+    begin
+
+    end;
+}

BusinessCentral.LinterCop.Test/TestCases/Rule0088/HasDiagnostic/OptionVariable.al

@@ -0,0 +1,14 @@
+codeunit 50100 MyCodeunit
+{
+    procedure MyProcedure([|TestOption: Option " ","Test1","Test2"|])
+    begin
+        case TestOption of
+            TestOption::" ":
+                exit;
+            TestOption::"Test1":
+                Message('test 1');
+            TestOption::"Test2":
+                Message('test 2');
+        end;
+    end;
+}

BusinessCentral.LinterCop.Test/TestCases/Rule0088/NoDiagnostic/CDSDocument.al

@@ -0,0 +1,37 @@
+table 50100 "Dataverse Project PTE"
+{
+    ExternalName = 'prefix_project';
+    TableType = CDS;
+    Description = '';
+    Caption = 'Project';
+
+    fields
+    {
+        field(1; prefix_projectId; Guid)
+        {
+            ExternalName = 'prefix_projectid';
+            ExternalType = 'Uniqueidentifier';
+            ExternalAccess = Insert;
+            Description = 'Unique identifier for entity instances';
+            Caption = 'Project';
+        }
+        field(2; prefix_name; Text[100])
+        {
+            ExternalName = 'prefix_name';
+            ExternalType = 'String';
+            Description = 'The name of the custom entity.';
+            Caption = 'Name';
+        }
+        field(25; statecode; [|Option|])
+        {
+            ExternalName = 'statecode';
+            ExternalType = 'State';
+            ExternalAccess = Modify;
+            Description = 'Status of the Project';
+            Caption = 'Status';
+            InitValue = " ";
+            OptionMembers = " ",Active,Inactive;
+            OptionOrdinalValues = -1, 0, 1;
+        }
+    }
+}

BusinessCentral.LinterCop.Test/TestCases/Rule0088/NoDiagnostic/EventSubscriberOption.al

@@ -0,0 +1,23 @@
+codeunit 50100 MyCodeunit
+{
+    trigger OnRun()
+    var
+        OptionVariable: Option "", "test1";
+    begin
+        OnCodeunitRun(OptionVariable);
+    end;
+
+    [IntegrationEvent(false, false)]
+    local procedure OnCodeunitRun(var OptionVariable: Option "", "test1")
+    begin
+    end;
+}
+
+codeunit 50101 MySubscriber
+{
+    [EventSubscriber(ObjectType::Codeunit, Codeunit::MyCodeunit, OnCodeunitRun, '', false, false)]
+    local procedure OnCodeunitRun_MyCodeunit(var [|OptionVariable: Option "", "test1"|])
+    begin
+
+    end;
+}

BusinessCentral.LinterCop.Test/TestCases/Rule0088/NoDiagnostic/ObsoleteFieldOption.al

@@ -0,0 +1,18 @@
+table 50100 "My Table"
+{
+    fields
+    {
+        field(1; MyField; Integer)
+        {
+            DataClassification = ToBeClassified;
+        }
+        field(2; "Test Option"; [|Option|])
+        {
+            Caption = 'Test Option';
+            ObsoleteState = Pending;
+            DataClassification = CustomerContent;
+            OptionCaption = '0,1,2,3,4,5';
+            OptionMembers = "0","1","2","3","4","5";
+        }    
+    }
+}

BusinessCentral.LinterCop.Test/TestCases/Rule0088/NoDiagnostic/OptionParameter.al

@@ -0,0 +1,15 @@
+//TODO: Test with an codeunit from a dependency extension so the ."GetLocation().IsInSource" returns false
+codeunit 50100 MyCodeunit
+{
+    var
+        ModeGlobal: Option "None","Allow deletion",Match; // Should not raise a diagnostic
+
+    procedure MyProcedure()
+    var
+        ReservationManagement: Codeunit "Reservation Management";
+        ModeLocal: Option "None","Allow deletion",Match; // Should not raise a diagnostic
+    begin
+        ReservationManagement.SetItemTrackingHandling(ModeLocal);
+        ReservationManagement.SetItemTrackingHandling(ModeGlobal);
+    end;
+}

BusinessCentral.LinterCop/Design/Rule0088AvoidOptionTypes.cs

@@ -0,0 +1,128 @@
+using System.Collections.Immutable;
+using BusinessCentral.LinterCop.Helpers;
+using Microsoft.Dynamics.Nav.CodeAnalysis;
+using Microsoft.Dynamics.Nav.CodeAnalysis.Diagnostics;
+using Microsoft.Dynamics.Nav.CodeAnalysis.Symbols;
+using Microsoft.Dynamics.Nav.CodeAnalysis.Syntax;
+using Microsoft.Dynamics.Nav.CodeAnalysis.Utilities;
+
+namespace BusinessCentral.LinterCop.Design;
+
+[DiagnosticAnalyzer]
+public class Rule0088AvoidOptionTypes : DiagnosticAnalyzer
+{
+    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } =
+        ImmutableArray.Create(DiagnosticDescriptors.Rule0088AvoidOptionTypes);
+
+    public override void Initialize(AnalysisContext context)
+    {
+        context.RegisterSyntaxNodeAction(
+            new Action<SyntaxNodeAnalysisContext>(this.AnalyzeSyntaxNodes),
+            new SyntaxKind[]{
+                    SyntaxKind.OptionDataType
+            }
+        );
+
+        context.RegisterSymbolAction(
+            new Action<SymbolAnalysisContext>(this.AnalyzeVariables),
+            SymbolKind.GlobalVariable,
+            SymbolKind.LocalVariable);
+    }
+
+    private void AnalyzeSyntaxNodes(SyntaxNodeAnalysisContext ctx)
+    {
+        if (ctx.IsObsoletePendingOrRemoved() || ctx.Node is not OptionDataTypeSyntax optionDataType)
+        {
+            return;
+        }
+
+        if (optionDataType.Parent is SimpleTypeReferenceSyntax && !IsParameterOrReturnValue(optionDataType) ||
+             ctx.ContainingSymbol is IMethodSymbol method && method.IsEventSubscriber() ||
+             ctx.ContainingSymbol.GetContainingApplicationObjectTypeSymbol() is ITableTypeSymbol table && table.TableType == TableTypeKind.CDS)
+        {
+            return;
+        }
+
+        ctx.ReportDiagnostic(Diagnostic.Create(
+            DiagnosticDescriptors.Rule0088AvoidOptionTypes,
+            optionDataType.GetLocation()
+        ));
+    }
+
+    private static bool IsParameterOrReturnValue(OptionDataTypeSyntax optionDataType)
+    {
+        var parent = optionDataType.Parent?.Parent;
+        return parent is not null && (parent.Kind == SyntaxKind.Parameter || parent.Kind == SyntaxKind.ReturnValue);
+    }
+
+    private void AnalyzeVariables(SymbolAnalysisContext ctx)
+    {
+        if (ctx.IsObsoletePendingOrRemoved() || ctx.Symbol is not IVariableSymbol variable)
+            return;
+
+        if (variable.Type.GetNavTypeKindSafe() != NavTypeKind.Option)
+            return;
+
+        bool? HasVariablesNotInSource = null;
+        switch (variable.Kind)
+        {
+            case SymbolKind.LocalVariable:
+                ISymbol? containingSymbol = variable.ContainingSymbol;
+                if (containingSymbol is null)
+                    return;
+
+                var LocalVariablesName = GetReferencedVariableNames(containingSymbol, variable);
+                HasVariablesNotInSource = ((IMethodSymbol)containingSymbol.OriginalDefinition).LocalVariables
+                                                                .Where(var => !var.Type.GetLocation().IsInSource)
+                                                                .Where(var => LocalVariablesName?.Contains(var.OriginalDefinition.Name) == true)
+                                                                .Any();
+                break;
+
+            case SymbolKind.GlobalVariable:
+                IApplicationObjectTypeSymbol? applicationObjectTypeSymbol = variable.GetContainingApplicationObjectTypeSymbol();
+                if (applicationObjectTypeSymbol is null)
+                    return;
+
+                var GlobalVariablesName = GetReferencedVariableNames(applicationObjectTypeSymbol, variable);
+                HasVariablesNotInSource = applicationObjectTypeSymbol.GetMembers()
+                    .Where(member => member.Kind == SymbolKind.GlobalVariable || member.Kind == SymbolKind.Method)
+                    .SelectMany(member => member is IMethodSymbol methodSymbol
+                        ? methodSymbol.LocalVariables
+                        : member is IVariableSymbol variableSymbol ? Enumerable.Repeat(variableSymbol, 1) : Enumerable.Empty<IVariableSymbol>())
+                    .Where(var => !var.Type.GetLocation().IsInSource)
+                    .Where(var => GlobalVariablesName?.Contains(var.OriginalDefinition.Name) == true)
+                    .Any();
+                break;
+        }
+
+        if (HasVariablesNotInSource == false)
+        {
+            ctx.ReportDiagnostic(Diagnostic.Create(
+                DiagnosticDescriptors.Rule0088AvoidOptionTypes,
+                variable.GetLocation()
+            ));
+        }
+    }
+
+    private static IEnumerable<string>? GetReferencedVariableNames(ISymbol? containingSymbol, IVariableSymbol variable)
+    {
+        SyntaxNode? syntaxNode = containingSymbol?.DeclaringSyntaxReference?.GetSyntax();
+        if (syntaxNode is null)
+            return null;
+
+        var nodes = syntaxNode.DescendantNodes()
+            .OfType<ArgumentListSyntax>()
+            .Where(argList => argList.Arguments.Any(argument =>
+                (argument is OptionAccessExpressionSyntax optionAccess &&
+                 optionAccess.Expression.GetIdentifierOrLiteralValue() == variable.Name) ||
+                argument.GetIdentifierOrLiteralValue() == variable.Name));
+
+        return nodes
+            .SelectMany(node => node.AncestorsAndSelf()
+                .OfType<ExpressionStatementSyntax>()
+                .SelectMany(exprStmt => exprStmt.DescendantNodes()
+                    .OfType<MemberAccessExpressionSyntax>()
+                    .Select(memberAccess => memberAccess.Expression.ToString().UnquoteIdentifier())))
+            .Distinct();
+    }
+}

BusinessCentral.LinterCop/LinterCop.ruleset.json

@@ -436,6 +436,11 @@
       "id": "LC0087",
       "action": "Warning",
       "justification": "Use IsNullGuid() to check for empty GUID values."
+    },
+    {
+      "id": "LC0088",
+      "action": "Info",
+      "justification": "Avoid option types to get rid of some of the bad patterns that come along with option fields."
     }
   ]
 }

BusinessCentral.LinterCop/LinterCopAnalyzers.Generated.cs

@@ -894,4 +894,14 @@ public static class DiagnosticDescriptors
         isEnabledByDefault: true,
         description: LinterCopAnalyzers.GetLocalizableString("Rule0087UseIsNullGuidDescription"),
         helpLinkUri: "https://github.com/StefanMaron/BusinessCentral.LinterCop/wiki/LC0087");
+
+    public static readonly DiagnosticDescriptor Rule0088AvoidOptionTypes = new(
+        id: LinterCopAnalyzers.AnalyzerPrefix + "0088",
+        title: LinterCopAnalyzers.GetLocalizableString("Rule0088AvoidOptionTypesTitle"),
+        messageFormat: LinterCopAnalyzers.GetLocalizableString("Rule0088AvoidOptionTypesFormat"),
+        category: "Design",
+        defaultSeverity: DiagnosticSeverity.Info,
+        isEnabledByDefault: true,
+        description: LinterCopAnalyzers.GetLocalizableString("Rule0088AvoidOptionTypesDescription"),
+        helpLinkUri: "https://github.com/StefanMaron/BusinessCentral.LinterCop/wiki/LC0088");
 }