StefanMaron / BusinessCentral.LinterCop

Community driven code linter for AL (MS Dynamics 365 Business Central)
https://stefanmaron.com
MIT License
72 stars 30 forks source link

Warn if eventsubscribers don't use var for parameters defined as var. #676

Closed dannoe closed 2 months ago

dannoe commented 2 months ago

This implements the idea from #654 and adds tests for the rule.

I also did some additional things:

I am open to suggestions, vetoes, or requests to split these changes into multiple PRs.

dannoe commented 2 months ago

I tried a new way to add the DiagnosticDescriptors. For this rule the Descriptors is now in a nested class inside the rule itself. The reason for this is, that I find the LinterCopAnalyzers.Generated.cs file very unclear and unreadable with these extreme long lines.

I do have a question about the nested class inside the rule itself. There are a few exceptions where the rule and the descriptor aren't coupled, like the descriptors Rule0005VariableCasingShouldNotDifferFromDeclaration is used in the Rule0003 and Rule0005 class. Can this then still work?

As long as the nested class is accessible from other places in the code (like public or internal for the same assembly) this should be no problem.

Arthurvdv commented 2 months ago

First I wanted to thank you contributing, really appreciatie you're taking the time to help further improve the LinterCop :-)

When I was creating a example for the wiki, I've run into ArgumentNullException (due tough a typo I've created)

image

codeunit 50100 MyBusinessLogic
{
    [IntegrationEvent(false, false)]
    local procedure OnBeforeMyBusinessLogic(var Customer: Record Customer; var IsHandled: Boolean)
    begin
    end;
}

codeunit 50501 MyEventSubscriber
{
    [EventSubscriber(ObjectType::Codeunit, Codeunit::MyBusinessLogic, NonExistingEventName, '', false, false)]
    local procedure OnBeforeMyBusinessLogicOnMyBusinessLogic(Customer: Record Customer)
    begin
    end;
}

While the AL code is wrong due to the NonExistingEventName, it would be great if the LinterCop doesn't throw an exception during development. Do you want me to have a look into this of is this something you can easily resolve?

dannoe commented 2 months ago

While the AL code is wrong due to the NonExistingEventName, it would be great if the LinterCop doesn't throw an exception during development. Do you want me to have a look into this of is this something you can easily resolve?

That's interesting. The exceptions occurred because there was a null given as a parameter to the GetFirstMethod method. But according to nullable annotations of microsoft code, the ValueText of the IAttributeArgumentSymbol is never null (it is string not string?). But somehow it still gives null in case of non-compilable code.

At first I couldn't reproduce this on my site because I was testing it with a large project (700+ files). But I was able to reproduce it with a smaller test project and added an additional null check.

Edit: I removed the NRE fix from this PR and opened a seperate PR (#678)

Arthurvdv commented 2 months ago

Again, thanks for the contribution, much appreciated :-)