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.

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

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

Open
Anton-Latukha opened this issue Aug 10, 2021 · 2 comments

Comments

@Anton-Latukha
Copy link
Collaborator

Anton-Latukha commented Aug 10, 2021

It is strange that data type for operator definition has everything, except avoids having predefined absolute precedence for the operator.

hnix/src/Nix/Parser.hs

Lines 598 to 604 in 57a55fd

data NOperatorDef
= NUnaryDef Text NUnaryOp
| NBinaryDef Text NBinaryOp NAssoc
| NSpecialDef Text NSpecialOp NAssoc
deriving (Eq, Ord, Generic, Typeable, Data, Show, NFData)

I am certain - the OperatorInfo was invented as a side-car for it:

hnix/src/Nix/Parser.hs

Lines 718 to 724 in 57a55fd

data OperatorInfo = OperatorInfo
{ precedence :: Int
, associativity :: NAssoc
, operatorName :: Text
} deriving (Eq, Ord, Generic, Typeable, Data, Show)

If we look at what is special (tail) between repetitive functions:

hnix/src/Nix/Parser.hs

Lines 724 to 775 in 57a55fd

getUnaryOperator :: NUnaryOp -> OperatorInfo
getUnaryOperator = (m Map.!)
where
m =
Map.fromList $
concat $
zipWith
buildEntry
[1 ..]
(nixOperators $ fail "unused")
buildEntry i =
concatMap $
\case
(NUnaryDef name op, _) -> [(op, OperatorInfo i NAssocNone name)]
_ -> mempty
getBinaryOperator :: NBinaryOp -> OperatorInfo
getBinaryOperator = (m Map.!)
where
m =
Map.fromList $
concat $
zipWith
buildEntry
[1 ..]
(nixOperators $ fail "unused")
buildEntry i =
concatMap $
\case
(NBinaryDef name op assoc, _) -> [(op, OperatorInfo i assoc name)]
_ -> mempty
getSpecialOperator :: NSpecialOp -> OperatorInfo
getSpecialOperator NSelectOp = OperatorInfo 1 NAssocLeft "."
getSpecialOperator o = m Map.! o
where
m =
Map.fromList $
concat $
zipWith
buildEntry
[1 ..]
(nixOperators $ fail "unused")
buildEntry i =
concatMap $
\case
(NSpecialDef name op assoc, _) -> [(op, OperatorInfo i assoc name)]
_ -> mempty

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant