Rule to warn about paramters by value that are passed to events by reference

[pri-kise]

The BaseApp is currently refactored in many areas and events are moved to new codeunits.

The Old events are called via new procedures.

Now it can happen that someone forgets to declare a variable by reference in the new procedure although the event passes the parameter by reference.

This can lead to problems like the following: http://github.com/microsoft/BCApps/issues/1268

I've created an idea (https://experience.dynamics.com/ideas/idea/?ideaid=fcc7cecb-5d29-ef11-8ee7-6045bdb88c35) that this is added to a CodeCop, but until the plattform adds such an check I think this could be useful to have it in the BCLinterCop.

[Arthurvdv]

@dannoe has created an new rule for this 🥳

[pri-kise]

If you refer to the new rule LC0065 then I think that it was solved on the wrong way.

I was hoping for a rule that warns when you pass a procedure parameter (by value) to an event where this parameter is used by reference.
as it is done in the Microsoft BaseApp by public/internal methods to call an local event.

    [Obsolete('This is a temporary method for compatibility only. Please use the "No. Series" codeunit instead', '24.0')]
    internal procedure RaiseObsoleteOnAfterGetNextNo3(NoSeriesLine: Record "No. Series Line"; ModifySeries: Boolean) //NoSeriesLine should be called by reference
    begin
        OnAfterGetNextNo3(NoSeriesLine, ModifySeries);
    end;

    [Obsolete('This event is obsolete. Please use the extensibility options provided by the No. Series module.', '24.0')]
    [IntegrationEvent(false, false)]
    local procedure OnAfterGetNextNo3(var NoSeriesLine: Record "No. Series Line"; ModifySeries: Boolean)
    begin
    end;

[dannoe]

In fact, I misunderstood your suggestion. The way I implemented it made sense to me because I had this problem quite a few times. Your suggestion, on the other hand, is something I haven't experienced myself (yet?).

Just a thought on this rule:

It could be on purpose to give a record, that was passed by value, to an event subscriber via reference.
Lets say I have a function, that only needs the values for filtering. And I am reusing the record parameter to filter and search for it:

internal procedure UpdateOtherSalesLines(SalesLine: Record "Sales Line")
begin
    SalesLine.SetRange("Document No.", SalesLine."Document No.");
    SalesLine.SetFilter("Line No.", '<>%1', SalesLine."Line No.");
    if SalesLine.FindSet(true) then
        repeat 
          // do something 
          SalesLine.Modify(true);
        until SalesLine.Next() = 0;
end;

Later I make my code extensible by adding two events (maybe a customer app needs to filter for specific lines and change some more fields)

internal procedure UpdateOtherSalesLines(SalesLine: Record "Sales Line")
begin
    SalesLine.SetRange("Document No.", SalesLine."Document No.");
    SalesLine.SetFilter("Line No.", '<>%1', SalesLine."Line No.");
+    OnUpdateOtherSalesLinesOnBeforeFindSet(SalesLine);
    if SalesLine.FindSet(true) then
        repeat 
          // do something
+        OnUpdateOtherSalesLinesOnBeforeModify(SalesLine);          
        until SalesLine.Next() = 0;
end;

+    [IntegrationEvent(false, false)]
+    local procedure OnUpdateOtherSalesLinesOnBeforeFindSet(var SalesLine: Record "Sales Line")
+    begin
+    end

+    [IntegrationEvent(false, false)]
+    local procedure OnUpdateOtherSalesLinesOnBeforeModify(var SalesLine: Record "Sales Line")
+    begin
+    end

I would now get the warning for the parameter of my old (!) procedure that may have been introduced a long time ago.
If I would just add var to the parameter, I might change the logical flow of the caller, because my events could now change the record filters in the caller.

PS: And yes, reusing the parameter is not ideal, but it happens, because people are lazy sometimes ;)

I am not against the implementation of the rule, but I would probably disable it.

[pri-kise]

@dannoe thanks for your detailed answer.

I understand your point and I would suggest that this rule can be an info by default.

About your new rule.
Isn't this rule in conflict with rule AA0150 when I don't change the variable in my code?
https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/analyzers/codecop-aa0150

[dannoe]

About your new rule.
Isn't this rule in conflict with rule AA0150 when I don't change the variable in my code?
https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/analyzers/codecop-aa0150

This may become an issue in the future, but in the current version (14.0.16.22338), AA0150 is not triggered for procedures with any of the following attributes (as Rule0065 is only triggered for EventSubscribers)

"obsolete",
"integrationevent",
"eventsubscriber",
"confirmhandler",
"internalevent",
"businessevent"