diff --git a/src/Simplify.elm b/src/Simplify.elm index 87f915b95..ec55a6b5b 100644 --- a/src/Simplify.elm +++ b/src/Simplify.elm @@ -1375,8 +1375,14 @@ expressionVisitorHelp node context = Expression.RecordUpdateExpression (Node recordNameRange _) setters -> onlyErrors (recordAccessChecks (Node.range node) (Just recordNameRange) (Node.value field) setters) - Expression.LetExpression { expression } -> - onlyErrors (recordAccessLetInChecks (Node.range node) field expression) + Expression.LetExpression _ -> + onlyErrors (distributeFieldAccess True "a let/in" record field) + + Expression.IfBlock _ _ _ -> + onlyErrors (distributeFieldAccess False "an if/then/else" record field) + + Expression.CaseExpression _ -> + onlyErrors (distributeFieldAccess False "a case/of" record field) _ -> onlyErrors [] @@ -1385,6 +1391,88 @@ expressionVisitorHelp node context = onlyErrors [] +distributeFieldAccess : Bool -> String -> Node Expression -> Node String -> List (Error {}) +distributeFieldAccess isLet kind ((Node recordRange _) as record) (Node fieldRange fieldName) = + case recordLeavesRanges record of + { records, withoutParens, withParens } -> + if not (List.isEmpty withParens && List.isEmpty withoutParens) && 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 + ) + ] + + +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 + } + + +recordLeavesRanges : + Node Expression + -> { records : List Range, withoutParens : List Range, withParens : List Range } +recordLeavesRanges (Node range expr) = + case expr of + Expression.IfBlock _ thenNode elseNode -> + combineRecordLeavesRanges + (recordLeavesRanges thenNode) + (recordLeavesRanges elseNode) + + Expression.LetExpression { expression } -> + recordLeavesRanges expression + + Expression.ParenthesizedExpression child -> + recordLeavesRanges child + + Expression.CaseExpression { cases } -> + List.foldl + (\( _, e ) -> combineRecordLeavesRanges (recordLeavesRanges e)) + { records = [], withParens = [], withoutParens = [] } + cases + + Expression.RecordExpr _ -> + { records = [ range ], withParens = [], withoutParens = [] } + + Expression.RecordAccess _ _ -> + { records = [], withParens = [], withoutParens = [ range ] } + + Expression.RecordUpdateExpression _ _ -> + { records = [ range ], withParens = [], withoutParens = [] } + + Expression.FunctionOrValue _ _ -> + { records = [], withParens = [], withoutParens = [ range ] } + + _ -> + { records = [], withParens = [ range ], withoutParens = [] } + + recordAccessChecks : Range -> Maybe Range -> String -> List (Node RecordSetter) -> List (Error {}) recordAccessChecks nodeRange recordNameRange fieldName setters = case @@ -1465,39 +1553,6 @@ needsParens expr = False -recordAccessLetInChecks : Range -> Node String -> Node Expression -> List (Error {}) -recordAccessLetInChecks nodeRange (Node fieldRange fieldName) expr = - let - fieldRangeStart : Location - fieldRangeStart = - fieldRange.start - - fieldRemovalFix : Fix - fieldRemovalFix = - Fix.removeRange - { start = { row = fieldRangeStart.row, column = fieldRangeStart.column - 1 } - , end = fieldRange.end - } - in - [ Rule.errorWithFix - { message = "Field access can be simplified" - , details = [ "Accessing the field outside a let expression can be simplified to access the field inside it" ] - } - nodeRange - (if needsParens (Node.value expr) then - [ Fix.insertAt (Node.range expr).start "(" - , Fix.insertAt (Node.range expr).end (")." ++ fieldName) - , fieldRemovalFix - ] - - else - [ Fix.insertAt (Node.range expr).end ("." ++ fieldName) - , fieldRemovalFix - ] - ) - ] - - type alias CheckInfo = { lookupTable : ModuleNameLookupTable , inferredConstants : ( Infer.Inferred, List Infer.Inferred ) diff --git a/tests/SimplifyTest.elm b/tests/SimplifyTest.elm index 376492565..285302438 100644 --- a/tests/SimplifyTest.elm +++ b/tests/SimplifyTest.elm @@ -10443,49 +10443,81 @@ a = d.c , test "should simplify record accesses for let/in expressions" <| \() -> """module A exposing (..) +a = (let b = c in { e = 3 }).e +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = [ "Accessing the field outside a let/in expression can be simplified to access the field inside it" ] + , under = ".e" + } + |> Review.Test.whenFixed """module A exposing (..) +a = (let b = c in { e = 3 }.e) +""" + ] + , test "should simplify record accesses for let/in expressions, even if the leaf is not a record expression" <| + \() -> + """module A exposing (..) a = (let b = c in f x).e """ |> Review.Test.run (rule defaults) |> Review.Test.expectErrors [ Review.Test.error { message = "Field access can be simplified" - , details = [ "Accessing the field outside a let expression can be simplified to access the field inside it" ] - , under = "(let b = c in f x).e" + , details = [ "Accessing the field outside a let/in expression can be simplified to access the field inside it" ] + , under = ".e" } |> Review.Test.whenFixed """module A exposing (..) a = (let b = c in (f x).e) +""" + ] + , test "should simplify record accesses for let/in expressions, even if the leaf is not a record expression, without adding unnecessary parentheses" <| + \() -> + """module A exposing (..) +a = (let b = c in x).e +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = [ "Accessing the field outside a let/in expression can be simplified to access the field inside it" ] + , under = ".e" + } + |> Review.Test.whenFixed """module A exposing (..) +a = (let b = c in x.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 [ Review.Test.error { message = "Field access can be simplified" - , details = [ "Accessing the field outside a let expression can be simplified to access the field inside it" ] - , under = "(((let b = c in f x))).e" + , details = [ "Accessing the field outside a let/in expression can be simplified to access the field inside it" ] + , 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 [ Review.Test.error { message = "Field access can be simplified" - , details = [ "Accessing the field outside a let expression can be simplified to access the field inside it" ] - , under = "(let b = c in f x).e" + , details = [ "Accessing the field outside a let/in expression can be simplified to access the field inside it" ] + , 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)" <| @@ -10497,11 +10529,70 @@ a = (let b = c in (f x).e).f |> Review.Test.expectErrors [ Review.Test.error { message = "Field access can be simplified" - , details = [ "Accessing the field outside a let expression can be simplified to access the field inside it" ] - , under = "(let b = c in (f x).e).f" + , details = [ "Accessing the field outside a let/in expression can be simplified to access the field inside it" ] + , under = ".f" } |> Review.Test.whenFixed """module A exposing (..) a = (let b = c in (f x).e.f) +""" + ] + , test "should simplify record accesses for if/then/else expressions" <| + \() -> + """module A exposing (..) +a = (if x then { f = 3 } else { z | f = 3 }).f +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = [ "Accessing the field outside an if/then/else expression can be simplified to access the field inside it" ] + , under = ".f" + } + |> Review.Test.whenFixed """module A exposing (..) +a = (if x then { f = 3 }.f else { z | f = 3 }.f) +""" + ] + , test "should not simplify record accesses if some branches are not records" <| + \() -> + """module A exposing (..) +a = (if x then a else { f = 3 }).f +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectNoErrors + , test "should simplify record accesses for nested if/then/else expressions" <| + \() -> + """module A exposing (..) +a = (if x then { f = 3 } else if y then { z | f = 4 } else { z | f = 3 }).f +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = [ "Accessing the field outside an if/then/else expression can be simplified to access the field inside it" ] + , under = ".f" + } + |> Review.Test.whenFixed """module A exposing (..) +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 {f = 2} else + case b of Nothing -> { f = 4 } + Just _ -> { f = 5 }).f +""" + |> Review.Test.run (rule defaults) + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Field access can be simplified" + , details = [ "Accessing the field outside an if/then/else expression can be simplified to access the field inside it" ] + , under = ".f" + } + |> Review.Test.whenFixed """module A exposing (..) +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) """ ] ]