diff --git a/src/ALCops.ApplicationCop.Test/Rules/UseReturnValueForDatabaseReadMethods/HasDiagnostic/GetBySystemIdMethod.al b/src/ALCops.ApplicationCop.Test/Rules/UseReturnValueForDatabaseReadMethods/HasDiagnostic/GetBySystemIdMethod.al new file mode 100644 index 0000000..0840e5b --- /dev/null +++ b/src/ALCops.ApplicationCop.Test/Rules/UseReturnValueForDatabaseReadMethods/HasDiagnostic/GetBySystemIdMethod.al @@ -0,0 +1,18 @@ +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + MyTable: Record MyTable; + MyGuid: Guid; + begin + MyTable.[|GetBySystemId|](MyGuid); + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Code[20]) { } + } +} \ No newline at end of file diff --git a/src/ALCops.ApplicationCop.Test/Rules/UseReturnValueForDatabaseReadMethods/HasDiagnostic/GetMethod.al b/src/ALCops.ApplicationCop.Test/Rules/UseReturnValueForDatabaseReadMethods/HasDiagnostic/GetMethod.al new file mode 100644 index 0000000..2aa2e74 --- /dev/null +++ b/src/ALCops.ApplicationCop.Test/Rules/UseReturnValueForDatabaseReadMethods/HasDiagnostic/GetMethod.al @@ -0,0 +1,17 @@ +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + MyTable: Record MyTable; + begin + MyTable.[|Get|](); + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Code[20]) { } + } +} \ No newline at end of file diff --git a/src/ALCops.ApplicationCop.Test/Rules/UseReturnValueForDatabaseReadMethods/NoDiagnostic/GetBySystemIdMethod.al b/src/ALCops.ApplicationCop.Test/Rules/UseReturnValueForDatabaseReadMethods/NoDiagnostic/GetBySystemIdMethod.al new file mode 100644 index 0000000..f6f577c --- /dev/null +++ b/src/ALCops.ApplicationCop.Test/Rules/UseReturnValueForDatabaseReadMethods/NoDiagnostic/GetBySystemIdMethod.al @@ -0,0 +1,18 @@ +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + MyTable: Record MyTable; + MyGuid: Guid; + begin + if MyTable.[|GetBySystemId|](MyGuid) then; + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Code[20]) { } + } +} \ No newline at end of file diff --git a/src/ALCops.ApplicationCop.Test/Rules/UseReturnValueForDatabaseReadMethods/NoDiagnostic/GetMethod.al b/src/ALCops.ApplicationCop.Test/Rules/UseReturnValueForDatabaseReadMethods/NoDiagnostic/GetMethod.al new file mode 100644 index 0000000..53446d0 --- /dev/null +++ b/src/ALCops.ApplicationCop.Test/Rules/UseReturnValueForDatabaseReadMethods/NoDiagnostic/GetMethod.al @@ -0,0 +1,17 @@ +codeunit 50100 MyCodeunit +{ + procedure MyProcedure() + var + MyTable: Record MyTable; + begin + if MyTable.[|Get|]() then; + end; +} + +table 50100 MyTable +{ + fields + { + field(1; MyField; Code[20]) { } + } +} \ No newline at end of file diff --git a/src/ALCops.ApplicationCop.Test/Rules/UseReturnValueForDatabaseReadMethods/UseReturnValueForDatabaseReadMethods.cs b/src/ALCops.ApplicationCop.Test/Rules/UseReturnValueForDatabaseReadMethods/UseReturnValueForDatabaseReadMethods.cs new file mode 100644 index 0000000..f388fa4 --- /dev/null +++ b/src/ALCops.ApplicationCop.Test/Rules/UseReturnValueForDatabaseReadMethods/UseReturnValueForDatabaseReadMethods.cs @@ -0,0 +1,43 @@ +using RoslynTestKit; + +namespace ALCops.ApplicationCop.Test +{ + public class UseReturnValueForDatabaseReadMethods : NavCodeAnalysisBase + { + private AnalyzerTestFixture _fixture; + private string _testCasePath; + + [SetUp] + public void Setup() + { + _fixture = RoslynFixtureFactory.Create(); + + _testCasePath = Path.Combine( + Directory.GetParent( + Environment.CurrentDirectory)!.Parent!.Parent!.FullName, + Path.Combine("Rules", nameof(UseReturnValueForDatabaseReadMethods))); + } + + [Test] + [TestCase("GetMethod")] + [TestCase("GetBySystemIdMethod")] + public async Task HasDiagnostic(string testCase) + { + var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(HasDiagnostic), $"{testCase}.al")) + .ConfigureAwait(false); + + _fixture.HasDiagnosticAtAllMarkers(code, DiagnosticIds.UseReturnValueForDatabaseReadMethods); + } + + [Test] + [TestCase("GetMethod")] + [TestCase("GetBySystemIdMethod")] + public async Task NoDiagnostic(string testCase) + { + var code = await File.ReadAllTextAsync(Path.Combine(_testCasePath, nameof(NoDiagnostic), $"{testCase}.al")) + .ConfigureAwait(false); + + _fixture.NoDiagnosticAtAllMarkers(code, DiagnosticIds.UseReturnValueForDatabaseReadMethods); + } + } +} \ No newline at end of file diff --git a/src/ALCops.ApplicationCop/ALCops.ApplicationCopAnalyzers.cs b/src/ALCops.ApplicationCop/ALCops.ApplicationCopAnalyzers.cs index e4535a1..d14da45 100644 --- a/src/ALCops.ApplicationCop/ALCops.ApplicationCopAnalyzers.cs +++ b/src/ALCops.ApplicationCop/ALCops.ApplicationCopAnalyzers.cs @@ -921,6 +921,33 @@ internal static string ToolTipShouldStartWithSpecifiesTitle { } } + /// + /// Looks up a localized string similar to Database read methods, like Record.Get(), returns a boolean indicating whether the record was successfully retrieved. Failing to use this return value can lead to uncaught errors, poor error handling, and a lack of actionable feedback for users when something goes wrong.. + /// + internal static string UseReturnValueForDatabaseReadMethodsDescription { + get { + return ResourceManager.GetString("UseReturnValueForDatabaseReadMethodsDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to The return value of the '{0}' method must be used to improve error handling or provide meaningful feedback to the user.. + /// + internal static string UseReturnValueForDatabaseReadMethodsMessageFormat { + get { + return ResourceManager.GetString("UseReturnValueForDatabaseReadMethodsMessageFormat", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Use return value for better error handling. + /// + internal static string UseReturnValueForDatabaseReadMethodsTitle { + get { + return ResourceManager.GetString("UseReturnValueForDatabaseReadMethodsTitle", resourceCulture); + } + } + /// /// Looks up a localized string similar to Zero (0) should be reserved as the empty Enum value. Business Central stores Enums as integers and does not support null; new records (and existing records after adding a field via table extension) default to 0, which makes non-empty meaning at 0 ambiguous.. /// diff --git a/src/ALCops.ApplicationCop/ALCops.ApplicationCopAnalyzers.resx b/src/ALCops.ApplicationCop/ALCops.ApplicationCopAnalyzers.resx index 0a5a6f8..d9d7f39 100644 --- a/src/ALCops.ApplicationCop/ALCops.ApplicationCopAnalyzers.resx +++ b/src/ALCops.ApplicationCop/ALCops.ApplicationCopAnalyzers.resx @@ -405,6 +405,15 @@ For field ToolTips, the Business Central user assistance model expects the text to start with the verb 'Specifies' to describe what the field represents. + + Use return value for better error handling + + + The return value of the '{0}' method must be used to improve error handling or provide meaningful feedback to the user. + + + Database read methods, like Record.Get(), returns a boolean indicating whether the record was successfully retrieved. Failing to use this return value can lead to uncaught errors, poor error handling, and a lack of actionable feedback for users when something goes wrong. + Reserve Enum value zero (0) for empty value diff --git a/src/ALCops.ApplicationCop/Analyzers/UseReturnValueForDatabaseReadMethods.cs b/src/ALCops.ApplicationCop/Analyzers/UseReturnValueForDatabaseReadMethods.cs new file mode 100644 index 0000000..99b352c --- /dev/null +++ b/src/ALCops.ApplicationCop/Analyzers/UseReturnValueForDatabaseReadMethods.cs @@ -0,0 +1,55 @@ +using System.Collections.Immutable; +using ALCops.Common.Extensions; +using ALCops.Common.Reflection; +using Microsoft.Dynamics.Nav.CodeAnalysis; +using Microsoft.Dynamics.Nav.CodeAnalysis.Diagnostics; +using Microsoft.Dynamics.Nav.CodeAnalysis.Syntax; + +namespace ALCops.ApplicationCop.Analyzers; + +[DiagnosticAnalyzer] +public class UseReturnValueForDatabaseReadMethods : DiagnosticAnalyzer +{ + public override ImmutableArray SupportedDiagnostics { get; } = + ImmutableArray.Create( + DiagnosticDescriptors.UseReturnValueForDatabaseReadMethods); + + private static readonly ImmutableHashSet DatabaseReadMethods = + ImmutableHashSet.Create( + StringComparer.OrdinalIgnoreCase, + "Find", + "FindFirst", + "FindLast", + "Get", + "GetBySystemId"); + + public override void Initialize(AnalysisContext context) => + context.RegisterOperationAction( + AnalyzeInvocation, + EnumProvider.OperationKind.InvocationExpression); + + private void AnalyzeInvocation(OperationAnalysisContext ctx) + { + if (ctx.IsObsolete() || ctx.Operation is not IInvocationExpression invocation) + return; + + if (invocation.TargetMethod.MethodKind != EnumProvider.MethodKind.BuiltInMethod || + invocation.TargetMethod.ContainingSymbol?.Name != "Table" || + !DatabaseReadMethods.Contains(invocation.TargetMethod.Name)) + return; + + if (ctx.Operation.Syntax.Parent.Kind == EnumProvider.SyntaxKind.ExpressionStatement) + { + var methodName = invocation.TargetMethod.Name; + if (invocation.Syntax is not InvocationExpressionSyntax invocationSyntax) + return; + + var location = invocationSyntax.Expression.GetLocation(); + + ctx.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.UseReturnValueForDatabaseReadMethods, + location, + methodName)); + } + } +} \ No newline at end of file diff --git a/src/ALCops.ApplicationCop/DiagnosticDescriptors.cs b/src/ALCops.ApplicationCop/DiagnosticDescriptors.cs index 79e0ac5..2567b25 100644 --- a/src/ALCops.ApplicationCop/DiagnosticDescriptors.cs +++ b/src/ALCops.ApplicationCop/DiagnosticDescriptors.cs @@ -285,6 +285,16 @@ public static class DiagnosticDescriptors description: ApplicationCopAnalyzers.ToolTipShouldStartWithSpecifiesDescription, helpLinkUri: GetHelpUri(DiagnosticIds.ToolTipShouldStartWithSpecifies)); + public static readonly DiagnosticDescriptor UseReturnValueForDatabaseReadMethods = new( + id: DiagnosticIds.UseReturnValueForDatabaseReadMethods, + title: ApplicationCopAnalyzers.UseReturnValueForDatabaseReadMethodsTitle, + messageFormat: ApplicationCopAnalyzers.UseReturnValueForDatabaseReadMethodsMessageFormat, + category: Category.Design, + defaultSeverity: DiagnosticSeverity.Info, + isEnabledByDefault: true, + description: ApplicationCopAnalyzers.UseReturnValueForDatabaseReadMethodsDescription, + helpLinkUri: GetHelpUri(DiagnosticIds.UseReturnValueForDatabaseReadMethods)); + public static readonly DiagnosticDescriptor ZeroEnumValueReservedForEmpty = new( id: DiagnosticIds.ZeroEnumValueReservedForEmpty, title: ApplicationCopAnalyzers.ZeroEnumValueReservedForEmptyTitle, diff --git a/src/ALCops.ApplicationCop/DiagnosticIds.cs b/src/ALCops.ApplicationCop/DiagnosticIds.cs index 1ab96c9..65efa6c 100644 --- a/src/ALCops.ApplicationCop/DiagnosticIds.cs +++ b/src/ALCops.ApplicationCop/DiagnosticIds.cs @@ -31,4 +31,5 @@ public static class DiagnosticIds public static readonly string LabelTokenRequiresTokSuffix = "AC0027"; public static readonly string TableFieldToolTipShouldBeDefined = "AC0028"; public static readonly string DuplicateToolTipBetweenPageAndTable = "AC0029"; + public static readonly string UseReturnValueForDatabaseReadMethods = "AC0030"; } \ No newline at end of file diff --git a/src/ALCops.Common/Reflection/EnumProvider.cs b/src/ALCops.Common/Reflection/EnumProvider.cs index b3667b6..673d639 100644 --- a/src/ALCops.Common/Reflection/EnumProvider.cs +++ b/src/ALCops.Common/Reflection/EnumProvider.cs @@ -1375,7 +1375,7 @@ public static class SyntaxKind private static readonly Lazy _lessThanToken = new(() => ParseEnum(nameof(NavCodeAnalysis.SyntaxKind.LessThanToken))); private static readonly Lazy _endOfLineTrivia = - new(() => ParseEnum(nameof(NavCodeAnalysis.SyntaxKind.EndOfLineTrivia))); + new(() => ParseEnum(nameof(NavCodeAnalysis.SyntaxKind.EndOfLineTrivia))); private static readonly Lazy _enumExtensionType = new(() => ParseEnum(nameof(NavCodeAnalysis.SyntaxKind.EnumExtensionType))); private static readonly Lazy _enumDataType = @@ -1388,6 +1388,8 @@ public static class SyntaxKind new(() => ParseEnum(nameof(NavCodeAnalysis.SyntaxKind.Entitlement))); private static readonly Lazy _exitStatement = new(() => ParseEnum(nameof(NavCodeAnalysis.SyntaxKind.ExitStatement))); + private static readonly Lazy _expressionStatement = + new(() => ParseEnum(nameof(NavCodeAnalysis.SyntaxKind.ExpressionStatement))); private static readonly Lazy _falseKeyword = new(() => ParseEnum(nameof(NavCodeAnalysis.SyntaxKind.FalseKeyword))); private static readonly Lazy _fieldKeyword = @@ -1640,6 +1642,7 @@ public static class SyntaxKind public static NavCodeAnalysis.SyntaxKind EnumValue => _enumValue.Value; public static NavCodeAnalysis.SyntaxKind Entitlement => _entitlement.Value; public static NavCodeAnalysis.SyntaxKind ExitStatement => _exitStatement.Value; + public static NavCodeAnalysis.SyntaxKind ExpressionStatement => _expressionStatement.Value; public static NavCodeAnalysis.SyntaxKind FalseKeyword => _falseKeyword.Value; public static NavCodeAnalysis.SyntaxKind FieldKeyword => _fieldKeyword.Value; public static NavCodeAnalysis.SyntaxKind ForKeyword => _forKeyword.Value; diff --git a/src/ALCops.LinterCop/ALCops.LinterCopAnalyzers.cs b/src/ALCops.LinterCop/ALCops.LinterCopAnalyzers.cs index 3628179..fd015d4 100644 --- a/src/ALCops.LinterCop/ALCops.LinterCopAnalyzers.cs +++ b/src/ALCops.LinterCop/ALCops.LinterCopAnalyzers.cs @@ -148,7 +148,7 @@ internal static string AppManifestRuntimeBehindTitle { } /// - /// Looks up a localized string similar to APClops: Use modern Date/Time/DateTime method. + /// Looks up a localized string similar to ALCops: Use modern Date/Time/DateTime method. /// internal static string BuiltInDateTimeMethodCodeAction { get {