apex-dev-tools / apex-dev-tools/apex-ls
Detect unused method and constructor parameters
- Lingua principale
- Scala
- Stelle
- 12
- Fork
- 3
- Merge medio
- 13h 43m
- PR unite (30g)
- 19
Descrizione
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.
Guida per i contributori
Nessuna guida per i contributori indicizzata per questo repository
Valutazione
Questa issue non è ancora stata valutata.