Skip to content

Commit

Permalink
Ignore expression code fix (#1253)
Browse files Browse the repository at this point in the history
* Add ignore expression code fix.

* Check for multiline diagnostic sooner.

* Check new expression if it needs parentheses or not.

* Construct correct path for SynExpr.shouldBeParenthesizedInContext

* Update FCS with fix from Brian

* Update AST usage

* Add test to ignore tuple.
  • Loading branch information
nojaf authored Apr 12, 2024
1 parent 8c28496 commit b5fa4aa
Show file tree
Hide file tree
Showing 11 changed files with 201 additions and 11 deletions.
3 changes: 3 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,6 @@ fsharp_max_array_or_list_width=80
fsharp_max_dot_get_expression_width=80
fsharp_max_function_binding_width=80
fsharp_max_value_binding_width=80

[src/FsAutoComplete/CodeFixes/*.fs]
fsharp_experimental_keep_indent_in_branch = true
2 changes: 1 addition & 1 deletion paket.dependencies
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ lowest_matching: true

nuget BenchmarkDotNet 0.13.5
nuget Fantomas.Client >= 0.9
nuget FSharp.Compiler.Service >= 43.8.300-preview.24175.1
nuget FSharp.Compiler.Service >= 43.8.300-preview.24205.4
nuget Ionide.Analyzers 0.7.0
nuget FSharp.Analyzers.Build 0.3.0
nuget Ionide.ProjInfo >= 0.62.0
Expand Down
6 changes: 3 additions & 3 deletions paket.lock
Original file line number Diff line number Diff line change
Expand Up @@ -749,16 +749,16 @@ NUGET
FSharp.Core (>= 7.0.200) - restriction: || (== net6.0) (== net7.0) (== net8.0) (&& (== netstandard2.0) (>= net6.0)) (&& (== netstandard2.1) (>= net6.0))
System.Collections.Immutable (>= 6.0) - restriction: || (== net6.0) (== net7.0) (== net8.0) (&& (== netstandard2.0) (>= net6.0)) (&& (== netstandard2.1) (>= net6.0))
remote: https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json
FSharp.Compiler.Service (43.8.300-preview.24175.1)
FSharp.Core (8.0.300-beta.24175.1)
FSharp.Compiler.Service (43.8.300-preview.24205.4)
FSharp.Core (8.0.300-beta.24205.4)
System.Buffers (>= 4.5.1)
System.Collections.Immutable (>= 7.0)
System.Diagnostics.DiagnosticSource (>= 7.0.2)
System.Memory (>= 4.5.5)
System.Reflection.Emit (>= 4.7)
System.Reflection.Metadata (>= 7.0)
System.Runtime.CompilerServices.Unsafe (>= 6.0)
FSharp.Core (8.0.300-beta.24175.1)
FSharp.Core (8.0.300-beta.24205.4)

GROUP Build
STORAGE: NONE
Expand Down
2 changes: 1 addition & 1 deletion src/FsAutoComplete.Core/FCSPatches.fs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ module SyntaxTreeOps =

| SynExpr.TryFinally(tryExpr = e1; finallyExpr = e2) -> walkExpr e1 || walkExpr e2

| SynExpr.Sequential(_, _, e1, e2, _) -> walkExpr e1 || walkExpr e2
| SynExpr.Sequential(expr1 = e1; expr2 = e2) -> walkExpr e1 || walkExpr e2

| SynExpr.SequentialOrImplicitYield(_, e1, e2, _, _) -> walkExpr e1 || walkExpr e2

Expand Down
4 changes: 2 additions & 2 deletions src/FsAutoComplete.Core/TestAdapter.fs
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ let private ModuleWithSuffixType = "ModuleWithSuffix"

let rec private (|Sequentials|_|) =
function
| SynExpr.Sequential(_, _, e, Sequentials es, _) -> Some(e :: es)
| SynExpr.Sequential(_, _, e1, e2, _) -> Some [ e1; e2 ]
| SynExpr.Sequential(expr1 = e; expr2 = Sequentials es) -> Some(e :: es)
| SynExpr.Sequential(expr1 = e1; expr2 = e2) -> Some [ e1; e2 ]
| _ -> None

let getExpectoTests (ast: ParsedInput) : TestAdapterEntry<range> list =
Expand Down
4 changes: 2 additions & 2 deletions src/FsAutoComplete.Core/UntypedAstUtils.fs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ module Syntax =
/// An recursive pattern that collect all sequential expressions to avoid StackOverflowException
let rec (|Sequentials|_|) =
function
| SynExpr.Sequential(_, _, e, Sequentials es, _) -> Some(e :: es)
| SynExpr.Sequential(_, _, e1, e2, _) -> Some [ e1; e2 ]
| SynExpr.Sequential(expr1 = e; expr2 = Sequentials es) -> Some(e :: es)
| SynExpr.Sequential(expr1 = e1; expr2 = e2) -> Some [ e1; e2 ]
| _ -> None

let (|ConstructorPats|) =
Expand Down
104 changes: 104 additions & 0 deletions src/FsAutoComplete/CodeFixes/IgnoreExpression.fs
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
module FsAutoComplete.CodeFix.IgnoreExpression

open FSharp.Compiler.Syntax
open FSharp.Compiler.SyntaxTrivia
open FSharp.Compiler.Text
open FSharp.UMX
open FsToolkit.ErrorHandling
open Ionide.LanguageServerProtocol.Types
open FsAutoComplete.CodeFix.Types
open FsAutoComplete
open FsAutoComplete.LspHelpers

let title = "Ignore expression"

let fix (getParseResultsForFile: GetParseResultsForFile) : CodeFix =
Run.ifDiagnosticByCode (set [ "20" ]) (fun diagnostic (codeActionParams: CodeActionParams) ->
asyncResult {
let fileName = codeActionParams.TextDocument.GetFilePath() |> Utils.normalizePath
let fcsPos = protocolPosToPos codeActionParams.Range.Start
let mDiag = protocolRangeToRange (UMX.untag fileName) diagnostic.Range

// Only do single line for now
if mDiag.StartLine <> mDiag.EndLine then
return []
else

let! (parseAndCheckResults: ParseAndCheckResults, _line: string, sourceText: IFSACSourceText) =
getParseResultsForFile fileName fcsPos

let mExprOpt =
(fcsPos, parseAndCheckResults.GetParseResults.ParseTree)
||> ParsedInput.tryPick (fun path node ->
match node with
| SyntaxNode.SynExpr(e) when Range.equals mDiag e.Range -> Some(path, e)
| _ -> None)

match mExprOpt with
| None -> return []
| Some(path, expr) ->

let needsParentheses =
let appPipe, appIgnore =
let lineNumber = expr.Range.StartLine

let mkRangeInFile (offset: int) (length: int) : range =
Range.mkRange
expr.Range.FileName
(Position.mkPos lineNumber (expr.Range.EndColumn + offset))
(Position.mkPos lineNumber (expr.Range.EndColumn + offset + length))

let mPipe = mkRangeInFile 2 2
let mIgnore = mkRangeInFile 5 6

let appPipe =
SynExpr.App(
ExprAtomicFlag.NonAtomic,
true,
SynExpr.LongIdent(
false,
SynLongIdent(
[ FSharp.Compiler.Syntax.Ident("op_PipeRight", mPipe) ],
[],
[ Some(IdentTrivia.OriginalNotation "|>") ]
),
None,
mPipe
),
expr, // The expr that will now be piped into ignore.
Range.unionRanges expr.Range mPipe
)

let appIgnore =
SynExpr.App(
ExprAtomicFlag.NonAtomic,
false,
appPipe,
SynExpr.Ident(FSharp.Compiler.Syntax.Ident("ignore", mIgnore)),
Range.unionRanges expr.Range mIgnore
)

appPipe, appIgnore

let pathWithPipeIgnore =
SyntaxNode.SynExpr appPipe :: SyntaxNode.SynExpr appIgnore :: path

SynExpr.shouldBeParenthesizedInContext sourceText.GetLineString pathWithPipeIgnore expr

let newText =
let currentText = sourceText.GetSubTextFromRange expr.Range

if not needsParentheses then
$"%s{currentText} |> ignore"
else
$"(%s{currentText}) |> ignore"

return
[ { SourceDiagnostic = None
Title = title
File = codeActionParams.TextDocument
Edits =
[| { Range = fcsRangeToLsp expr.Range
NewText = newText } |]
Kind = FixKind.Fix } ]
})
6 changes: 6 additions & 0 deletions src/FsAutoComplete/CodeFixes/IgnoreExpression.fsi
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module FsAutoComplete.CodeFix.IgnoreExpression

open FsAutoComplete.CodeFix.Types

val title: string
val fix: getParseResultsForFile: GetParseResultsForFile -> CodeFix
3 changes: 2 additions & 1 deletion src/FsAutoComplete/LspServers/AdaptiveServerState.fs
Original file line number Diff line number Diff line change
Expand Up @@ -1925,7 +1925,8 @@ type AdaptiveState(lspClient: FSharpLspClient, sourceTextFactory: ISourceTextFac
AddTypeAliasToSignatureFile.fix forceGetFSharpProjectOptions tryGetParseAndCheckResultsForFile
UpdateTypeAbbreviationInSignatureFile.fix tryGetParseAndCheckResultsForFile
AddBindingToSignatureFile.fix forceGetFSharpProjectOptions tryGetParseAndCheckResultsForFile
ReplaceLambdaWithDotLambda.fix getLanguageVersion tryGetParseAndCheckResultsForFile |])
ReplaceLambdaWithDotLambda.fix getLanguageVersion tryGetParseAndCheckResultsForFile
IgnoreExpression.fix tryGetParseAndCheckResultsForFile |])

let forgetDocument (uri: DocumentUri) =
async {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
module private FsAutoComplete.Tests.CodeFixTests.IgnoreExpressionTests

open Expecto
open Helpers
open Utils.ServerTests
open Utils.CursorbasedTests
open FsAutoComplete.CodeFix

let tests state =
serverTestList (nameof IgnoreExpression) state defaultConfigDto None (fun server ->
[ let selectCodeFix = CodeFix.withTitle IgnoreExpression.title

testCaseAsync "ignore constant"
<| CodeFix.check
server
"
let a b =
9$0
null"
Diagnostics.acceptAll
selectCodeFix
"
let a b =
9 |> ignore
null"

testCaseAsync "ignore infix application"
<| CodeFix.check
server
"
let a b =
0 / 9$0
null"
Diagnostics.acceptAll
selectCodeFix
"
let a b =
0 / 9 |> ignore
null"

testCaseAsync "ignore member invocation"
<| CodeFix.check
server
"
open System.Collections.Generic
let foo () =
let dict = dict []
di$0ct.TryAdd(\"foo\", \"bar\")
()"
Diagnostics.acceptAll
selectCodeFix
"
open System.Collections.Generic
let foo () =
let dict = dict []
dict.TryAdd(\"foo\", \"bar\") |> ignore
()"

testCaseAsync "ignore tuple"
<| CodeFix.check
server
"
let _ =
1, 2$0
null"
Diagnostics.acceptAll
selectCodeFix
"
let _ =
(1, 2) |> ignore
null"

])
3 changes: 2 additions & 1 deletion test/FsAutoComplete.Tests.Lsp/CodeFixTests/Tests.fs
Original file line number Diff line number Diff line change
Expand Up @@ -3436,4 +3436,5 @@ let tests textFactory state =
AddTypeAliasToSignatureFileTests.tests state
UpdateTypeAbbreviationInSignatureFileTests.tests state
AddBindingToSignatureFileTests.tests state
ReplaceLambdaWithDotLambdaTests.tests state ]
ReplaceLambdaWithDotLambdaTests.tests state
IgnoreExpressionTests.tests state ]

0 comments on commit b5fa4aa

Please sign in to comment.