Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dict.fromList & Set.fromList duplicate key merge #328

Merged
merged 3 commits into from
Dec 27, 2024
Merged

Dict.fromList & Set.fromList duplicate key merge #328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bf22737
merge set and dict fromList elements with equal key
lue-bird Dec 22, 2024
67f5031
correct Dict.fromList duplicate key detection
lue-bird Dec 22, 2024
89e1e23
correct existing tests to include Set and Dict fromList merges
lue-bird Dec 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## [Unreleased]

The rule also simplifies:
- `Set.fromList [ a, a ]` to `Set.fromList [ a ]`
- `Dict.fromList [ ( a, v0 ), ( a, v1 ) ]` to `Dict.fromList [ ( a, v1 ) ]`

## [2.1.5] - 2024-06-28

The rule also simplifies (thanks to [@morteako]):
Expand Down
157 changes: 157 additions & 0 deletions src/Simplify.elm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1011,6 +1011,9 @@ Destructuring using case expressions
Set.fromList (Set.toList set)
--> set

Set.fromList [ a, a ]
--> Set.fromList [ a ]

Set.map f Set.empty -- same for Set.filter, Set.remove...
--> Set.empty

Expand Down Expand Up @@ -1097,6 +1100,9 @@ Destructuring using case expressions
Dict.fromList (Dict.toList dict)
--> dict

Dict.fromList [ ( key, a ), ( key, b ) ]
--> Dict.fromList [ ( key, b ) ]

Dict.isEmpty Dict.empty
--> True

Expand Down Expand Up @@ -5998,6 +6004,69 @@ setFromListChecks =
[ intoFnCheckOnlyCall (emptiableFromListChecks setCollection)
, wrapperFromListSingletonChecks setCollection
, onSpecificFnCallReturnsItsLastArgCheck Fn.Set.toList
, intoFnCheckOnlyCall
(\checkInfo ->
if checkInfo.expectNaN then
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Later improvement: I guess we could do this check but only for literals if expectNaN is True.

Nothing

else
case Node.value checkInfo.firstArg of
Expression.ListExpr elements ->
let
allDifferent :
Result
{ keyRange : Range, nextKeyRange : Range }
(List (Node Expression))
allDifferent =
List.foldl
(\key otherKeysToCheckOrFoundDuplicate ->
case otherKeysToCheckOrFoundDuplicate of
Err foundDuplicate ->
Err foundDuplicate

Ok otherKeysToCheck ->
case otherKeysToCheck of
(Node nextKeyRange _) :: _ ->
if Normalize.isAnyTheSameAs checkInfo key otherKeysToCheck then
Err
{ keyRange = Node.range key
, nextKeyRange = nextKeyRange
}

else
Ok (otherKeysToCheck |> List.drop 1)

[] ->
-- key is the last element
-- so it can't be equal to any other key
Ok []
)
(Ok (List.drop 1 elements))
elements
in
case allDifferent of
Ok _ ->
Nothing

Err firstDuplicateKey ->
Just
(Rule.errorWithFix
{ message =
"Set.fromList on a list with a duplicate key will only keep one of them"
, details =
[ "Maybe one of the keys was supposed to be a different value? If not, you can remove one of the duplicate keys." ]
}
firstDuplicateKey.keyRange
[ Fix.removeRange
{ start = firstDuplicateKey.keyRange.start
, end = firstDuplicateKey.nextKeyRange.start
}
]
)

_ ->
Nothing
)
]


