Unused Declaration Detection

[luc-blaeser]

Unused Declaration Detection

Detection of unused program declarations with compiler warnings.

Program example example.mo:

import Array "mo:base/Array";
import Debug "mo:base/Debug";

actor {
    var variable1 = 0;
    var variable2 = "TEST";

    func testUnusedFunction(parameter1 : Bool, parameter2 : Int) {
        var variable2 = 2;
        var variable3 = 3;
        let variable4 = 4;
        if (variable1 == 0 and variable3 == 3) {
            let variable2 = parameter1;
            Debug.print(debug_show(variable2));
        };
    };
};

Compiler messages:

example.mo:1.8-1.13: warning [M0194], Unused declaration Array
example.mo:6.9-6.18: warning [M0194], Unused declaration variable2
example.mo:8.10-8.28: warning [M0194], Unused declaration testUnusedFunction
example.mo:8.48-8.58: warning [M0194], Unused declaration parameter2
example.mo:9.13-9.22: warning [M0194], Unused declaration variable2
example.mo:11.13-11.22: warning [M0194], Unused declaration variable4

Coverage

The analysis detects the following unused declarations:

• Variables
• Parameters, including shared context
• Functions
• Classes
• Objects
• Modules
• Imports
• Private fields in objects and classes

Special aspects:

• System functions are considered implicitly used.
• Non-accessed stable variables are considered unused, even if they could be accessed in a future upgraded program version.

Warnings

The warning of an unused declaration can be suppressed by prefixing the identifier by an underscore.

Example:

object Silence {
    public func log(_message: Text) { // Suppress the warning for the unused `_message` parameter.
    }
}

Tweaks from #4407

• don't warn about unused declarations in code from packages (assuming packaces are third party you can't silence them anyway):
  • annotate LibPath Ast nodes with source package, if any, as tracked and determined during import resolution.
  • predicate unused declaration warnings on package origin.
• don't reject unused declarations in the repl treating top-level code as belonging to fake package "" (a mild hack).
  The repl can't know the rest of the interaction so any warning is premature and a nuisance.
• change terminology of declarations/variables to bindings/indentifiers (for consistency with rest of code)
• add error-code description in M0194.md
• add changelog entry.

Future: we could suppress all warnings, not just unused declarations - from imported package code this way, should we want to. A --lint mode could re-enable them for further auditing. The rationale is that the warnings are of interest to and actionable on only by the author of the package, not the client.

Future Work

The following analyses are not yet implemented but would be beneficial to support:

• Unused recursive function calls (direct or indirect recursion).
• Unused type definitions, unused type parameters
• Unused branch labels
• Unused variant options
• Unused public fields: Additional aspects to consider:
  • Accesses via paths outside the declaration scope.
  • Possible usage before declaration.
  • Polymorphism of structural typing.
  • A library module may expose more (directly or indirectly) public declarations than used.
• Write-only mutable variables: Mutable variables that are never read but only written
• Unnecessary mutability of read-only variables: Recommend let instead of var.

[github-actions]

Comparing from 7c1052e to 5f5dfbe:
In terms of gas, 1 tests improved and the mean change is -2.2%.
In terms of size, no changes are observed in 5 tests.

[crusso]

This is great! I still need to review carefully but after a cursory skim it certainly looks good to me.

Haven't tried it but will a recursive use silence the warning even if a function is never called non-recursively?

[crusso]

Finally got a chance to play with this... Looks good but some issues remain.

The repl will complain on almost every interaction, because you typically define stuff before using it later. Perhaps we should just suppress the warnings there.

[nix-shell:~/clean/motoko/test/fail]$ rlwrap moc
Motoko compiler (source 0.10.4-68-g88749501b-dirty)
> let x = 1;
stdin:1.5-1.6: warning [M0194], Unused declaration x
let x : Nat = 1
> x + x;
2 : Nat
> 

A recursive but unused functions is accepted sans warning. Ditto for mutually recursive functions. OCaml seems to be able to detect and warn about these, so if easy to fix, it would be nice to do the same. But perhaps it's non-trivial, in which case we could live with it for now.

I wonder if we should go the extra step and warn about a reference to a silenced identifier? OCaml does not, so perhaps a good enough reason not to. I.e. should

let _x = 1;
let y = _x; // warn on reference to silenced _x?

produce a (different) warning or not?

Some simple cases I tried:

func rec1() { rec1() }; // accepted, but reject as unused?

func rec() { }; //reject ok

do {let unused = 1 };

func g(x : ()) {};

//let unused = 1;
let _ok = 1;
let hmm = _ok; // should we warn about the use of a silenced identifier? OCaml doens't actually, so perhas not

do { func f() { g() ; }; both f and g are only used recursively accept or reject?
     func g() { f() };
};

[crusso]

Suggested change

	let emit (id, region) = warn env region "M0194" "Unused declaration %s" id in
	let emit (id, region) = warn env region "M0194" "unused identifier %s" id in

For consistency with other errors, probably best to lowercase the message. Also, the manual uses the terminology identifier/binding rather than variable/declaration but I don't feel that strongly about it.

[crusso]

I had a quick look at how ocaml does more accurate unused binding declarations for recursive bindings and found this comment. Not sure I grokk it but perhaps it'll inspire you:

https://github.com/ocaml/ocaml/blob/trunk/typing/typecore.ml#L6075

(* Algorithm to detect unused declarations in recursive bindings:
     - During type checking of the definitions, we capture the 'value_used'
       events on the bound identifiers and record them in a slot corresponding
       to the current definition (!current_slot).
       In effect, this creates a dependency graph between definitions.

     - After type checking the definition (!current_slot = None),
       when one of the bound identifier is effectively used, we trigger
       again all the events recorded in the corresponding slot.
       The effect is to traverse the transitive closure of the graph created
       in the first step.

     We also keep track of whether *all* variables in a given pattern
     are unused. If this is the case, for local declarations, the issued
     warning is 26, not 27.
   *)

[crusso]

I just tried building motoko-base/test and got some warnings we should probably fix:

unused.txt

But I guess this raises the question whether we should disable the production of warnings on code from third party libraries (perhaps just from every package).

Whaddya think?

[crusso]

re-order conjunction to make it cheaper?

[luc-blaeser]

Thank you very much for having improved and extended this PR and brought it to the quality level for merging it with master.

As for the detection of unused recursion, I also believe that we would need to do some transitive closure analysis (like Ocaml also does). As this is however different to the current design, I believe it is pragmatic to add this later.