Remove Unused Binding codefix throws an exception for unused function parameters

[baronfel]

given the following sample code:

let inc i =
    2

where the i parameter is unused, the i parameter has a diagnostic saying it's unused, but asking for code actions at the i location results in the following stacktrace:

Message: System.IndexOutOfRangeException: Index was outside the bounds of the array.
   at FsAutoComplete.CodeFix.Navigation.loop@107(ISourceText lines, FSharpFunc`2 posChange, FSharpFunc`2 terminalCondition, FSharpFunc`2 checkCondition, Position firstPos, Position finalPos, Position pos) in /home/runner/work/FsAutoComplete/FsAutoComplete/src/FsAutoComplete/CodeFixes.fs:line 108
   at FsAutoComplete.CodeFix.Navigation.walkPos(ISourceText lines, Position pos, FSharpFunc`2 posChange, FSharpFunc`2 terminalCondition, FSharpFunc`2 checkCondition) in /home/runner/work/FsAutoComplete/FsAutoComplete/src/FsAutoComplete/CodeFixes.fs:line 115
   at FsAutoComplete.CodeFix.Navigation.walkBackUntilCondition@146-2.Invoke(FSharpFunc`2 checkCondition) in /home/runner/work/FsAutoComplete/FsAutoComplete/src/FsAutoComplete/CodeFixes.fs:line 146
   at FsAutoComplete.CodeFix.RemoveUnusedBinding.binder@1-152(Diagnostic diagnostic, CodeActionParams codeActionParams, ISourceText lines, Range _arg2) in /home/runner/work/FsAutoComplete/FsAutoComplete/src/FsAutoComplete/CodeFixes/RemoveUnusedBinding.fs:line 53
   at [email protected](FSharpResult`2 _arg1)
   at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvokeNoHijackCheck[a,b](AsyncActivation`1 ctxt, FSharpFunc`2 userCode, b result1) in D:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 404
   at [email protected](AsyncActivation`1 ctxt) in /home/runner/work/FsAutoComplete/FsAutoComplete/src/FsAutoComplete/CodeFixes/RemoveUnusedBinding.fs:line 51
   at [email protected](AsyncActivation`1 ctxt)
   at [email protected](AsyncActivation`1 ctxt) in /home/runner/work/FsAutoComplete/FsAutoComplete/src/FsAutoComplete/FsAutoComplete.Lsp.fs:line 568
   at <StartupCode$FsAutoComplete-Core>[email protected](AsyncActivation`1 ctxt) in /home/runner/work/FsAutoComplete/FsAutoComplete/src/FsAutoComplete.Core/Commands.fs:line 561
   at [email protected](AsyncActivation`1 ctxt) in /home/runner/work/FsAutoComplete/FsAutoComplete/src/FsAutoComplete/FsAutoComplete.Lsp.fs:line 567
   at [email protected](AsyncActivation`1 ctxt)
   at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction) in D:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 104

This is in version 0.46.6.

[baronfel]

This is actually showing a bug in the logic of the codefix itself, I think: We essentially replicate the code from dotnet/fsharp and at least on my test setup this logic seems to be trying to remove the entire inc binding, despite the diagnostic being for the i parameter pattern here.

I think the logic of this overall needs another pass.

Taking the logic as it stands, the diagnostic is fired for (1,9)-(1,10), the range of the i parameter. This range is used to try and get the range of the binding containing the range, which for this example is something like (1,4)-(1,10). This is returned and we try to walk backwards from there to the matching let/use.

I think instead of checking for immediate inclusion, since we have the start of the range in question we should be able to be a bit more precise:

• try to check the SynPat of the binding to cover the parameter case, and if it is a parameter, remove the parameter entirely. This will be interesting because we'll have to track the parens-ness of the pat in order to remove the correct range.
• if that check fails, then try the overall binding.

@cartermp any thoughts here? This kind of error should be showing in VS as well by my reading, just warning 1182 isn't enabled by default so folks may miss it.

[baronfel]

This worked out pretty well. The logic in question changed just slightly and now we support removing patterns as well:

https://github.com/baronfel/fsautocomplete/blob/887cc7ab5f74b14194d05d3bebd5fe4637e7bae4/src/FsAutoComplete/CodeFixes/RemoveUnusedBinding.fs#L19-L67