Hlint: suggest nub alternative

[philderbeast]

Fixes #11824.

When fired, this suggestion renders as:

$ cabal-install-solver/src/Distribution/Solver/Modular/Assignment.hs:56:64-69: Suggestion: Use ordNub
Found:
  nub xs
Perhaps:
  ordNub xs
Note: The growth rate of nub is O(n^2) and for ordNub it is O(n log n).  May require adding import of either;
  - Distribution.Utils.Generic from Cabal-syntax, or
  - Distribution.Simple.Utils from Cabal.

I didn't correct O(n log n) to the O(n log d) from containers haddocks for nubOrd. Neither did I suggest using that equivalent function from containers. For that, I raised #11826.

I had to add Ord instances to a few data types, BuildTarget and MatchError. I changed the Eq constraint to an Ord constraint for findMatch and nubMatches.

I left a note in .hlint.yaml to only use single letter variables in rules.

I used a separate commit for changes to tests but on approval will squash all commits before adding the merge me label.

---

• Patches conform to the coding conventions.
• Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

[jappeace]

nice!

[jappeace]

I guess the sort no longer needed

[Bodigrim]

Not quite: ordNub does not change the order of elements, so it's not semantically equivalent to drop sort. (I don't know whether sorting makes any difference for the purposes of this function and elsewhere, but perhaps we can leave figuring this out for another time)

[Bodigrim]

Not a regression, but this is wasteful. Would be nice to have a helper areUnique :: Ord a => [a] -> Bool, there is at least one more use case for it above.

[philderbeast]

Can you please be more specific?

-- Like `uniqueNames` in the way it is implemented but taking a `[a]`.
areUnique :: Ord a => [a] -> Bool
areUnique xs = length xs == length (ordNub xs)

I went looking in this repository for other [a] -> Bool helper functions and there are a few, including these ones with the same signature:

cabal/Cabal-tests/tests/UnitTests/Distribution/Compat/Graph.hs

Lines 64 to 71 in e7aaad8

	hasNoDups :: Ord a => [a] -> Bool
	hasNoDups = loop Set.empty
	where
	loop _ [] = True
	loop s (x:xs) | s' <- Set.insert x s, Set.size s' > Set.size s
	= loop s' xs
	| otherwise
	= False

cabal/cabal-install/tests/UnitTests/Distribution/Client/UserConfig.hs

Lines 93 to 96 in e7aaad8

	listUnique :: Ord a => [a] -> Bool
	listUnique xs =
	let sorted = sort xs
	in nub sorted == xs

cabal/cabal-install/tests/UnitTests/Distribution/Solver/Modular/WeightedPSQ.hs

Lines 52 to 54 in e7aaad8

	isSorted :: Ord a => [a] -> Bool
	isSorted (x1 : xs@(x2 : _)) = x1 <= x2 && isSorted xs
	isSorted _ = True

[philderbeast]

I also found these two doing the same thing, the opposite of all items being unique, all items being duplicates:

cabal/solver-benchmarks/HackageBenchmark.hs

Lines 351 to 352 in e7aaad8

	allEqual :: Eq a => [a] -> Bool
	allEqual xs = length (nub xs) == 1

cabal/cabal-install-solver/src/Distribution/Solver/Modular/Linking.hs

Lines 518 to 521 in e7aaad8

	allEqual :: Eq a => [a] -> Bool
	allEqual [] = True
	allEqual [_] = True
	allEqual (x:y:ys) = x == y && allEqual (y:ys)

Other implementations found hoogling:

• Agda's

allEqual :: Eq a => [a] -> Bool
allEqual []       = True
allEqual (x : xs) = all (== x) xs

• Extra's

allSame :: Eq a => [a] -> Bool
allSame [] = True
allSame (x:xs) = all (x ==) xs

[Bodigrim]

I mean that areUnique xs = length xs == length (ordNub xs) is a very expensive implementation. It is better than the one using nub, but still not great. You would expect such function to return False as soon as it meets the first non-unique element, but the implementation has to build ordNub xs in full until being able to compare lengths.

It should be something like

isUnique :: [a] -> Bool 
isUnique = go S.empty 
  where 
    go !_ [] = True 
    go seen (x : xs) = if S.member x seen then False else go (S.insert x seen) xs

---

allEqual expressed via nub also has bad performance, it should be like Agda's implementation.

Up to you whether you want to fix these things in this PR, it's indeed a bit of scope creep.

[Bodigrim]

Ideally this deserves @since. Same below.