microsoft / AL

Home of the Dynamics 365 Business Central AL Language extension for Visual Studio Code. Used to track issues regarding the latest version of the AL compiler and developer tools available in the Visual Studio Code Marketplace or as part of the AL Developer Preview builds for Dynamics 365 Business Central.
MIT License
744 stars 245 forks source link

AA0198 and AA0244 and "this" keyword #7877

Open PeterConijn opened 1 day ago

PeterConijn commented 1 day ago

Please include the following with each issue:

1. Describe the bug This is a tricky one since the compiler is technically correct, but keeping this (no pun intended) enabled like it is would introduce a lot of warnings and is - I think - contrary to the reasons that "this" was added to AL. I realize that it might be hard for the compiler to check this consistently, however.

The problem is that when you have a global variable and a local variable or parameter of the same name, AA0244 still fires, even if you consistently use the "this" keyword.

I would like to see what the community thinks of this. For my part, I am not going to advise my team to use "this" to specify global variables/procedures if it introduces more warnings that we then have to handle.

2. To Reproduce Steps to reproduce the behavior:

  1. Ensure CodeCop is enabled in your project
  2. Create a codeunit with a global variable
  3. Create a public/internal procedure in that codeunit that has either a local variable or a parameter of the same name as the global variable.
  4. Assign the global variable using the "this" keyword (i.e. this.MyVar := MyVar)
  5. Check the warnings and see that AA0244 still fires
codeunit 50060 "This Test"
{
    var
        Customer: Record Customer;

    procedure AssignCustomer(Customer: Record Customer);
    begin
        this.Customer := Customer;
    end;
}
codeunit 50060 "This Test"
{
    var
        Customer: Record Customer;

    procedure AssignCustomer(CustomerNo: Code[20])
    var
        Customer: Record Customer;
    begin
        if Customer.Get(CustomerNo) then
            this.Customer := Customer;
    end;
}

Note: Because the developers need to copy and paste the code snippet, including a code snippet as a media file (i.e. .gif) is not sufficient.

3. Expected behavior If the "this" keyword is used consistently throughout procedures to denote the global variable, I hope that the compiler can recognize this and not give the warning (yes, I know that it is hard to determine whether it is used consistently)

4. Actual behavior Both rules still fire even though the "this" would indicate the variables are different.

Image

Image

5. Versions:

Final Checklist

Please remember to do the following:

dannoe commented 21 hours ago

In my eyes using the same name for a local/parameter and a global variable is a code smell (even with this introduced) and could easily lead to errors. I would vote for keeping the warnings as is.