Expand Down Expand Up @@ -6090,6 +6159,94 @@ dictFromListChecks =
intoFnChecksFirstThatConstructsError
[ intoFnCheckOnlyCall (emptiableFromListChecks dictCollection)
, onSpecificFnCallReturnsItsLastArgCheck Fn.Dict.toList
, intoFnCheckOnlyCall
(\checkInfo ->
if checkInfo.expectNaN then
Nothing

else
case Node.value checkInfo.firstArg of
Expression.ListExpr elements ->
let
knownEntryTuples : List { entryRange : Range, first : Node Expression }
knownEntryTuples =
elements
|> List.filterMap
(\entry ->
case AstHelpers.getTuple2 checkInfo.lookupTable entry of
Nothing ->
Nothing

Just otherEntryTupleParts ->
Just
{ entryRange = Node.range entry
, first = otherEntryTupleParts.first
}
)

allDifferent :
Result
{ entryRange : Range
, keyRange : Range
, nextEntryRange : Range
}
(List { entryRange : Range, first : Node Expression })
allDifferent =
List.foldl
(\entry otherEntriesToCheckOrFoundDuplicate ->
case otherEntriesToCheckOrFoundDuplicate of
Err foundDuplicate ->
Err foundDuplicate

Ok otherEntriesToCheck ->
case otherEntriesToCheck of
nextEntry :: _ ->
if
Normalize.isAnyTheSameAsBy checkInfo
entry.first
.first
otherEntriesToCheck
then
Err
{ entryRange = entry.entryRange
, keyRange = Node.range entry.first
, nextEntryRange = nextEntry.entryRange
}

else
Ok (otherEntriesToCheck |> List.drop 1)

[] ->
-- entry is the last element
-- so it can't be equal to any other key
Ok []
)
(Ok (List.drop 1 knownEntryTuples))
knownEntryTuples
in
case allDifferent of
Ok _ ->
Nothing

Err firstDuplicateKey ->
Just
(Rule.errorWithFix
{ message =
"Dict.fromList on entries with a duplicate key will only keep the last entry"
, details =
[ "Maybe one of the keys was supposed to be a different value? If not, you can remove earlier entries with duplicate keys." ]
}
firstDuplicateKey.keyRange
[ Fix.removeRange
{ start = firstDuplicateKey.entryRange.start
, end = firstDuplicateKey.nextEntryRange.start
}
]
)

_ ->
Nothing
)
]


Expand Down
31 changes: 30 additions & 1 deletion src/Simplify/Normalize.elm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
module Simplify.Normalize exposing (Comparison(..), areAllTheSame, compare, compareWithoutNormalization, getNumberValue, normalize)
module Simplify.Normalize exposing (Comparison(..), areAllTheSame, compare, compareWithoutNormalization, getNumberValue, isAnyTheSameAs, isAnyTheSameAsBy, normalize)

import Dict
import Elm.Syntax.Expression as Expression exposing (Expression)
Expand All @@ -21,6 +21,35 @@ areAllTheSame resources first rest =
List.all (\node -> normalize resources node == normalizedFirst) rest


isAnyTheSameAs : Infer.Resources a -> Node Expression -> List (Node Expression) -> Bool
isAnyTheSameAs resources first rest =
let
normalizedFirst : Node Expression
normalizedFirst =
normalize resources first
in
List.any (\node -> normalize resources node == normalizedFirst) rest


isAnyTheSameAsBy :
Infer.Resources a
-> Node Expression
-> (element -> Node Expression)
-> List element
-> Bool
isAnyTheSameAsBy resources first restElementToNode rest =
let
normalizedFirst : Node Expression
normalizedFirst =
normalize resources first
in
List.any
(\node ->
normalize resources (restElementToNode node) == normalizedFirst
)
rest


