apex-dev-tools / apex-dev-tools/apex-ls

Detect unused method and constructor parameters

Offen
#520 0 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
backlog enhancement
Vorherrschende Sprache
Scala
Sterne
12
Forks
3
Ø Merge
13 Std. 43 Min.
Gemergte PRs (30 T.)
19

Beschreibung

Detect method and constructor parameters that are never read in the body — a silently dropped input.

## Origin

Fell out of #286, which tried to warn on parameters shadowing class fields. That rule was closed as
won't-do: 4,263 warnings on the sample projects, only ~1-2% actionable, because it flags the standard
`this.x = x` constructor idiom.

But the handful of genuine problems it did surface all shared a different shape — **the parameter is
never used**. The field-name collision was incidental. Extracting that as the rule worth having.

## Motivating examples

Both found in `apex-samples` during the #286 audit.

**`fflib_AppBindingResolver`** — recursive calls pass `subModules`, but the body ignores the
parameter and loops over `getModules()` again:

```apex
) {
List result = new List();
for (fflib_IAppBindingModule module : getModules()) {
```

**`sdgField`** — several constructor overloads accept `options` and never use it, so the field keeps
its inline empty list:

```apex
global sdgField(
...,
List options
) {
this.ID = ID;
...
this.FieldOrder = 1;
}
```

In both cases the caller reasonably believes it passed something that took effect. That is a real
defect and it is invisible on inspection.

## Why this is a better rule than #286's

- It keys on the parameter being **unused**, which is a property of the code, not a naming
convention. It cannot be satisfied or defeated by how the author chose to name things.
- The `this.x = x` idiom is not a false positive here, because such a parameter **is** used.
- It should be quiet on well-written code, which is what #286 failed at.

## Scope and open questions

Likely belongs with the existing unused analysis (`UnusedPlugin`) rather than in
`FormalParameter.verify`, since it needs body reference information rather than modifier or
declaration state. Where exactly is part of the work.

Points to settle before implementing — several of these decide whether this is viable at all, so
**measure against `apex-samples` early**, as #286 showed the sample corpus is what exposes a noisy
rule:

- **Interface and abstract method declarations** have no body — must be excluded outright.
- **Overrides and interface implementations** frequently must accept a parameter the implementation
does not need. This is legitimate and common. Excluding all overrides may remove much of the value;
not excluding them may reproduce #286's noise problem. This is the key question.
- **`@TestVisible` / test code** — decide whether test methods participate.
- **Suppression** — should `@SuppressWarnings('Unused')` cover this, consistent with existing unused
handling?
- **Severity** — warning, in line with the rest of the unused analysis and with the general
preference for warnings over errors on advisory checks.
- Whether a parameter used **only** in a disabled or unreachable branch should count as used.

## Acceptance

Before this is worth landing, it should be run over `apex-samples` and the results audited the same
way #286 was: report the total count and a hand-audited sample judging what proportion is genuinely
actionable. If the answer is not clearly better than #286's 1-2%, close this too.

Beitragsleitfaden

Für dieses Repository ist kein Beitragsleitfaden indexiert

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.