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

Detect unused method and constructor parameters

Abierto
#520 0 comentarios 0 reacciones 0 asignados Ver en GitHub
backlog enhancement
Lenguaje dominante
Scala
Estrellas
12
Forks
3
Merge medio
13 h 43 min
PR fusionados (30 d)
19

Descripción

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.

Guía de contribución

No hay ninguna guía de contribución indexada para este repositorio

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.