Skip to content

Commit

Permalink
Only apply simplification if all branches are records
Browse files Browse the repository at this point in the history
  • Loading branch information
@miniBill
miniBill committed Sep 5, 2022
1 parent 6aa4cee commit 072f175
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 64 deletions.
89 changes: 35 additions & 54 deletions src/Simplify.elm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1376,13 +1376,13 @@ expressionVisitorHelp node context =
onlyErrors (recordAccessChecks (Node.range node) (Just recordNameRange) (Node.value field) setters)

Expression.LetExpression _ ->
onlyErrors (distributeFieldAccess True "a let/in" record field)
onlyErrors (distributeFieldAccess "a let/in" record field)

Expression.IfBlock _ _ _ ->
onlyErrors (distributeFieldAccess False "an if/then/else" record field)
onlyErrors (distributeFieldAccess "an if/then/else" record field)

Expression.CaseExpression _ ->
onlyErrors (distributeFieldAccess False "a case/of" record field)
onlyErrors (distributeFieldAccess "a case/of" record field)

_ ->
onlyErrors []
Expand All @@ -1391,57 +1391,41 @@ expressionVisitorHelp node context =
onlyErrors []


distributeFieldAccess : Bool -> String -> Node Expression -> Node String -> List (Error {})
distributeFieldAccess isLet kind ((Node recordRange _) as record) (Node fieldRange fieldName) =
distributeFieldAccess : String -> Node Expression -> Node String -> List (Error {})
distributeFieldAccess kind ((Node recordRange _) as record) (Node fieldRange fieldName) =
case recordLeavesRanges record of
{ records, withoutParens, withParens } ->
if List.isEmpty records && not isLet then
[]

else
[ let
removalRange : Range
removalRange =
{ start = recordRange.end, end = fieldRange.end }
in
Rule.errorWithFix
{ message = "Field access can be simplified"
, details = [ "Accessing the field outside " ++ kind ++ " expression can be simplified to access the field inside it" ]
}
removalRange
(Fix.removeRange removalRange
:: List.map
(\leafRange -> Fix.insertAt leafRange.end ("." ++ fieldName))
(withoutParens ++ records)
++ List.concatMap
(\leafRange ->
[ Fix.insertAt leafRange.start "("
, Fix.insertAt leafRange.end (")." ++ fieldName)
]
)
withParens
)
]
Nothing ->
[]

Just [] ->
[]

combineRecordLeavesRanges :
{ records : List Range, withoutParens : List Range, withParens : List Range }
-> { records : List Range, withoutParens : List Range, withParens : List Range }
-> { records : List Range, withoutParens : List Range, withParens : List Range }
combineRecordLeavesRanges left right =
{ records = left.records ++ right.records
, withoutParens = left.withoutParens ++ right.withoutParens
, withParens = left.withParens ++ right.withParens
}
Just ranges ->
[ let
removalRange : Range
removalRange =
{ start = recordRange.end, end = fieldRange.end }
in
Rule.errorWithFix
{ message = "Field access can be simplified"
, details = [ "Accessing the field outside " ++ kind ++ " expression can be simplified to access the field inside it" ]
}
removalRange
(Fix.removeRange removalRange
:: List.map
(\leafRange -> Fix.insertAt leafRange.end ("." ++ fieldName))
ranges
)
]


recordLeavesRanges :
Node Expression
-> { records : List Range, withoutParens : List Range, withParens : List Range }
-> Maybe (List Range)
recordLeavesRanges (Node range expr) =
case expr of
Expression.IfBlock _ thenNode elseNode ->
combineRecordLeavesRanges
Maybe.map2 (++)
(recordLeavesRanges thenNode)
(recordLeavesRanges elseNode)

Expand All @@ -1453,24 +1437,21 @@ recordLeavesRanges (Node range expr) =

Expression.CaseExpression { cases } ->
List.foldl
(\( _, e ) -> combineRecordLeavesRanges (recordLeavesRanges e))
{ records = [], withParens = [], withoutParens = [] }
(\( _, e ) -> Maybe.map2 (++) (recordLeavesRanges e))
(Just [])
cases

