Deriving Via by jkopanski · Pull Request #965 · B-Lang-org/bsc

jkopanski · 2026-06-06T17:11:01Z

My take on #964 during ZuriHac.

For now it has a commit with my dev env setup that I'll get rid of later.

Now that there will be another way to derive instaces I think multiline deriving declarations a'la Haskell should be permitted.

Since now there will be multiple deriving strategies there is a need to record that additional information in the syntax tree. Original deriving mechanism is called stock deriving (like in Haskell). There is no changes planned to the way this works. Follow up commits will add new deriving functions.

This actually generates some plausibly passing code. There are probably quite few situations where it'll fail. Like for example when the associated typeclass function uses the datatype as an argument to higher kinded type. Current solution won't substitute this variable. I don't think there is anything to be done here tbh, we won't be able to figure out how to wrap/unwrap it. Perhaps we should error with better message in that scenario?

krame505

Looks good! I'll let @quark17 approve merging.

krame505 · 2026-06-08T19:56:17Z


+data CDeriving
+        = CStock [CTypeclass]
+        -- TODO: expr?


Not sure what this TODO is about?

This was my first change in the code, I wasn't sure if the Id is the proper node to hold via target. Actually I'm still not sure if that is the correct choice here, but it worked out.

I guess in theory we could have a partially-applied type constructor instead of the name of a type here? Does Haskell support that?

I don't think so. Haskell expects a type here. Given that DerivingVia works by the way of coerce, it can be arbitrarily complex type, see example from ghc user guide:

newtype Kleisli m a b = Kleisli (a -> m b) deriving (Semigroup, Monoid) via (a -> App m b)

In here (bsc) I've assumed, that the type and constructor names must be the same. Perhaps this could only be constructor name, if I were not to generate type signature for the typeclass methods. But I've had the hunch the errors would be slightly better if I were to do it.

I would either remove the TODO comment or make it more clear.

krame505 · 2026-06-08T20:27:38Z

@@ -0,0 +1,12 @@
+package DerivingViaHK() where


Does HK mean higher-kinded? That kind of threw me at first because a is kind *. But I guess Maybe is kind * -> *? The issue here is that a isn't at the top-level, I guess.

yeah I wasn't sure how to name it. Indeed that's about for example Maybe, I'm not making any effort to substitute a for concrete data type we are deriving instance for, when a is used as argument to TAp. I don't think I could derive that kind of function anyway.

krame505 · 2026-06-08T20:29:06Z

Actually it looks like there are some testsuite failures, nothing major: https://github.com/B-Lang-org/bsc/actions/runs/27140760096/job/80181556806?pr=965

jkopanski · 2026-06-09T08:48:36Z

Actually it looks like there are some testsuite failures, nothing major: https://github.com/B-Lang-org/bsc/actions/runs/27140760096/job/80181556806?pr=965

If I saw it right failing tests were: EContextReduction1, bugID169 and ETooGeneral1. All of those used deriving () which I didn't think was valid and thus I replaced sepBy with sepBy1. Now it's reverted.

krame505 · 2026-06-10T18:35:37Z

Thanks, looks good to me.

@matx-chris, can we include this in our next internal BSC release?

matx-amy · 2026-06-26T21:50:05Z

+pDerivingStock = lp ..+ sepBy pTypeclass cm +.. rp         >>- CStock
+
+pDerivingVia :: CParser CDeriving
+pDerivingVia = pTypeclass +.+ l L_via ..+ pTyConId          >>> CVia


is there any chance this could be generalized to allow a parenthesized type? deriving Foo via (Bar Baz) is useful

I don't think so, at least not using the syntax proposed in the #964.

Right now the generated code uses the via identifier in two places, the class method type signature and data constructor. I guess the type signature isn't really needed here, I've added it because I've had a hunch it could lead to better error messages when something is not right.

What is necessary is data constructor that is used in the patterns in the generated code, and Bar Baz is a type not a constructor.

This all comes from the fact that it works quite different from the Haskell DerivingVia. Haskell variant just uses coerce. Here we generate as described in the issue that relies on the convention (single constructor datatype, with constructor name same as the type name), since there are no newtypes available (right?).

quark17

Thanks for your interest in developing BSC and contributing this!

I have not closely looked at doVia yet, but have comments on everything else. I think we need to decide whether this constructor approach is what we want to implement, or whether we should do something like use coerce (should be an easy simplification to doVia) or take an arbitrary expression for what to call (would require parsing an expression, also possibly not difficult?).

quark17 · 2026-07-05T09:47:06Z

 getFTCDn (Cdata { cd_internal_summands = summands,
                  cd_derivings = derivings }) =
-    S.unions (map f summands) `S.union` S.fromList (map typeclassId derivings)
+    S.unions (map f summands) `S.union` mconcat (map getFTDer derivings)


Here and a few lines below, I would probably use S.unions instead of mconcat

