New Rule: Detect global variables that could be local variables

[finnpedersenkazes]

Title

Detect global variables that could be local variables

Description

A global variable should not be declared at object scope when it is only used by a single procedure.

The analyzer should detect global variables where all references are contained within one procedure and suggest moving the variable declaration to that procedure's local var section.

Example:

codeunit 50100 MyCodeunit
{
    var
        Customer: Record Customer;

    local procedure DoSomething()
    begin
        Customer.Get('10000');
    end;
}

Customer is only used by DoSomething() and could therefore be declared as a local variable:

codeunit 50100 MyCodeunit
{
    local procedure DoSomething()
    var
        Customer: Record Customer;
    begin
        Customer.Get('10000');
    end;
}

Keeping variables at the smallest possible scope makes dependencies clearer and avoids unnecessary object state. This is especially relevant for record variables, where filters, keys, field values, marks, etc. may otherwise persist as part of the object state.

I have especially detected this in code converted from classic C/AL to AL.

[someC0d3r]

The rule is useful in principle but “used by only one procedure” does not prove that a global variable can safely become local.

A simple counterexample is a variable that retains state between calls:

var
    Counter: Integer;

procedure GetNextNumber(): Integer
begin
    Counter += 1;
    exit(Counter);
end;

Moving Counter to local scope would make the procedure return 1 on every call.

Another well-known example is the GetRecordOnce pattern used by setup tables:

procedure GetRecordOnce()
begin
    if RecordHasBeenRead then
        exit;

    Get();
    RecordHasBeenRead := true;
end;

Although RecordHasBeenRead may only be referenced by this procedure, it must remain global to cache state between calls. Making it local would cause repeated database reads.

Therefore "used by only one procedure" should only identify a candidate. Stateful variables need additional analysis, while stateless variables such as Label can usually be moved safely.

[Arthurvdv]

I’ve reviewed last year a PR from @iwanscheidsnc for this:
StefanMaron/BusinessCentral.LinterCop#1130

Unfortunately at that moment in time we’ve decided to not go through with it. A fresh view on this is a great idea, where you can read some considerations on the comments of the PR.

[someC0d3r]

Hmm.. how can ALCops determine whether a global variable is intended to retain state between calls or can safely be moved to local scope? imho this cannot be determined reliably (.... or? I have at least not a single clue..)
I would personally consider such a diagnostic too risky. A less experienced developer might take the finding at face value, move the variable to local scope and unknowingly change the application’s behavior.
I can understand applying the rule to immutable declarations such as Label variables. For writable, stateful variables, however, it becomes more complicated.. :-/

alternatively, the default severity could be set to None, requiring developers to enable the rule explicitly. The diagnostic message should also clearly state that the developer must carefully verify whether the variable can safely be moved to local scope without changing its behavior

[Arthurvdv]

It would be great if we indeed could determine whether a global variable is intended to retain state between calls (or indeed can safely be moved to a local scope).

The only thing i could think of is if we assume that all the global variables are used to hold and expose state, then maybe a new rule could be something like: _" ... missing Get, Set, and/or Clear method for global variable ...". This could be an public or internal procedure or the variable should be declared as a protected var.

But probably it's not always a good idea to have a Getter and/or Setter for a global variable (that keeps state)?

[someC0d3r]

Edit:
I've created following Draft PRs regarding this post so you can test the rule itself (current default severity is 'Hidden', you can set it higher tho; for testing purposes you can use a custom ruleset or set the default severity yourself higher):
#506
ALCops/alcops.dev#165

Currently it is only implemented for objects of type 'codeunit', see reason in the Analyzers PR - Labels theoretically could be easy to analyze and report (since their state can't be changed), but other data types could be a bit more tricky to analyze in objects other than codeunits (I've added two examples in the reason in the PR)

