[tkroes]

Summary

Moves the TryFunction database-write diagnostics from LinterCop to PlatformCop and separates direct writes from calls that may write.

ID	Behavior	Severity	Default
PC0040	Reports database-write built-ins on resolved, non-temporary record/table receivers.	Warning	Enabled
PC0041	Reports custom procedure calls and selected built-ins that may write.	Info	Disabled; opt-in via ruleset

PC0040 recognizes calls written with or without parentheses and avoids same-named non-record built-ins such as List.Insert, TextBuilder.Insert and HttpClient.Delete. Both diagnostics skip obsolete TryFunctions and code outside TryFunctions. Temporary records, temporary tables, and temporary page source tables are not reported as direct writes. RecordRef writes are intentionally skipped because temporary status is only known at runtime.

Validation

• PlatformCop tests: 665 passed.
• LinterCop tests: 409 passed.
• PlatformCop and LinterCop Release builds passed for netstandard2.1, net8.0 and net10.0, with zero warnings/errors.
• dotnet format ALCops.sln --verify-no-changes: passed.
• Validate-Rules.ps1: passed.
• git diff --check: passed.

Documentation

Companion rule pages and PlatformCop index updates: ALCops/alcops.dev#213.

[MODUSCarstenScholling]

Sorry if I'm rushing through this. Just back from vacation today.
To avoid noise, would you consider the possible noisy LC0102 to be disabled by default? And reduce the Error to a Warning? Because there could be code that relies on DisableWriteInsideTryFunctions = false.

Just to consider.

[tkroes]

@MODUSCarstenScholling I will do that, these were just the defaults for our internal linters, but it makes more sense to keep these fairly low profile and only ramp them up using rulesets. Hope to get around to fixing this and my other PR sometime this week.

[Arthurvdv]

Great work, this is an long requested diagnostic to have, see #533.

I ran a code reviews on the PR and checked the findings against the code and the Microsoft docs. Below is what I'd like to discuss before we merge. The biggest point is where the rule lives; most of the other points follow from that.

1. Move the rule to PlatformCop

LinterCop is scoped to "non-breaking code smells … where multiple valid options exist". A write inside a TryFunction isn't a style choice. It's a runtime-semantics hazard, so it belongs in PlatformCop: "AL language and runtime semantic correctness, preventing patterns that always fail or behave unpredictably … independent of the Business Central domain model". It isn't ApplicationCop either, because nothing here depends on how tables, pages or permissions are modelled.

From Handling errors using try methods:

• Database changes made inside a try method are not rolled back when the error is caught.
• On-premises, DisableWriteInsideTryFunctions defaults to true, so the write itself causes a runtime error.
• Online there is no restriction. The write succeeds and stays committed even when the TryFunction returns false.

It's "fails or behaves unpredictably" rather than "always fails", which fits PlatformCop.

Concretely:

• Rename and renumber both dianostics
• Keep the severity at Warning, enabled by default, but only after the false positives below are fixed. A PlatformCop warning says "this is a bug", so false positives cost more there.
• The message currently says the method "must not be called from a TryFunction". That's only true on-prem. Something like "'{0}' writes to the database inside a TryFunction; the change is not rolled back when the error is caught" describes both environments.

2. Disable LC0102 (the "potential modification" variant) by default

LC0102 reports every call to a non-built-in method inside a TryFunction (MyHelper(), IsValid(Rec), event publishers, interface calls, …), plus Commit, Validate, Run and others. There's no signal that tells a pure helper apart from one that writes, so nearly every report is noise. The severity commit set it to Hidden while keeping isEnabledByDefault: true. Hidden diagnostics don't show up in the editor or the alc output, so the rule still runs on every call but nobody sees the result. Yo can set the default severity to Info and isEnabledByDefault: false, so user should opt-in for this diagnoastic.

3. False positives: temporary records and non-record built-ins

The analyzer classifies by method name and MethodKind only, and never looks at the receiver. That produces two kinds of false positive:

codeunit 50100 MyCodeunit
{
    [TryFunction]
    local procedure TryBuildBuffer()
    var
        TempBuffer: Record MyTable temporary;
        Names: List of [Text];
        Builder: TextBuilder;
    begin
        TempBuffer.Insert();        // reported, but a temporary record never touches the database
        TempBuffer.DeleteAll();     // reported, same
        Names.Insert(1, 'x');       // reported: List.Insert is a BuiltInMethod named "Insert"
        Builder.Insert(1, 'x');     // reported: TextBuilder.Insert, same
    end;
}

Records whose table has TableType = Temporary have the same problem.

Common already has what's needed:

• GetReceiverTableType(...) in OperationExtensions resolves the receiver for every form: MyRec.Insert(), Rec.Insert(), this.Insert() and a bare Insert() in a table. It returns null when the receiver isn't a record (List, TextBuilder, HttpClient, …).
• IRecordTypeSymbol.IsTemporary() / ITableTypeSymbol.IsTemporary() cover both the temporary keyword and TableType = Temporary.

RecordRef (RecRef.Insert()) isn't a record receiver for this helper, so it would not be reported. Whether a RecordRef is temporary is only known at runtime, so I'd keep that as a documented limitation rather than report it and risk false positives.

4. False negative: calls without parentheses

AL lets you write MyRecord.Insert;, MyRecord.DeleteAll; or a bare Modify; without parentheses. For those the parser produces a MemberAccessExpressionSyntax (or a lone IdentifierNameSyntax) instead of an InvocationExpressionSyntax. The current DescendantNodes().OfType<InvocationExpressionSyntax>() walk never sees them, so the rule silently misses these writes.

Walking operations instead of syntax fixes this: IInvocationExpression represents both spellings. It's also what GetReceiverTableType expects, so points 3 and 4 have the same fix (see the code under 5).

5. Simplify the analyzer

A few smaller things in AnalyzeCodeBlock:

• ctx.CodeBlock is not MethodOrTriggerDeclarationSyntax methodSyntax || methodSyntax is not MethodDeclarationSyntax methodDeclaration matches the same node twice, and methodSyntax isn't used afterwards. A single property pattern does the same job.
• IsDirectDatabaseModificationMethod checks up to four hash sets on every invocation. Build one set once in the static initializer instead. ImmutableHashSet.Union keeps the SemanticFacts.NameEqualityComparer of the source set.
• With LC0102 gone, the PotentialDatabaseModificationMethods set and the second branch disappear.

Put together with 3 and 4, the analyzer could look roughly like this (sketchy and untested):

using System.Collections.Immutable;
using ALCops.Common;
using ALCops.Common.Extensions;
using ALCops.Common.Reflection;
using Microsoft.Dynamics.Nav.CodeAnalysis;
using Microsoft.Dynamics.Nav.CodeAnalysis.Diagnostics;
using Microsoft.Dynamics.Nav.CodeAnalysis.Semantics;
using Microsoft.Dynamics.Nav.CodeAnalysis.Syntax;

namespace ALCops.PlatformCop.Analyzers;

