refactor: Row parsing by croyzor · Pull Request #116 · Quantinuum/brat

croyzor · 2026-04-28T08:35:37Z

Redo the way we represent rows in syntax.

We now represent rows for function types and annotations in Term as TypeRowElem ty (= Named String ty | Anon ty) rather than adding dummy names when desugaring
Refactor the Flat syntax so that it doesn't mix in any Raw terms to represent types in rows

Now, Brat.Syntax.Raw only really does some disambiguation between variables and constructors, and we should soon be able to get rid of it altogether!

acl-cqc

Yes! This looks fab, thank you 👍 😄. I don't think any comments are big except possibly in Elaborator.hs. Do you object to CType' or WC being functors? I could do that myself if you like the idea.

For awhile I thought this was keeping the named/anonymous distinction into the graph, something I tried once but couldn't get through, but now I realize this is a different change, albeit probably a big step towards the former. Love it!

Maybe edit the description where you say in Core to "in Term" or something that is more clearly still talking about parsing not the brat graph. (I guess writing Brat.Syntax.Core would also make that clear)

acl-cqc · 2026-05-01T11:14:11Z

 | PApp
 deriving (Bounded, Enum, Eq, Ord, Show)
+
+data TypeAliasF tm = TypeAlias FC QualName [(PortName,TypeKind)] tm deriving Show


What's the F? that it has an FC in it? I'd be tempted to call it TypeAliasFC if you think it's important to say that, again not perfect I admit

The F is for "Functor", to say that this is the generic definition, but I guess it'd be more consistent to call it TypeAlias'

Ah ok, alright then. Ah yes I see it's only moved anyway. Might you deriving Functor then?

acl-cqc · 2026-05-01T12:03:11Z

-unelab _ _ (tm ::: outs) = FAnnotation (unelab Chky Nouny <$> tm) (toRawRo outs)
+unelab _ _ (tm ::: outs) = FAnnotation (unelab Chky Nouny <$> tm) (f outs)
+ where
+  f :: [TypeRowElem (KindOr (Term Chk Noun))] -> [TypeRowElem (WC (KindOr Flat))]


Nit: this might be slightly easier to read as f :: (KindOr (Term Chk Noun)) -> WC (KindOr Flat) and put the outer two fmaps at the callsite

The outermost fmap is just over the list, so that could even just be map, too

Ah, actually this is just the same as unelabRo below, so just use that

acl-cqc · 2026-05-01T12:17:24Z


  Internal error: Expected empty unders after abstract, got:
-  (b1 :: Int)
+  (_1 :: Int)


Yes!!! That's much more sensible!!!

(Just to check - is it even possible to call something _1 and get this message yourself? We should choose the synthetic _ prefix so that it isn't...e.g. consider '0)

acl-cqc · 2026-05-01T12:18:55Z

                     ^^^^^^^^^

-  Port a1 is ambiguous in (a1 :: Nat), (a1 :: Nat)
+  Port not found: a1 in (_0 :: Nat, _0 :: Nat)


Yay! :).

Can we try a similar test case where you explicitly pull _0 and see that that doesn't work (the element is anonymous, not named; the synthetic _ is only for printing/displaying, not pulling)?

Good catch! The idea is that _0 isn't a valid user-supplied name for a port, but because of this it's not in our grammar for port pulling either, I'll add it back in.

I think the grammar for ports is [a-zA-Z][0-9a-zA-Z]*, so we could make the automatically inferred ports just 0,1,2...

Done in 977b810

acl-cqc · 2026-05-01T12:42:36Z

                ^^^^^^^^^^^

-  Type error: Expected a (kernel) function or vector of functions, got { (a1 :: Bool) -> (a1 :: { (a1 :: Qubit) -o (a1 :: Qubit) }) }
+  Type error: Expected a (kernel) function or vector of functions, got { (_0 :: Bool) -> (_1 :: { (_0 :: Qubit) -o (_0 :: Qubit) }) }