The PRs are only a "proof-of-concept" - so nothing I would currently want & suggest to merge into main..
(... also the PRs got closed by me instantly, so they don't appear on the PR list)

---

Disclaimer
All of the following is written by AI. I've gave it some of the edge cases I've come up with and let it also think about some. I've reviewed it and all of it looked fine and made sense for me so far. Just wanna mention that! :)

---

I believe the rule could be useful if it is restricted to high-confidence cases

The important distinction is not only whether a global variable is directly referenced by a single procedure, but whether its previous global state can influence the procedure.

A mutable variable should only produce a diagnostic if every execution path leading to a read initializes the relevant state first. Calls that pass the variable by reference using a var parameter also require special consideration: the called procedure may read the existing value, modify it conditionally, or otherwise depend on state retained from an earlier invocation.

Here are some positive and negative examples:

codeunit 50100 "Variable Scope Cases"
{
    var
        MyGlobalVariable: Integer;
        MyBranchValue: Integer;
        MyCustomerFromGet: Record Customer;
        MyClearedCustomer: Record Customer;
        MyProcedureOnlyLabel: Label 'Customer: %1', Comment = '%1 = Customer name';
        MyGuardedValue: Integer;

        // Negative examples
        MyCounter: Integer;
        MyConditionalValue: Integer;
        MyByReferenceValue: Integer;
        MyRecursiveValue: Integer;

    // Case 1:
    // The variable is unconditionally overwritten whenever the procedure is called.
    // Expected: Diagnostic
    // 'MyGlobalVariable' can be moved to the local scope of 'MyDummyProcedure'.
    local procedure MyDummyProcedure()
    begin
        MyGlobalVariable := 1;
        Message('%1', MyGlobalVariable);
    end;

    // Case 2:
    // Every possible execution path assigns a new value before the variable is read.
    // Expected: Diagnostic
    // 'MyBranchValue' can be moved to the local scope of 'ShowBranchValue'.
    local procedure ShowBranchValue(UseFirstValue: Boolean)
    begin
        if UseFirstValue then
            MyBranchValue := 1
        else
            MyBranchValue := 2;

        Message('%1', MyBranchValue);
    end;

    // Case 3:
    // This is the example from the original post.
    //
    // Record.Get loads the requested record before any of its field values are read.
    // If Get fails and its Boolean return value is omitted, a runtime error is raised,
    // so there is no execution path that continues with the previous record value.
    //
    // In this specific example, the previously loaded record cannot influence the
    // successful execution path.
    // Expected: Diagnostic
    // 'MyCustomerFromGet' can be moved to the local scope of
    // 'ShowCustomerFromOriginalPost'.
    local procedure ShowCustomerFromOriginalPost()
    begin
        MyCustomerFromGet.Get('10000');
        Message('%1', MyCustomerFromGet.Name);
    end;

    // Case 4:
    // The complete Record state is explicitly cleared before it is used.
    // Filters, keys and field values from an earlier call cannot affect the result.
    // Expected: Diagnostic
    // 'MyClearedCustomer' can be moved to the local scope of 'ShowCustomer'.
    local procedure ShowCustomer(CustomerNo: Code[20])
    begin
        Clear(MyClearedCustomer);

        if MyClearedCustomer.Get(CustomerNo) then
            Message('%1', MyClearedCustomer.Name);
    end;

    // Case 5:
    // Labels are immutable and cannot retain mutable state between calls.
    // Expected: Diagnostic
    // 'MyProcedureOnlyLabel' can be moved to the local scope of 'ShowCustomerName'.
    local procedure ShowCustomerName(CustomerName: Text)
    begin
        Message(MyProcedureOnlyLabel, CustomerName);
    end;

    // Case 6:
    // An early exit does not prevent the move because every path that reads the
    // variable assigns it first.
    // Expected: Diagnostic
    // 'MyGuardedValue' can be moved to the local scope of 'ShowGuardedValue'.
    local procedure ShowGuardedValue(ShouldContinue: Boolean)
    begin
        if not ShouldContinue then
            exit;

        MyGuardedValue := 42;
        Message('%1', MyGuardedValue);
    end;

    // Case 7:
    // The previous value is read and incremented.
    // Moving the variable would make every invocation display 1.
    // Expected: No diagnostic
    local procedure ShowNextNumber()
    begin
        MyCounter += 1;
        Message('%1', MyCounter);
    end;

    // Case 8:
    // The variable is initialized only conditionally.
    // When InitializeValue is false, a value from an earlier invocation may be used.
    // Expected: No diagnostic
    local procedure ShowConditionalValue(InitializeValue: Boolean)
    begin
        if InitializeValue then
            MyConditionalValue := 42;

        Message('%1', MyConditionalValue);
    end;

    // Case 9:
    // The global variable is directly referenced only by this procedure, but it is
    // passed by reference to another procedure. The called procedure reads and
    // modifies the existing value.
    //
    // Moving the variable to local scope would make every invocation start at zero
    // and display 42. As a global variable, subsequent invocations display 43, 44,
    // and so on.
    // Expected: No diagnostic
    local procedure ShowValueChangedByReference()
    begin
        UpdateValue(MyByReferenceValue);
        Message('%1', MyByReferenceValue);
    end;

    local procedure UpdateValue(var Value: Integer)
    begin
        if Value = 0 then
            Value := 42
        else
            Value += 1;
    end;

    // Case 10:
    // Even though the variable is assigned before it is read, recursion makes
    // global and local scope behave differently. Recursive calls overwrite the
    // global value, while each local variable belongs to its own stack frame.
    // Expected: No diagnostic
    local procedure RecursiveProcedure(Number: Integer): Integer
    begin
        MyRecursiveValue := Number;

        if Number > 1 then
            RecursiveProcedure(Number - 1);

        exit(MyRecursiveValue);
    end;
}

Record.Get is an interesting special case. In the original example, it loads the requested record before its fields are read. A failed Get, when the optional Boolean return value is omitted, raises a runtime error, so execution cannot continue using field values from an earlier call.

However, Record.Get must not automatically be treated as equivalent to Clear. According to the documentation, Get uses the primary key and ignores ordinary filters while retrieving the record, but it does not change the current sorting key or existing filters.

For example, the following would require more careful analysis:

MyCustomerFromGet.Get('10000');
Message('%1', MyCustomerFromGet.GetFilters());

The loaded record fields are newly populated, but GetFilters() may still return filters retained by the global Record variable. Moving it to local scope could therefore change the result.

This means the analyzer would need to distinguish between:

• State established by Get, such as the loaded field values.
• Record state retained across the call, such as filters and the current key.
• Subsequent operations that may depend on that retained state.

Case 9 is another reason why checking only the direct references of a global variable is insufficient. At the call site, passing a variable to a var parameter should be treated as a potential read and a potential write, not as a definite assignment.

It might still be safe when the variable is fully initialized before the call:

MyByReferenceValue := 0;
UpdateValue(MyByReferenceValue);

However, recognizing additional safe cases would require interprocedural data-flow analysis. The analyzer would need to inspect the called procedure and prove that the incoming value is never read before being overwritten on every possible return path. This becomes even more difficult for external procedures or calls that may publish events.

Case 10 similarly demonstrates that assignment-before-read analysis alone is insufficient because recursion or reentrancy can make global and local variables behave differently.

I would therefore suggest limiting the diagnostic to cases where:

• All direct references are contained within exactly one procedure or trigger.
• Immutable Label variables are considered safe candidates.
• A mutable variable is fully assigned or cleared on every execution path before its first read.
• Known operations such as Record.Get are handled according to the specific state they initialize or retain.
• Passing the variable to a var parameter is treated as a potential read and write.
• Variables passed by reference are excluded unless their safety can be proven through interprocedural analysis.
• Compound assignments such as += are not treated as initialization.
• Conditionally initialized variables are excluded.
• Direct and indirect recursion are excluded.
• protected var declarations are excluded because extensions may reference them.
• Stateful object types and execution models, such as SingleInstance codeunits, manually bound event subscribers and repeatedly invoked triggers, are handled conservatively.

A cautious diagnostic message might be:

Global variable '{0}' is only used in '{1}' and appears to be reinitialized before every read. Consider moving it to local scope.

Using “appears to be” and “consider” avoids claiming that the move is always safe while still identifying useful, high-confidence candidates.

[Arthurvdv]

Looks promising! I want to add that I think we should consider leaving translatable variables, like Label as global variables.

The trans-unit ID in the .xlf file encodes the label's full containment path within the object. Each segment in that path is derived from the symbol kind and a hash of its name.

For a label declared as a global variable, the ID only contains the object itself and the variable name.
For a label declared locally inside a procedure or trigger, every intermediate container (the procedure and for nested structures like table fields or page actions the field or action as well, becomes part of the ID).

This means:

• Renaming a procedure or trigger changes the trans-unit IDs of all local labels within it, breaking existing translations (translators have to redo them or the IDs need manual remapping in the .xlf file).
• Moving a label between procedures, or from a trigger into a different trigger, also changes its ID (even if the variable name stays the same).
• Global label variables are only tied to the object and the variable name, so refactoring procedures, renaming triggers or restructuring logic has no effect on their translation IDs.

That's the practical argument for declaring label variables at the object level rather than locally inside procedures or triggers. This makes the translation IDs more stable when you refactor code. If you rename a procedure, reorganize trigger logic, or move functionality between methods, your translations survive.

[someC0d3r]

Regarding whether labels should be declared globally or locally, there was a discussion about this some time ago that I started on the BC Professionals Discord server:

Invite link: https://discord.gg/NQs5tyDne
Message link: https://discord.com/channels/731573782593929269/731580959803179048/1534112629168144515

Screenshot for those who don't have Discord:

Unfortunately, not many people shared their opinion there.

Personally, when we migrated from C/AL to AL, we gradually moved most global labels into the local scope where they are actually used.

One of the main reasons was the XLIFF output. With local labels, the translation unit provides much more precise context: instead of only showing the object in which the label exists, it can also indicate the procedure, trigger, action, field, etc. where it is used. This makes maintaining translations considerably easier, especially when the XLIFF files are handed over to an external translation provider who may not be that familiar with the codebase..

I also prefer it from a code-structure perspective. Especially in larger management codeunits or reports, I like the global var section to contain variables that are actually global because they need to be shared across multiple procedures. Having dozens of labels there that are only used once makes the global variable section unnecessarily noisy.

With tools such as NAB AL Tools, maintaining local labels is also quite comfortable. For example, when renaming procedures, it detects the change and updates the corresponding translation unit accordingly.

Just have a look at something like this in the Base App:

https://github.com/microsoft/BCApps/blob/e7751a5b00b07fb0c8565675a87ba374b8a5f50d/src/Layers/W1/BaseApp/BusinessSetupSubscribers.Codeunit.al#L63-L358

https://github.com/microsoft/BCApps/blob/e7751a5b00b07fb0c8565675a87ba374b8a5f50d/src/Layers/W1/BaseApp/Inventory/Tracking/MatchedOrderLineMgmt.Codeunit.al#L1240-L1265

In these example, also Locked labels are declared in the global scope, which doesn't really make sense since they don't affect the XLIFF output at all:
https://github.com/microsoft/BCApps/blob/e7751a5b00b07fb0c8565675a87ba374b8a5f50d/src/Layers/W1/BaseApp/RoleCenters/RoleCenterNotificationMgt.Codeunit.al#L28-L65

(1/2)
https://github.com/microsoft/BCApps/blob/e7751a5b00b07fb0c8565675a87ba374b8a5f50d/src/Layers/W1/BaseApp/Manufacturing/Document/ProdOrderStatusManagement.Codeunit.al#L62-L71

(2/2)
https://github.com/microsoft/BCApps/blob/e7751a5b00b07fb0c8565675a87ba374b8a5f50d/src/Layers/W1/BaseApp/Manufacturing/Document/ProdOrderStatusManagement.Codeunit.al#L96-L114

That is basically the kind of global var block I would like to avoid. I could probably find quite a few more examples like this in the standard apps.

So I'm not entirely sure what the best approach for the rule should be.

Maybe we could make the behavior configurable, for example:

• Consider Moving Labels to Local Scope
• Consider Moving Labels to Global Scope

Alternatively, the rule could simply ignore Label variables altogether..
Whats your opinion here?

I know that BCQuality also recommends global labels, so maybe there is a bit of personal bias involved here. :D

Still, declaring something globally purely because it makes translation maintenance easier feels somewhat wrong to me from a code-design perspective. If a label is only used inside a single procedure, trigger, action, etc., I would generally expect it to live in that scope as well.

And looking at heavily bloated global var sections like the examples linked above doesn't really convince me otherwise