normalize : Infer.Resources a -> Node Expression -> Node Expression
normalize resources node =
case Node.value node of
Expand Down
71 changes: 71 additions & 0 deletions tests/Simplify/DictTest.elm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,55 @@ a = (Dict.fromList >> f >> g) << Dict.toList
|> Review.Test.whenFixed """module A exposing (..)
import Dict
a = (f >> g)
"""
]
, test "should not report Dict.fromList with duplicate keys when expecting NaN" <|
\() ->
"""module A exposing (..)
import Dict
a = Dict.fromList [ ( key, v0 ), ( key, v1 ) ]
b = Dict.fromList [ ( ( 1, "", [ 'a' ] ), v0 ), ( ( 1, "", [ 'a' ] ), v1 ) ]
"""
|> Review.Test.run TestHelpers.ruleExpectingNaN
|> Review.Test.expectNoErrors
, test "should replace Dict.fromList [ ( key, v0 ), ( key, v1 ) ] by Dict.fromList [ ( key, v1 ) ]" <|
\() ->
"""module A exposing (..)
import Dict
a = Dict.fromList [ ( key, v0 ), ( key, v1 ) ]
"""
|> Review.Test.run ruleWithDefaults
|> Review.Test.expectErrors
[ Review.Test.error
{ message = "Dict.fromList on entries with a duplicate key will only keep the last entry"
, details = [ "Maybe one of the keys was supposed to be a different value? If not, you can remove earlier entries with duplicate keys." ]
, under = "key"
}
|> Review.Test.atExactly
{ start = { row = 3, column = 23 }, end = { row = 3, column = 26 } }
|> Review.Test.whenFixed """module A exposing (..)
import Dict
a = Dict.fromList [ ( key, v1 ) ]
"""
]
, test "should replace Dict.fromList [ ( ( 1, \"\", [ 'a' ] ), v0 ), ( ( 1, \"\", [ 'a' ] ), v1 ) ] by Dict.fromList [ ( ( 1, \"\", [ 'a' ] ), v1 ) ]" <|
\() ->
"""module A exposing (..)
import Dict
a = Dict.fromList [ ( ( 1, "", [ 'a' ] ), v0 ), ( ( 1, "", [ 'a' ] ), v1 ) ]
"""
|> Review.Test.run ruleWithDefaults
|> Review.Test.expectErrors
[ Review.Test.error
{ message = "Dict.fromList on entries with a duplicate key will only keep the last entry"
, details = [ "Maybe one of the keys was supposed to be a different value? If not, you can remove earlier entries with duplicate keys." ]
, under = """( 1, "", [ 'a' ] )"""
}
|> Review.Test.atExactly
{ start = { row = 3, column = 23 }, end = { row = 3, column = 41 } }
|> Review.Test.whenFixed """module A exposing (..)
import Dict
a = Dict.fromList [ ( ( 1, "", [ 'a' ] ), v1 ) ]
"""
]
]
Expand Down Expand Up @@ -528,6 +577,17 @@ a = Dict.size (Dict.fromList [(1,1), (2,1), (3,1), (3,2), (0x3,2)])
|> Review.Test.whenFixed """module A exposing (..)
import Dict
a = 3
"""
, Review.Test.error
{ message = "Dict.fromList on entries with a duplicate key will only keep the last entry"
, details = [ "Maybe one of the keys was supposed to be a different value? If not, you can remove earlier entries with duplicate keys." ]
, under = "3"
}
|> Review.Test.atExactly
{ start = { row = 3, column = 46 }, end = { row = 3, column = 47 } }
|> Review.Test.whenFixed """module A exposing (..)
import Dict
a = Dict.size (Dict.fromList [(1,1), (2,1), (3,2), (0x3,2)])
"""
]
, test "should replace Dict.size (Dict.fromList [(1.3,()), (-1.3,()), (2.1,()), (2.1,())]) by 3" <|
Expand All @@ -546,6 +606,17 @@ a = Dict.size (Dict.fromList [(1.3,()), (-1.3,()), (2.1,()), (2.1,())])
|> Review.Test.whenFixed """module A exposing (..)
import Dict
a = 3
"""
, Review.Test.error
{ message = "Dict.fromList on entries with a duplicate key will only keep the last entry"
, details = [ "Maybe one of the keys was supposed to be a different value? If not, you can remove earlier entries with duplicate keys." ]
, under = "2.1"
}
|> Review.Test.atExactly
{ start = { row = 3, column = 53 }, end = { row = 3, column = 56 } }
|> Review.Test.whenFixed """module A exposing (..)
import Dict
a = Dict.size (Dict.fromList [(1.3,()), (-1.3,()), (2.1,())])
"""
]
]
Expand Down
Loading
Loading