Skip to content
2 changes: 2 additions & 0 deletions hackage-server.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ common language-defaults
-Wno-deprecated-flags
-Wno-unused-do-bind
-Wno-unused-record-wildcards
-- TODO remove all Typeable derivings and remove this flag. Safe since GHC 7.10
-Wno-deriving-typeable
-Werror=incomplete-patterns
-Werror=missing-methods

Expand Down
8 changes: 4 additions & 4 deletions src/Data/StringTable.hs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import Distribution.Server.Framework.MemSize
data StringTable id
= StringTable
!BS.ByteString -- all the strings concatenated
!(A.UArray Int Word32) -- offset table
!(A.UArray Int Word32) -- ^ Invariant: the lower bound of the array is 0
deriving (Show, Typeable)

$(deriveSafeCopy 0 'base ''StringTable)
Expand All @@ -42,7 +42,7 @@ instance MemSize (StringTable id) where
lookup :: Enum id => StringTable id -> String -> Maybe id
lookup (StringTable bs tbl) str = binarySearch 0 (topBound-1) (BS.pack str)
where
(0, topBound) = A.bounds tbl
(_assumedZero, topBound) = A.bounds tbl
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is an improvement, since the algorithm now runs further if the _assumedZero isn't actually zero. But should it run further? It's basically the question of whether you want asserts in prod or not.

Personally, I think partiality is better than an incorrect result. Even though the compiler warns for partiality, but it doesn't warn for incorrectness. Of course, it would be even better to prove the algorithm correct. But that isn't always feasible, and the lack of dependent types make it extra difficult.

Copy link
Author

Choose a reason for hiding this comment

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

That's fair. How about:

  • An explicit case
  • With a call to error in the non-zero lower bound case (with a description of the violated invariant)
  • A HasCallStack constraint


binarySearch a b key
| a > b = Nothing
Expand Down Expand Up @@ -81,12 +81,12 @@ construct strs = StringTable bs tbl

enumStrings :: Enum id => StringTable id -> [String]
enumStrings (StringTable bs tbl) = map (BS.unpack . index' bs tbl) [0..h-1]
where (0,h) = A.bounds tbl
where (_assumedZero, h) = A.bounds tbl


enumIds :: Enum id => StringTable id -> [id]
enumIds (StringTable _ tbl) = map toEnum [0..h-1]
where (0,h) = A.bounds tbl
where (_assumedZero, h) = A.bounds tbl

prop :: [String] -> Bool
prop strs =
Expand Down
2 changes: 1 addition & 1 deletion src/Distribution/Client.hs
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ responseReadBSL rsp =
traverse (fmap BS.fromChunks . brConsume) rsp

httpSession :: Verbosity -> String -> Version -> HttpSession a -> IO a
httpSession verbosity agent version (HttpSession action) = do
httpSession _verbosity agent version (HttpSession action) = do
manager <- newTlsManager
let env = HttpEnv { httpManager = manager
, initialHeaders = [ (hUserAgent, BSS.pack $ agent ++ "/" ++ showVersion version) ]
Expand Down
3 changes: 2 additions & 1 deletion src/Distribution/Server/Features/Core.hs
Original file line number Diff line number Diff line change
Expand Up @@ -763,7 +763,8 @@ coreFeature ServerEnv{serverBlobStore = store} UserFeature{..}

serveCabalFileRevisionName :: DynamicPath -> ServerPartE Response
serveCabalFileRevisionName dpath = do
pkgid1 <- packageTarballInPath dpath
-- TODO bug? Maybe something to do with #1439
_pkgid1 <- packageTarballInPath dpath
pkgid2 <- packageInPath dpath
guard (pkgVersion pkgid2 == pkgVersion pkgid2)
Comment on lines +766 to 769
Copy link
Author

Choose a reason for hiding this comment

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

This is the sort of bogus code warnings are supposed to catch. I haven't yet learned enough to understand what is going on here, but surely it can't be right to compare pkgVersion pkgid2 with itself.

pkginfo <- packageInPath dpath >>= lookupPackageId
Expand Down
6 changes: 3 additions & 3 deletions src/Distribution/Server/Features/DownloadCount/State.hs
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,9 @@ updateHistory (InMemStats day perPkg) (OnDiskStats (NCM _ m)) =
updatesMap :: Map.Map PackageName OnDiskPerPkg
updatesMap = Map.fromList
[ (pkgname, applyUpdates pkgs)
| pkgs <- groupBy ((==) `on` (packageName . fst))
(cmToList perPkg :: [(PackageId, Int)])
, let pkgname = packageName (fst (head pkgs))
| pkgs@((pkgId, _):_) <- groupBy ((==) `on` (packageName . fst))
(cmToList perPkg :: [(PackageId, Int)])
, let pkgname = packageName pkgId
Comment on lines -179 to +181
Copy link
Member

Choose a reason for hiding this comment

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

Given that this is a list comprehension, a failing pattern will cause the element to be skipped. Previously, it would have been a crash. See my other comment for my reasoning of why crashes are better than incorrect results. So I don't think we should do this right now in this PR.

Copy link
Author

Choose a reason for hiding this comment

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

Well, we're leaning on the guarantee that the sublists returned by groupBy are non-empty, per the docs of groupBy and group.

There's the alternative approach of using Data.List.NonEmpty.groupBy which we certainly could take here.

Copy link
Member

Choose a reason for hiding this comment

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

That seems like it would be better since it has a more exact type!

]

applyUpdates :: [(PackageId, Int)] -> OnDiskPerPkg
Expand Down
6 changes: 3 additions & 3 deletions src/Distribution/Server/Features/EditCabalFiles.hs
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,6 @@ instance ToSElem Change where
where
-- TODO/FIXME: stringly hack
what = case what0 of
('a':'d':'d':'e':'d':_) -> 'A' : tail what0
('r':'e':'m':'o':'v':'e':'d':_) -> 'R' : tail what0
_ -> "Changed " ++ what0
('a':what1@('d':'d':'e':'d':_)) -> 'A' : what1
('r':what1@('e':'m':'o':'v':'e':'d':_)) -> 'R' : what1
_ -> "Changed " ++ what0
Comment on lines +171 to +173
Copy link
Member

Choose a reason for hiding this comment

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

This one I like, as readability is hardly impaired.

4 changes: 2 additions & 2 deletions src/Distribution/Server/Features/PackageCandidates.hs
Original file line number Diff line number Diff line change
Expand Up @@ -304,12 +304,12 @@ candidatesFeature ServerEnv{serverBlobStore = store}
where
cpiToJSON :: [CandPkgInfo] -> Value
cpiToJSON [] = Null -- should never happen
cpiToJSON pkgs = object
cpiToJSON pkgs@(pkg:_) = object
Copy link
Member

Choose a reason for hiding this comment

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

This change is good! It makes the function total, the type system recognizes the complete pattern match, and readability is not impaired.

[ Key.fromString "name" .= pn
, Key.fromString "candidates" .= pvs
]
where
pn = T.pack . display . pkgName . candInfoId . head $ pkgs
pn = T.pack . display . pkgName . candInfoId $ pkg
pvs = [ object [ Key.fromString "version" .= (T.pack . display . packageVersion . candInfoId) p
, Key.fromString "sha256" .= (blobInfoHashSHA256 . pkgTarballGz . fst) tarball
]
Expand Down
2 changes: 2 additions & 0 deletions src/Distribution/Server/Features/ReverseDependencies/State.hs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE TemplateHaskell #-}
{-# LANGUAGE TupleSections #-}
-- for instance MemSize Dependency
{-# OPTIONS_GHC -Wno-orphans #-}

module Distribution.Server.Features.ReverseDependencies.State
( NodeId
Expand Down
5 changes: 3 additions & 2 deletions src/Distribution/Server/Features/Security/Migration.hs
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,9 @@ readPrecomputedHashes env@ServerEnv{ serverVerbosity = verbosity } = do
else throwIO err

parseEntry :: String -> (MD5, (SHA256, Length))
parseEntry line = let [md5, sha256, len] = words line
in (md5, (sha256, read len))
parseEntry line = case words line of
[md5, sha256, len] -> (md5, (sha256, read len))
_ -> error $ "Invalid precomputed hash line: " <> show line

{-------------------------------------------------------------------------------
Migration infrastructure
Expand Down
5 changes: 3 additions & 2 deletions src/Distribution/Server/Packages/PackageIndex.hs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ import Distribution.Package
import Distribution.Types.Dependency
import Distribution.Version ( withinRange )
import Distribution.Simple.Utils (lowercase)
import qualified Data.List.NonEmpty as NonEmpty

-- | The collection of information about packages from one or more 'PackageDB's.
--
Expand Down Expand Up @@ -155,9 +156,9 @@ fromList pkgs = mkPackageIndex
where
fixBucket = -- out of groups of duplicates, later ones mask earlier ones
-- but Map.fromListWith (++) constructs groups in reverse order
map head
map NonEmpty.head
-- Eq instance for PackageIdentifier is wrong, so use Ord:
. groupBy (\a b -> EQ == comparing packageId a b)
. NonEmpty.groupBy (\a b -> EQ == comparing packageId a b)
-- relies on sortBy being a stable sort so we
-- can pick consistently among duplicates
. sortBy (comparing packageId)
Expand Down
7 changes: 6 additions & 1 deletion src/Distribution/Server/Util/Merge.hs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
module Distribution.Server.Util.Merge where

import Data.Map
import qualified Data.Map.Merge.Strict as MapMerge


data MergeResult a b = OnlyInLeft a | InBoth a b | OnlyInRight b
Expand All @@ -17,4 +18,8 @@ mergeBy cmp = merge
LT -> OnlyInLeft x : merge xs (y:ys)

mergeMaps :: Ord k => Map k a -> Map k b -> Map k (MergeResult a b)
mergeMaps m1 m2 = unionWith (\(OnlyInLeft a) (OnlyInRight b) -> InBoth a b) (fmap OnlyInLeft m1) (fmap OnlyInRight m2)
mergeMaps =
MapMerge.merge
(MapMerge.mapMissing $ const OnlyInLeft)
(MapMerge.mapMissing $ const OnlyInRight)
(MapMerge.zipWithMatched $ const InBoth)