haskell-nix / hnix

A Haskell re-implementation of the Nix expression language
https://hackage.haskell.org/package/hnix
BSD 3-Clause "New" or "Revised" License
756 stars 115 forks source link

NOperatorDef lacks precedence & so OperatorInfo was invented because of that #982

Open Anton-Latukha opened 3 years ago

Anton-Latukha commented 3 years ago

It is strange that data type for operator definition has everything, except avoids having predefined absolute precedence for the operator. https://github.com/haskell-nix/hnix/blob/57a55fd4874c3c8d07836367dce678d113f2f823/src/Nix/Parser.hs#L598-L604

I am certain - the OperatorInfo was invented as a side-car for it: https://github.com/haskell-nix/hnix/blob/57a55fd4874c3c8d07836367dce678d113f2f823/src/Nix/Parser.hs#L718-L724

If we look at what is special (tail) between repetitive functions: https://github.com/haskell-nix/hnix/blob/57a55fd4874c3c8d07836367dce678d113f2f823/src/Nix/Parser.hs#L724-L775

Also, since precedence is removed from the operation definition field - functions implicitly establish precedence by ordering pattern matches/constructors.

And if we de-shotgun-surgery mentioned above functions:


getOperatorInfo :: _ -> OperatorInfo
getOperatorInfo = mmap spec
 where
  mmap = (m Map.!)
   where
    m =
      Map.fromList $
        concat $
          zipWith
            (concatMap . spec)
            [1 ..]
            l2
     where
      l2 :: [[(NOperatorDef, Operator Parser NExprLoc)]]
      l2 = nixOperators $ fail "unused"
  spec i =
    \case
      (NUnaryDef op name, _) -> [(op, OperatorInfo i NAssocNone name)]
      (NBinaryDef assoc op name, _) -> [(op, OperatorInfo i assoc name)]
      (NSpecialDef assoc op name, _) -> [(op, OperatorInfo i assoc name)]

All this 3 functions are there & all they do. They take an implicitly aligned nixOperators list, and (implicitly) treat its index as operator precedence. So function just takes the NOperatorDef, implicitly figures-outs the precedence from the Haskell hardcoded list & combines that info, with a loss of the info of the type of the operation. Also (closing eyes for lazy getter nixOperators $ fail "unused") the (NUnaryDef op name, _) -> [(op, OperatorInfo i NAssocNone name)] is quite nasty, unary operators do not have associativity, & the NAssocNone name - can't be (associativity is the minimal requirement to operations in regular algebras, if there would be AssocNone far-reaching consequences on the whole algebra) NAssocNone "really" means (gets used as) "Associative" (both left & right), which also requires to be fixed.

Adding the absolute precedence to the definitions - would complete the definition, remove the OperatorInfo, reduce get{Unary,Binary,Special}Operation (which anyway need to tell what they really do), it would change Pretty module a bit - that would be pretty much it.

Anton-Latukha commented 3 years ago

NAssoc is just a nicer way of saying:

Control.Monad.Combinators.Expr.Operator
  ( Prefix
  , Postfix
  , InfixN
  , InfixR
  , InfixL
  )

As because there are only binary operators with associativity - that is fixity & unary get prefix & postfix, which actually also should be in their definition.

Anton-Latukha commented 3 years ago

The Source of this is also megaparsec makeExprParser which uses list index as precedence.

Implicit design hardcode is bad.

So list must be dismantled and constructed from definitions of precedence.