While doing so I've noticed that it might clearer (for me at least) to write it as: S.unions $ map f summands ++ map getFTDer derivings

But I've noticed that below there are many cases where there are unions of unions taken instead. So I've just did the simple change.

quark17 · 2026-07-05T10:03:56Z

-> pDerivationsIf True = option [] $
->     do pKeyword SV_KW_deriving
->        pInParens (pCommaSep1 (pTypeclass <?> "typeclass name"))
+> pDerivationsIf True = many pDeriving


You've changed BH syntax to allow multiple deriving clauses. While you haven't enabled deriving-via for BSV, here you have enabled multiple deriving clauses, which wasn't the syntax before. If we're not sure about the BSV syntax yet, then maybe we shouldn't change this.

quark17 · 2026-07-05T10:06:50Z


+data CDeriving
+        = CStock [CTypeclass]
+        -- TODO: expr?


I would either remove the TODO comment or make it more clear.

quark17 · 2026-07-05T10:12:22Z

-ppDer d [] = empty
-ppDer d is = text "deriving (" <> sepList (map (pvPrint d 0) is) (text ",") <> text ")"
+ppDer :: PDetail -> [CDeriving] -> Doc
+ppDer d drvs = vcatList (map (pPrint d 0) drvs) empty


I'm not sure we can use pPrint this way. The identifier in "deriving" can be qualified, and BSV and BH print qualifiers differently. I think we have to duplicate what pPrint of CDeriving is doing, except use pvPrint inside for IDs.

quark17 · 2026-07-05T10:20:07Z

+    in Right $ pure $ Cinstance (CQType ctx appliedClass) (map mkFun fs)
+
+doVia _ _ _ (di, tgt) =
+  Left (getPosition di, ECannotDerive (pfpString di ++ " via " ++ pfpString tgt))


Do we want to add a test case for this error situation?

quark17 · 2026-07-05T10:32:03Z

+# tests for deriving via
+
+compile_pass DerivingVia.bs
+compile_fail_error DerivingViaHK.bs T0030


This needs a comment explaining what's being tested. Is this a bug in BSC, that it gives an error or at least gives an unhelpful error? Or is this the message (and position) that you expect? (If it's a bug, we'll want to file it and record the number here.)

quark17 · 2026-07-05T10:34:26Z

+package DerivingVia() where
+
+import SplitPorts
+


I would also add uses of the derived instances, to prove that they exist and were defined properly. I don't know if that's easy to do with ShallowSplit or if we should use a simpler typeclass for this example.

quark17 · 2026-07-05T10:44:46Z

+
+import SplitPorts
+
+data Foo = Foo (Int 8)


You should also test on a data type with multiple disjuncts.

done in @nanavati commit

quark17 · 2026-07-05T10:52:11Z

 >      (SV_KW_actionvalue,         "actionvalue",          Bluespec38),
 >      (SV_KW_endactionvalue,      "endactionvalue",       Bluespec38),
 >      (SV_KW_deriving,            "deriving",             Bluespec38),
+>      (SV_KW_deriving,            "deriving",             Bluespec38),


This duplicate line should be removed.

quark17 · 2026-07-05T10:52:48Z

 >     | SV_KW_actionvalue
 >     | SV_KW_endactionvalue
 >     | SV_KW_deriving
+>     | SV_KW_via


I'd probably comment this out, if it's not being used yet.

nanavati · 2026-07-12T10:16:52Z

Adversarial review of a tree carrying this PR found one wrong-results-class gap, confirmed by reproduction: doVia generates the unwrap as a single-arm case (Ccase ... [CCaseArm (CPCon tgt [CPVar y]) ...]) without validating the target's shape. With a multi-constructor target:

data W a = W a | X a
instance (MyC a) => MyC (W a) where def = X def
data T = T (Int 8)
  deriving MyC via W