[DiagnosticAnalyzer]
public sealed class TryFunctionDatabaseWrite : DiagnosticAnalyzer
{
    private static readonly ImmutableHashSet<string> DatabaseWriteMethods =
        RecordMethodClassification.InsertMethods
            .Union(RecordMethodClassification.ModifyMethods)
            .Union(RecordMethodClassification.DeleteMethods)
            .Union(new[] { "Truncate", "AddLink", "CopyLinks", "DeleteLink", "DeleteLinks" });

    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } =
        ImmutableArray.Create(DiagnosticDescriptors.TryFunctionDatabaseWrite);

    public override void Initialize(AnalysisContext context) =>
        context.RegisterCodeBlockAction(AnalyzeCodeBlock);

    private static void AnalyzeCodeBlock(CodeBlockAnalysisContext ctx)
    {
        if (ctx.IsObsolete()
            || ctx.CodeBlock is not MethodDeclarationSyntax { Body: { } body } method
            || !method.IsTryFunction()
            || ctx.SemanticModel.GetOperation(body, ctx.CancellationToken) is not { } operation)
        {
            return;
        }

        new DatabaseWriteWalker(ctx).Visit(operation);
    }

    private sealed class DatabaseWriteWalker : OperationWalker
    {
        private readonly CodeBlockAnalysisContext _ctx;

        public DatabaseWriteWalker(CodeBlockAnalysisContext ctx) => _ctx = ctx;

        // IInvocationExpression covers both MyRec.Insert() and MyRec.Insert (no parentheses).
        public override void VisitInvocationExpression(IInvocationExpression operation)
        {
            _ctx.CancellationToken.ThrowIfCancellationRequested();

            if (operation.TargetMethod.MethodKind == EnumProvider.MethodKind.BuiltInMethod
                && DatabaseWriteMethods.Contains(operation.TargetMethod.Name)
                // null for non-record receivers (List, TextBuilder, HttpClient, RecordRef, ...)
                && operation.GetReceiverTableType(_ctx.OwningSymbol, out var recordType) is { } table
                // temporary records never write to the database
                && !table.IsTemporary()
                && recordType?.IsTemporary() != true)
            {
                _ctx.ReportDiagnostic(Diagnostic.Create(
                    DiagnosticDescriptors.TryFunctionDatabaseWrite,
                    operation.Syntax.GetLocation(),
                    operation.TargetMethod.Name));
            }

            base.VisitInvocationExpression(operation);
        }
    }
}

6. Tests

Add regression fixtures for the cases above, so they can't come back:

• NoDiagnostic/TemporaryRecord.al: temporary variable and a TableType = Temporary table.
• NoDiagnostic/NonRecordBuiltIns.al: List.Insert, TextBuilder.Insert, HttpClient.Delete.
• HasDiagnostic/WithoutParentheses.al: MyRecord.Insert;, MyRecord.DeleteAll;.
• HasDiagnostic/ImplicitRec.al: a bare Insert() / Modify; inside a TryFunction in a table.

For example:

codeunit 50100 TryFuncTemporaryRecord
{
    [TryFunction]
    local procedure TryFillBuffer()
    var
        TempMyRecord: Record MyTable temporary;
        MyTempTable: Record MyTempTable;
    begin
        [|TempMyRecord.Insert()|];
        [|TempMyRecord.DeleteAll()|];
        [|MyTempTable.Insert()|];
    end;
}

table 50100 MyTable
{
    fields { field(1; MyField; Integer) { } }
}

table 50101 MyTempTable
{
    TableType = Temporary;
    fields { field(1; MyField; Integer) { } }
}

codeunit 50100 TryFuncWithoutParentheses
{
    [TryFunction]
    local procedure TryWork()
    var
        MyRecord: Record MyTable;
    begin
        [|MyRecord.Insert|];
        [|MyRecord.DeleteAll|];
    end;
}

table 50100 MyTable
{
    fields { field(1; MyField; Integer) { } }
}

7. Housekeeping

• Rebase on main. The branch starts before fix(ci): fail the test job on compile errors and bump RoslynTestKit to 1.5.1 #592 was merged, so the build-test.yml, Directory.Packages.props, Setup-BCDevTools.ps1, get-bc-devtools.md and release-strategy.md hunks show up in this PR. Their content is already on main, so after a rebase they drop out and the PR only contains the rule.
• Commit message. XXXX: Set TryFunction diagnostic severities still has the placeholder prefix. With the move it'll be squashed anyway; the PR title should become something like feat(PC0040): report database writes inside TryFunctions.
• Docs page. Every rule needs a page in alcops.dev at content/docs/analyzers/PlatformCop/PC0040.md, otherwise the help link 404s. Please open that as a companion PR and link it here.

Happy to help with any of this of you need any help, feel free to reach out. Again thank you for this great rule 🤗

[tkroes]

Companion documentation PR for PC0040/PC0041: ALCops/alcops.dev#213. It adds both rule pages and updates the PlatformCop index.