fixUnusedIdentifier: Don't remove parameter in override or non-last parameter in callback #24306
Conversation
…arameter in callback
ghost
force-pushed
the
codeFixUnusedIdentifier_parameterInOverride
branch
from
4aadf21 to
d6f95e2
Compare
| // Can't remove a non-last parameter in a callback. (Can if future parameters are also unused.) | ||
| const index = parent.parameters.indexOf(p); | ||
| Debug.assert(index !== -1); | ||
| return parent.parameters.slice(index).every(p => p.name.kind === SyntaxKind.Identifier && !p.symbol.isReferenced) || !checker.getContextualType(parent); |
There was a problem hiding this comment.
even if you did not have a contextual type, the function may be called or assigned to something else that passes or expects different parameters.
There was a problem hiding this comment.
That's true, though it would mean we could never remove any parameters. Maybe we should play it safe in code-fix-all but still provide the option to delete as a single fix?
There was a problem hiding this comment.
I think it is always safe to remove from the end, contextual type or not.
We should think about updating call sites to remove the unused arguments as well.. but that is a different change..
There was a problem hiding this comment.
So never remove a non-last parameter? A method in a class may also be expected to conform to some interface that wasn't declared.
There was a problem hiding this comment.
a method with fewer required arguments is assignable to one with more. so that should work. but yes, it could result in errors.. really any change to the signature can result in errors.. so we can either be conservative and say we only do it if we know all references, and know it was never aliased, or we can be more open and say, as long as the error will be caught by type check, we should be fine..
ghost
force-pushed
the
codeFixUnusedIdentifier_parameterInOverride
branch
from
9e5e161 to
9bf1690
Compare
Fixes #24303