this compiles with zero warnings and the derived method elaborates to an unspecified value in the generated Verilog (assign r$D_IN = 8'b10101010 /* unspecified value */ in our repro) — the partial case falls through silently. GHC's DerivingVia enforces representational equality; the analogue here is requiring the via target to be a single-constructor, single-field wrapper whose constructor is named like the type (which the generated CCon tgt/CPCon tgt already assume).

Validated fix (guard clause before the main doVia clause):

viaTargetShapeOK :: [(Id, CDefn)] -> Id -> Bool
viaTargetShapeOK xs tgt =
    case lookup (unQualId tgt) xs of
      Just (Cdata { cd_original_summands = [summand] }) ->
          length (cos_arg_types summand) == 1 &&
          any (qualEq tgt) (cos_names summand)
      _ -> False

rejecting with a positioned ECannotDerive ("the via target must be a data type whose single constructor is named ... and takes a single argument"). With that in place the existing ShallowSplit uses still derive fine, and the multi-constructor case gets a clean compile-time error. Happy to push the change (plus a regression test asserting the error) to this branch if you'd like.

…wrap doVia wraps arguments with (CCon tgt [x]) and unwraps results with a single-arm case on (CPCon tgt [y]), silently assuming the target is a single-constructor, single-argument wrapper named like the type. A multi-constructor target compiled without any diagnostic, and the partial unwrap case elaborated to an unspecified value in the generated hardware whenever the delegated instance produced the other constructor. Check the shape up front and reject with a positioned error. When the target is written qualified, the candidate definition must match the written qualifier, not just be the first unqualified hit: a local type may shadow the imported one the user named, and judging the wrong definition's shape would let a bad target through (or reject a good one). Unqualified targets keep normal locals-first scoping, which is also what the generated CCon reference resolves to. Add a regression test asserting the rejection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019cqQdWKp7jr73GHQYnvhiq

nanavati · 2026-07-12T23:34:13Z

Follow-up to the above: the fix is now available as a ready-made commit on top of this branch — nanavati/bsc@bd91fcf1 (branch 965-via-target-shape), including the regression test and expected output.

One refinement over the snippet posted earlier: when the via target is written qualified (via Q.W), the shape check matches the candidate definition's own qualifier rather than taking the first unqualified hit — otherwise a local type shadowing the imported one the user named would be judged instead, which could let a multi-constructor target through. Unqualified targets keep normal locals-first scoping, which is also what the generated CCon reference resolves to.

The identical change is validated in our integration tree (multi-constructor target → positioned error; the existing ShallowSplit derivations unaffected). @jkopanski feel free to cherry-pick it into this PR, or say the word and I'll push it here directly.

jkopanski · 2026-07-13T13:41:33Z

@jkopanski feel free to cherry-pick it into this PR

I did, thanks

jkopanski · 2026-07-13T19:11:41Z

@@ -0,0 +1,3 @@
+Error: "DerivingViaMultiCon.bs", line 17, column 11: (T0000)


@nanavati I'm getting a following diff here:

--- DerivingViaMultiCon.bs.bsc-out.expected 2026-07-13 14:34:56.121794265 +0200 +++ DerivingViaMultiCon.bs.bsc-out 2026-07-13 21:02:35.052396080 +0200 @@ -1,3 +1,5 @@ +checking package dependencies +compiling DerivingViaMultiCon.bs Error: "DerivingViaMultiCon.bs", line 17, column 11: (T0000) Illegal deriving: MyC via W: the via target must be a data type whose single constructor is named W and takes a single argument

looking at other examples I think those, checking (...) compiling (...) lines should be there, right? I'm doubting myself given that you've said that you have pulled those changes out of the internal repo, which I presume is passing tests with this exact expected?

jkopanski force-pushed the deriving-via branch from 62074f8 to 1421449 Compare June 7, 2026 17:04

jkopanski added 4 commits June 8, 2026 15:19

DerivingVia tests

792ef23

Now that there will be another way to derive instaces I think multiline deriving declarations a'la Haskell should be permitted.

Document failure with higher kinded type

8a7b2f8

jkopanski force-pushed the deriving-via branch 2 times, most recently from be71076 to 8a7b2f8 Compare June 8, 2026 13:24

jkopanski marked this pull request as ready for review June 8, 2026 13:27

krame505 reviewed Jun 8, 2026

View reviewed changes

derivng () is a proper construct

532bf67

matx-amy reviewed Jun 26, 2026

View reviewed changes

quark17 reviewed Jul 5, 2026

View reviewed changes

jkopanski added 6 commits July 13, 2026 15:27

Remove duplicated SV keywords

53f10bc

CVPrint specific deriving clause printing

0329e0c

mconcat -> unions

f1a693d

missing newline

fcacab5

Inacurate comment over doVia

51d52dc

Revert to parsing only single derivation clause in bsv

0ff18e7

jkopanski commented Jul 13, 2026

View reviewed changes

jkopanski added 4 commits July 13, 2026 21:12

Missing lines in expected output

c602360

Test function obtained via DerivingVia

8f0b1df

Test doVia failure branch

1352a20

Comment on expected failure

90fbf55

jkopanski added 2 commits July 22, 2026 14:26

Replace todo with more verbose comment

5bcb05e

Move comment to appropriate function

552c348

		@@ -0,0 +1,3 @@
		Error: "DerivingViaMultiCon.bs", line 17, column 11: (T0000)

Uh oh!

Conversation

jkopanski commented Jun 6, 2026

Uh oh!

krame505 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krame505 commented Jun 8, 2026

Uh oh!

jkopanski commented Jun 9, 2026

Uh oh!

krame505 commented Jun 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quark17 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!

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!

nanavati commented Jul 12, 2026

Uh oh!

nanavati commented Jul 12, 2026

Uh oh!

jkopanski commented Jul 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants