[New module system] Fixity rebalancing broken

[m-yac]

In #1363 tests/regression/repl-decls.icry fails because:

let x -<- y = x - y; infixl 5 -<-; let (-<-) : Integer -> Integer -> Integer
let x ->- y = x - y; infixr 5 ->-; let (->-) : Integer -> Integer -> Integer

42 -<- 10 -<- 100
42 ->- 10 ->- 100
42 ->- 10 -<- 100

outputs

-68
-68
-68

instead of what is used to, which was:

-68
132

[error] at repl-decls.icry:6:4--6:7 and repl-decls.icry:6:11--6:14
    The fixities of
      • (->-) (precedence 5, right-associative)
      • (-<-) (precedence 5, left-associative)
    are not compatible.
    You may use explicit parentheses to disambiguate.

After a quick first pass through the renamer, I saw no reason why this PR would break the existing fixity rebalancing (none of the relevant bits seem to be changed) – but I'll be taking a deeper look soon.

[qsctr]

It seems like this only happens when the operators are defined at the REPL and not in a module.

[yav]

I think the problem might with the fixity of operators that are not defined at the top level. For example, this seems to be incorrect also:

f = 10 .-. 2 .-. 1
  where
  infixr 5 .-.
  x .-. y = x - y

The result is 7 (i.e. (10 - 2) - 1) but it should be 9 (10 - (2 - 1)).

[qsctr]

Ok, the issue seems to be here:

cryptol/src/Cryptol/ModuleSystem/Binds.hs

Lines 157 to 158 in 70d9600

	topDeclsDefs :: [TopDecl PName] -> ModBuilder (Mod ())
	topDeclsDefs = declsToMod Nothing

When we are processing the TopDecls we pass Nothing as the ModPath but we really want Just (TopModule m) with the m from here

cryptol/src/Cryptol/ModuleSystem/Renamer.hs

Lines 174 to 182 in 70d9600

	renameTopDecls ::
	ModName -> [TopDecl PName] -> RenameM (NamingEnv,[TopDecl Name])
	renameTopDecls m ds0 =
	do -- Step 1: add implicit importgs
	let ds = addImplicitNestedImports ds0
	-- Step 2: compute what's defined
	(defs,errs) <- liftSupply (modBuilder (topDeclsDefs ds))

just like the code in master

cryptol/src/Cryptol/ModuleSystem/Renamer.hs

Lines 86 to 87 in cfb1631

	let mpath = TopModule m
	env <- liftSupply (defsOf (map (InModule (Just mpath)) ds))

because the name binding code treats Nothing as meaning we are defining a local name.

cryptol/src/Cryptol/ModuleSystem/Binds.hs

Lines 302 to 307 in 70d9600

	instance BindsNames (InModule (Bind PName)) where
	namingEnv (InModule mb b) = BuildNamingEnv $
	do let Located { .. } = bName b
	n <- case mb of
	Just m -> newTop NSValue m thing (bFixity b) srcRange
	Nothing -> newLocal NSValue thing srcRange -- local fixitiies?

which is actually the other issue that @yav pointed out, if we actually do happen to be defining a local name, then we do not record custom fixities. But currently in master we have the same code for that and don't support having local fixities either.

cryptol/src/Cryptol/ModuleSystem/NamingEnv.hs

Line 378 in cfb1631

Nothing -> newLocal NSValue thing srcRange -- local fixitiies?

So I guess I will open up another issue for that and fix this specific issue for now to behave the same as on master.

[qsctr]

Fixed in 4b3eac5