New rule to check IsHandled set to false

[ans-bar-bm]

Implements a new rule for the discussion #715. The intent of the rule is to disallow setting IsHandled to false, as that may overwrite IsHandled if it was set by a previous event subscriber, which may then cause issues.

Few points I am unsure about:

• Not sure about the wording of the diagnostic message, but i could not come up with a better one.
• Currently this disallows setting IsHandled to anything but true. Even having something like myVar := true; IsHandled := myVar; is not allowed as i could not find a way to check (reliably) if a variable / procedure is guaranteed to be true.
• Currently this disallows passing IsHandled as a var parameter to another procedure for the same reason as above. So a "wrapper" like below will also raise the diagnostic:

  
  local procedure MyEventSubscriber(var IsHandled: Boolean)
  begin
  MyOtherProcedure(IsHandled);
  end;

local procedure MyOtherProcedure(var IsHandledParameter: Boolean) begin // runs some code IsHandledParameter := true; end;

[ans-bar-bm]

Not quite sure what's going on with the failing versions, or rather how to check / fix those. If someone knows / has a hint i can see if i can fix them.

[Arthurvdv]

First, I apologize for the delay in responding to this PR. The reason for the wait is that I've been considering the best solution for handling the failing versions, going back and forth on different approaches.

My initial though to just suggest slapping a preprocessor directive around the rule, excluding this rule for older AL Language versions, like for example on the Rule0035. https://github.com/StefanMaron/BusinessCentral.LinterCop/blob/6c7a4c54c36f83af6ca173f1e069f8b6ee99ba87/BusinessCentral.LinterCop/Design/Rule0035ExplicitSetAllowInCustomizations.cs#L1

Root cause The root cause of these failures is .ConstantValue method which isn't implemented in those early v12 version of the AL Language. So we could add a preprocessor directive for the failing versions to retrieve the value some other way around.

... then I thought: These are old pre-release versions of the v12. Do we really need to have assets for these versions? 🤔 So I've adapt the workflow (https://github.com/StefanMaron/BusinessCentral.LinterCop/pull/779) to exclude old pre-release versions. If you uptake the changes for the pre-release into your branch these failing versions should be there anymore.

Some more feedback on your remarks

Not sure about the wording of the diagnostic message, but i could not come up with a better one.

I would suggest Title: Incorrect 'IsHandled' parameter assignment Description: "The 'IsHandled' parameter must always be set to 'true' when handled. Avoid assigning 'false' or any value that may be evaluated as false.

Currently this disallows setting IsHandled to anything but true. Even having something like myVar := true; IsHandled := myVar; is not allowed as i could not find a way to check (reliably) if a variable / procedure is guaranteed to be true.

If I understand this correctly, this potentially could raise a false positive? For now I would leave it as is and gather feedback on this in the pre-release version of the LinterCop.

Currently this disallows passing IsHandled as a var parameter to another procedure for the same reason as above. So a "wrapper" like below will also raise the diagnostic:

The same as above, lets try this out in de pre-release and see if this implementation is viable.

I have another suggestion maybe: Next to IsHandled, I also sometimes also see the use of Handled, see some examples: https://github.com/search?q=repo%3AStefanMaron%2FMSDyn365BC.Code.History+%22var+Handled%3A+Boolean%22&type=code

It could be opportune to also include this keyword in your rule?

[ans-bar-bm]

... then I thought: These are old pre-release versions of the v12. Do we really need to have assets for these versions? 🤔 So I've adapt the workflow (#779) to exclude old pre-release versions. If you uptake the changes for the pre-release into your branch these failing versions should be there anymore.

Thanks, that seems to work!

I would suggest Title: Incorrect 'IsHandled' parameter assignment Description: "The 'IsHandled' parameter must always be set to 'true' when handled. Avoid assigning 'false' or any value that may be evaluated as false.

Updated the wording.

Currently this disallows setting IsHandled to anything but true. Even having something like myVar := true; IsHandled := myVar; is not allowed as i could not find a way to check (reliably) if a variable / procedure is guaranteed to be true.

If I understand this correctly, this potentially could raise a false positive? For now I would leave it as is and gather feedback on this in the pre-release version of the LinterCop.

Yes, a false positive in a sense that the actual value is true, just wrapped in a variable or procedure return value. Although I'd argue if it's guaranteed to be true at compile time, might as well use true directly. So I'd agree to leave it as is and see what potential issues come up.

I have another suggestion maybe: Next to IsHandled, I also sometimes also see the use of Handled, see some examples: https://github.com/search?q=repo%3AStefanMaron%2FMSDyn365BC.Code.History+%22var+Handled%3A+Boolean%22&type=code

It could be opportune to also include this keyword in your rule?

Thanks, I've added Handled to the check.

[Arthurvdv]

Looks great! Let's merge this into the pre-release to gather some feedback.

Thank you for your patience on this PR and I apologize for the delay (these past few weeks have been swamped with work, and I wish I could have handled your PR sooner).