[Bug]: PC0037 for OnValidate of the same field

[TKapitan]

Analyzer

PlatformCop

Issue type

False positive (diagnostic fires when it shouldn't)

Rule ID

PC0037

Description

The PC0037 should not be triggered for assignment to fields in their own OnValidate triggers (and also in OnBefore/After validate). This is always by design.

These cases includes

• default values
• value transformation (e.g. unit price in the base app and rounding etc.)

AL code to reproduce

No response

Additional context

No response

[Arthurvdv]

Thanks for raising this @TKapitan and this could be a exception we didn't considered. Back in #259 we did land on one exception for this rule; temporary record variables. What never came up there is exactly your case: a field assigned inside its own OnValidate trigger. Calling Validate() would only re-enter the same OnValidate we're already running, so the rule's suggestion feels indeed like a false positive.

The simple case is easy and cheap to detect. A direct assignment to a field, written directly inside that same field's OnValidate is easy identifiable at the syntax/operation level by walking up to the enclosing trigger and comparing the field:

table 50000 MyTable
{
    fields
    {
        field(1; PrimaryKey; Code[20]) { }
        field(2; PostingDate; Date)
        {
            trigger OnValidate()
            begin
                if Rec.PostingDate < 0D then
                    Rec.PostingDate := WorkDate(); // self-assignment in own OnValidate — should NOT fire
            end;
        }
    }
}

Where it gets hard is when the assignment is moved out of the trigger and into a helper method or codeunit:

table 50000 MyTable
{
    fields
    {
        field(1; PrimaryKey; Code[20]) { }
        field(2; PostingDate; Date)
        {
            trigger OnValidate()
            var
                MyCodeunit: Codeunit MyCodeunit;
            begin
                MyCodeunit.SetPostingDate(Rec);
            end;
        }
    }
}
 
codeunit 50000 MyCodeunit
{
    procedure SetPostingDate(var MyTable: Record MyTable)
    begin
        MyTable.Validate(PostingDate, WorkDate());
    end;
}

Here, to know that an assignment "originated from the field's own OnValidate" you'd have to walk the call path interprocedurally. Doing that per assignment is a real performance killer and arguably beyond what an operation-level analyzer should attempt.

So my thinking is to handle the simple case (direct self-assignment inside the field's own OnValidate / OnBeforeValidate / OnAfterValidate) and leave the indirected case as a known limitation.

@StefanMaron and/or @ChristianHovenbitzer You both did a deep-dive on this topic at BC TechDays 2024. Would you be willing to assist me here on this? What do you think and what would is the best approach to resolve this?

[ChristianHovenbitzer]

Honestly, in this case I don't see a reason not to validate. The loop would break by itself with validating a non-empty value into the date field. I think writing a new value into the same field should also trigger a validate. Generally I would encourage everyone to define an exit condition to break infinite loops rather than not-validating, and if there is no exit condition that can be applied, a code comment is helpful in anyways.

What if i want to react on the WorkDate being written into the PostingDate?

Your second screenshot @Arthurvdv is a really nasty side effect on the function itself. That might be challenging to decipher, since here as well we might have exit conditions on top that would prevent infinite loops

[TKapitan]

@ChristianHovenbitzer well that's a really good question. Can you even do what you said? How it works?

Example - changing PostingDate (with :=)

Validate(PostingDate, 0D)

• OnBeforeValidate with 0D
• OnValidate 0D the changes the value to WorkDate with :=
• OnAfterValidate with WorkDate

Example - changing PostingDate (with your idea)

Validate(PostingDate, 0D)

• OnBeforeValidate with 0D
• OnValidate 0D the changes the value to WorkDate with Validate
• OnBeforeValidate with WorkDate
• OnValidate WorkDate, no change due to condition
• OnAfterValidate with WorkDate
• OnAfterValidate with 0D

I think this can lead to serios problems as the last validation is with a value that is no longer there. For this reason I enforce in PRs to never validate the field that triggered validation.

For critical processes I even have tests that parse the callstack to ensure this does not happen in ISVs I'm helping maintaining.

In summary

• if you want to react on new value in the same app: you have to add the logic after the change (e.g. as is in standard BC in many places that first there are updates to the same fields and only later the value is used or propagated.
• if you want to react on updated value in another app: use onaftervalidate (of course, it won't work if other apps changed the value in onaftervalidate, but that's wrong design, the change should be in onbeforevalidate)

@ChristianHovenbitzer

[Arthurvdv]

@TKapitan @ChristianHovenbitzer thanks both for the input.

I'm happy to loosen the rule: A direct assignment to a field, inside that same field's own OnValidate / OnBeforeValidate / OnAfterValidate method to no longer raise the diagnostic. This is the cheap, syntax/operation-level case I mentioned, walking up to the enclosing trigger and comparing the field, no interprocedural analysis needed.

Scope I want to be explicit about, so the intent stays intact:

• Assignments to other fields inside an OnValidate will still raise the diagnostics
• The indirected case (assignment moved into a helper method/codeunit) stays a known limitation, walking the call path per assignment is too complex/costly.

The rule still achieves its goal for every other scenario, so I'm comfortable this trims a false positive without weakening the intent.