Expression.RecordExpr _ ->
{ records = [ range ], withParens = [], withoutParens = [] }

Expression.RecordAccess _ _ ->
{ records = [], withParens = [], withoutParens = [ range ] }
Just [ range ]

Expression.RecordUpdateExpression _ _ ->
{ records = [ range ], withParens = [], withoutParens = [] }
Just [ range ]

Expression.FunctionOrValue _ _ ->
{ records = [], withParens = [], withoutParens = [ range ] }
Expression.RecordAccess _ _ ->
Just [ range ]

_ ->
{ records = [], withParens = [ range ], withoutParens = [] }
Nothing


recordAccessChecks : Range -> Maybe Range -> String -> List (Node RecordSetter) -> List (Error {})
Expand Down
20 changes: 10 additions & 10 deletions tests/SimplifyTest.elm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10443,7 +10443,7 @@ a = d.c
, test "should simplify record accesses for let/in expressions" <|
\() ->
"""module A exposing (..)
a = (let b = c in f x).e
a = (let b = c in { e = 3 }).e
"""
|> Review.Test.run (rule defaults)
|> Review.Test.expectErrors
Expand All @@ -10453,13 +10453,13 @@ a = (let b = c in f x).e
, under = ".e"
}
|> Review.Test.whenFixed """module A exposing (..)
a = (let b = c in (f x).e)
a = (let b = c in { e = 3 }.e)
"""
]
, test "should simplify record accesses for let/in expressions in parentheses" <|
\() ->
"""module A exposing (..)
a = (((let b = c in f x))).e
a = (((let b = c in {e = 2}))).e
"""
|> Review.Test.run (rule defaults)
|> Review.Test.expectErrors
Expand All @@ -10469,13 +10469,13 @@ a = (((let b = c in f x))).e
, under = ".e"
}
|> Review.Test.whenFixed """module A exposing (..)
a = (((let b = c in (f x).e)))
a = (((let b = c in {e = 2}.e)))
"""
]
, test "should simplify nested record accesses for let/in expressions (inner)" <|
\() ->
"""module A exposing (..)
a = (let b = c in f x).e.f
a = (let b = c in { e = { f = 2 } }).e.f
"""
|> Review.Test.run (rule defaults)
|> Review.Test.expectErrors
Expand All @@ -10485,7 +10485,7 @@ a = (let b = c in f x).e.f
, under = ".e"
}
|> Review.Test.whenFixed """module A exposing (..)
a = (let b = c in (f x).e).f
a = (let b = c in { e = { f = 2 } }.e).f
"""
]
, test "should simplify nested record accesses for let/in expressions (outer)" <|
Expand Down Expand Up @@ -10520,10 +10520,10 @@ a = (if x then { f = 3 } else { z | f = 3 }).f
a = (if x then { f = 3 }.f else { z | f = 3 }.f)
"""
]
, test "should not simplify record accesses if there are no records" <|
, test "should not simplify record accesses if some branches are not records" <|
\() ->
"""module A exposing (..)
a = (if x then a else b).f
a = (if x then a else { f = 3 }).f
"""
|> Review.Test.run (rule defaults)
|> Review.Test.expectNoErrors
Expand All @@ -10546,7 +10546,7 @@ a = (if x then { f = 3 }.f else if y then { z | f = 4 }.f else { z | f = 3 }.f)
, test "should simplify record accesses for mixed if/then/else and case expressions" <|
\() ->
"""module A exposing (..)
a = (if x then { f = 3 } else if y then q z else
a = (if x then { f = 3 } else if y then {f = 2} else
case b of Nothing -> { f = 4 }
Just _ -> { f = 5 }).f
"""
Expand All @@ -10558,7 +10558,7 @@ a = (if x then { f = 3 } else if y then q z else
, under = ".f"
}
|> Review.Test.whenFixed """module A exposing (..)
a = (if x then { f = 3 }.f else if y then (q z).f else
a = (if x then { f = 3 }.f else if y then {f = 2}.f else
case b of Nothing -> { f = 4 }.f
Just _ -> { f = 5 }.f)
"""
Expand Down

0 comments on commit 072f175

Please sign in to comment.