Skip to content

Commit

Permalink
fix precedence bug due to unlawful Ord PA instance
Browse files Browse the repository at this point in the history
  • Loading branch information
byorgey committed May 22, 2024
1 parent 25d6d61 commit 6e77ed7
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 4 deletions.
2 changes: 1 addition & 1 deletion src/Disco/Pretty.hs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ rt = local (\(PA p _) -> PA p InR)
mparens :: Member (Reader PA) r => PA -> Sem r (Doc ann) -> Sem r (Doc ann)
mparens pa doc = do
parentPA <- ask
(if pa < parentPA then parens else id) doc
(if pa `lowerPrec` parentPA then parens else id) doc

------------------------------------------------------------
-- Pretty type class
Expand Down
22 changes: 19 additions & 3 deletions src/Disco/Pretty/Prec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,26 @@ import Disco.Syntax.Operators
type Prec = Int

data PA = PA Prec BFixity
deriving (Show, Eq)
deriving (Show, Eq) -- Do NOT derive Ord, see note below.

instance Ord PA where
compare (PA p1 a1) (PA p2 a2) = compare p1 p2 `mappend` (if a1 == a2 then EQ else LT)
lowerPrec :: PA -> PA -> Bool
lowerPrec (PA p1 a1) (PA p2 a2) = p1 < p2 || (p1 == p2 && a1 /= a2)

-- Note re: lowerPrec: we used to have an unlawful Ord instance defined by
--
-- compare (PA p1 a1) (PA p2 a2) = compare p1 p2 `mappend` (if a1 == a2 then EQ else LT)
--
-- with the idea that we could test whether one precedence was lower
-- than another simply using (<).
--
-- However, this was unlawful since e.g. it does not satisfy x < y ==
-- y > x: If x and y have the same Prec value but different BFixity
-- values, we would have both x < y and y < x.
--
-- In base-4.18 apparently something in the default implementations of
-- Ord methods changed so that e.g. not (PA 2 InR < PA 2 InL). Hence
-- the 'mparens' method in such cases of nested same-precedence
-- operators was not emitting parentheses in cases where it should.

-- Standard precedence levels

Expand Down

0 comments on commit 6e77ed7

Please sign in to comment.