Hmmmm ok. So it would actually be a different change (but still good as a later PR I guess) to carry this on through into the later stages of the compiler, changing NamedPort to distinguish between named/unnamed (which could be done in a few different ways). I might have another go at that....

acl-cqc · 2026-05-01T12:44:32Z

 (addN2.brat_globals_decl_13_addN,BratNode Id [("thunk",{ (inp :: Int) -> (out :: Int) })] [("thunk",{ (inp :: Int) -> (out :: Int) })])
 (addN2.brat_globals_prim_2_N,BratNode (Prim ("","N")) [] [("value",Int)])
-(addN2.brat_globals_prim_8_add,BratNode (Prim ("","add")) [] [("a1",{ (a :: Int), (b :: Int) -> (c :: Int) })])
+(addN2.brat_globals_prim_8_add,BratNode (Prim ("","add")) [] [("_0",{ (a :: Int), (b :: Int) -> (c :: Int) })])


So at some point we should try to make these anonymous too, I'm not quite sure even where we'd have to change, might be NamedPort (https://github.com/Quantinuum/brat/pull/116/changes#r3173191053). Not for this PR, fine...

acl-cqc

Yes! I would say drop the support for pulling by compiler-generated names and update the test but basically good to go. Great stuff, thanks 🏆

(I'm also a bit suspicious about that typeclass. I might see whether I can remove that before you get to merge this....)

acl-cqc · 2026-05-15T15:45:28Z

 | PApp
 deriving (Bounded, Enum, Eq, Ord, Show)
+
+data TypeAliasF tm = TypeAlias FC QualName [(PortName,TypeKind)] tm deriving Show


Ah ok, alright then. Ah yes I see it's only moved anyway. Might you deriving Functor then?

acl-cqc · 2026-05-15T15:48:15Z

       -> Checking (-- [(Int, Test)] -- too much too hugr too soon?
                    [(Src, PrimTest (BinderType m))]
-                   ,[(String, (Src, BinderType m))] -- Remember the names given by programmers
+                   ,Solution m -- Remember the names given by programmers


You might wanna put this comment by the definition of solution, or at least, put something there explaining what Solution is. Whether you then still need this comment here I don't mind.

acl-cqc · 2026-05-15T15:48:27Z

@@ -59,7 +61,7 @@ solve :: forall m. Modey m
       -> Problem
       -> Checking (-- [(Int, Test)] -- too much too hugr too soon?


Drop this commented-out bit while you're here?

acl-cqc · 2026-05-15T15:49:55Z

+showSig :: (Show ty) => [TypeRowElem ty] -> String
+showSig xs = parens $ intercalate ", " (showElem <$> xs)
+ where
+  parens x = '(':x ++ ")"


make parens a toplevel? We have an export list here so it'd be private, which is fine

acl-cqc · 2026-05-15T15:50:09Z

-  = intercalate ", " [ '(':p ++ " :: " ++ show ty ++ ")"
-                     | (p, ty) <- x:xs]
+showSig :: (Show ty) => [TypeRowElem ty] -> String
+showSig xs = parens $ intercalate ", " (showElem <$> xs)


acl-cqc · 2026-05-15T16:58:39Z

    { fnName = fnName d
    , fnLoc = fnLoc d
-    , fnSig = fnSig d
+    , fnSig = fmap unWC <$> sig -- sus


Should the comment go on RawFuncDecl that it's fnSig does not include a WC and should?

acl-cqc · 2026-05-15T17:01:18Z

  list2Cons _ = customFailure (Custom "Internal error list2Cons")

-  portPull = port <* match PortColon
+  portPull = (try port <|> (fmap show <$> number)) <* match PortColon


I actually think you should not be able to portPull something using a compiler-generated name. Only using a name you specifically declared yourself. (I think you may have done this in response to an earlier comment by me that I should have phrased differently).

Reasoning: this makes the compiler's name-generation process part of the source code standard.

acl-cqc · 2026-05-15T17:05:10Z

+ nDecl = match TypeColon >> flatIOWithSpanFC
+
+ vDecl :: Parser (WC [FlatIO])
+ vDecl = functionSignature <&> (\(WC fc ty) -> WC fc [Named "thunk" (WC fc (Right ty))])


Ok so this adds an extra WC compared to what we were doing before. Might this justify stripping out a level of WC when making a Decl fnSig (which you called sus earlier )?

acl-cqc · 2026-05-15T17:10:03Z

 unelab _ _ (Con c args) = FCon c (unelab Chky Nouny <$> args)
-unelab _ _ (C (ss :-> ts)) = FFn (toRawRo ss :-> toRawRo ts)
-unelab _ _ (K cty) = FKernel $ fmap (\(p, ty) -> Named p (toRaw ty)) cty
+unelab _ _ (C (ss :-> ts)) = FFn ((unelabRo ss)


Suggested change

unelab _ _ (C (ss :-> ts)) = FFn ((unelabRo ss)

unelab _ _ (C cty) = FFn (fmap unelabRo cty)

CTy may not be a Functor but this is non-Hasochistic CType' which is.

Co-authored-by: Alan Lawrence <alan.lawrence@quantinuum.com>

…ming

…-traverse's

acl-cqc · 2026-05-15T17:43:05Z

(I'm also a bit suspicious about that typeclass. I might see whether I can remove that before you get to merge this....)

Done, I admit there was a little repetition but mostly type signatures and I was able to use FlatIO instead of something much longer for one case so I think that was good. Also generalized elabSig and it's counterpart elabIO over Traversable allowing to use in elaborate' for FFn and FKernel also.

Can you please find a better name for elaborateKindOrFlat though - is it somethitng like elaborateBratType ? :)

Redo row parsing

8c3b8fd

croyzor requested a review from acl-cqc April 28, 2026 08:35

acl-cqc reviewed May 1, 2026

View reviewed changes

croyzor added 6 commits May 1, 2026 16:16

Review comments

fa4a0f8

Specialise flatIO

fed4349

refactor: Introduce Solution type alias

50b2f41

Elaborable -> Elaboratable

8bc5ce4

traverse elabSigElem -> elabSig

7ddc09c

Automatic port names are just numbers, and now they can be pulled

977b810

croyzor requested a review from acl-cqc May 15, 2026 12:39

croyzor and others added 4 commits May 15, 2026 14:01

Oops! Update that golden file

533ea30

Don't allow generated port names to be pulled

4a3b36b

Update port pulling test

9059363

Merge branch 'main' into cr/refactor/row-parsing

a7e8ba3

acl-cqc approved these changes May 15, 2026

View reviewed changes

croyzor and others added 3 commits May 15, 2026 18:14

Another instance of Solution Brat

868db16

Co-authored-by: Alan Lawrence <alan.lawrence@quantinuum.com>

Drop Elaboratable. elabIO is fine, but elaborateKindOrFlat needs rena…

49528c5

…ming

generalize elabSig + elabIO over Traversable, remove the other double…

11229de

…-traverse's

croyzor and others added 2 commits May 29, 2026 11:07

elaborateKindOrFlat -> elaborateBratType

3d751e6

Merge branch 'main' into cr/refactor/row-parsing

921e759

croyzor merged commit d16cefc into main May 29, 2026
1 check passed

croyzor deleted the cr/refactor/row-parsing branch May 29, 2026 11:07

		@@ -59,7 +61,7 @@ solve :: forall m. Modey m
		-> Problem
		-> Checking (-- [(Int, Test)] -- too much too hugr too soon?

	unelab _ _ (C (ss :-> ts)) = FFn ((unelabRo ss)
	unelab _ _ (C cty) = FFn (fmap unelabRo cty)

Conversation

croyzor commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acl-cqc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

croyzor May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acl-cqc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acl-cqc commented May 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

croyzor commented Apr 28, 2026 •

edited

Loading

croyzor May 1, 2026 •

edited

Loading