Skip to content

Commit

Permalink
Fix else keyword not being part of SynIfThenElse range. Fixes #713 (#724
Browse files Browse the repository at this point in the history
)
  • Loading branch information
nojaf authored Mar 14, 2020
1 parent bc86642 commit c0faf0d
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 30 deletions.
64 changes: 64 additions & 0 deletions src/Fantomas.Tests/IfThenElseTests.fs
Original file line number Diff line number Diff line change
Expand Up @@ -804,4 +804,68 @@ module String =
if la <> lb then if la > lb then a' else b'
else if String.length a' < String.length b' then a'
else b'
"""

[<Test>]
let ``second else if where else and if are on separate lines, 713`` () =
formatSourceString false """if v1 < v2 then
-1
elif v1 > v2 then
1
else
if t1 < t2 then
-1
elif t1 > t2 then
1
else
0
""" config
|> prepend newline
|> should equal """
if v1 < v2 then -1
elif v1 > v2 then 1
else if t1 < t2 then -1
elif t1 > t2 then 1
else 0
"""

[<Test>]
let ``newline between else if, prior by elif`` () =
formatSourceString false """
module String =
let merge a b =
if la <> lb then
if la > lb then a' else b'
elif la = lb then a'
else
if String.length a' < String.length b' then a' else b'
""" config
|> prepend newline
|> should equal """
module String =
let merge a b =
if la <> lb then if la > lb then a' else b'
elif la = lb then a'
else if String.length a' < String.length b' then a'
else b'
"""

[<Test>]
let ``newline between else if, followed by else if`` () =
formatSourceString false """
module String =
let merge a b =
if la <> lb then
if la > lb then a' else b'
else
if String.length a' < String.length b' then a' else if String.length a' > String.length b' then b' else b'
""" config
|> prepend newline
|> should equal """
module String =
let merge a b =
if la <> lb then if la > lb then a' else b'
else if String.length a' < String.length b' then a'
else if String.length a' > String.length b' then b'
else b'
"""
52 changes: 22 additions & 30 deletions src/Fantomas/CodePrinter.fs
Original file line number Diff line number Diff line change
Expand Up @@ -1151,46 +1151,38 @@ and genExpr astContext synExpr =
| ElIf((e1,e2, _, _, _)::es, enOpt) ->
// https://docs.microsoft.com/en-us/dotnet/fsharp/style-guide/formatting#formatting-if-expressions
fun ctx ->
let elseKeywordRange =
ctx.Trivia
|> TriviaHelpers.``keyword tokens inside range`` ["IF"; "THEN"; "ELIF"; "ELSE"] synExpr.Range
|> fun tokens ->
// skip if .. then and take first else keyword
// ignore if third keyword is elif f.ex.

match synExpr with
// elif keyword matches the start of the optional else expression
| SynExpr.IfThenElse(_, _, Some(optionalExprElse), _, _, _, _)
when (List.exists (fun ({ TokenName = tn }: FSharpTokenInfo,t) -> tn = "ELIF" && t.Range.Start = optionalExprElse.Range.Start) tokens) ->
None
// the else keyword is floating between the thenExpression and the optional elseExpression
| SynExpr.IfThenElse(_, exprThen, Some(_), _, _, _, _) ->
let filtered =
tokens
|> List.filter (fun ({ TokenName = tn }, t) -> tn = "ELSE" && RangeHelpers.``range starts after`` exprThen.Range t.Range)
|> List.tryHead
|> Option.map (fun (_,t) -> t.Range)
filtered

let correctedElifRanges =
es
|> List.pairwise
|> List.map (fun ((_,beforeNode,_,_,_),(_,_,_,_, node)) -> (beforeNode.Range, node.Range))
|> fun tail ->
match es with
| (_,_,_,_,hn)::_ -> (e2.Range, hn.Range)::tail
| _ -> tail
|> List.indexed
|> List.choose(fun (idx, (beforeRange, elseIfRange)) ->
let rangeBetween = mkRange "between" beforeRange.End elseIfRange.Start
let keywordsFoundInBetween = TriviaHelpers.``keyword tokens inside range`` ["ELSE"] rangeBetween ctx.Trivia
match List.tryHead keywordsFoundInBetween with
| Some (_, elseKeyword) ->
(idx, mkRange "else if" elseKeyword.Range.Start elseIfRange.End)
|> Some
| _ ->
None
)
|> Map.ofList

let elfis =
let lastEsIndex = (List.length es) - 1
List.indexed es
|> List.map (fun (idx, (elf1, elf2, _, fullRange, _)) ->
if idx = lastEsIndex then
// In some scenarios the last else keyword of the 'else if' combination is not included in inner IfThenElse syn expr
// f.ex if a then b else // meh
// if c then d else e
let correctedRange =
match elseKeywordRange with
| Some er -> mkRange "else if" er.Start fullRange.End
| None -> fullRange
(elf1, elf2, correctedRange)
else
(elf1, elf2, fullRange)
)
Map.tryFind idx correctedElifRanges
|> Option.defaultValue fullRange

(elf1, elf2, correctedRange))

let hasElfis = not (List.isEmpty elfis)

Expand Down

0 comments on commit c0faf0d

Please sign in to comment.