Tests for Generics

[krame505]

This adds test cases for the generics mechanism added in B-Lang-org/bsc#284, and makes a couple of other minor changes required for the testsuite to pass. make check passes for me, pending #17 being merged.

The generics test cases include a use of generics to implement a custom version of Bits, tests for CShow, and both positive and negative tests that the generic representation types are as expected and that the from and to methods of the derived Generic instances work properly.

One change required to make the existing tests pass was in bsc.bluetcl/targeted/type, as the generated polymorphic field wrappers now show up in the bluetcl output. I'm not really sure how to avoid this if the change in behavior is undesirable, but since polymorphic fields rarely show up in actual code I doubt that this is an issue.

Another such change is the one to bsc.bugs/b381, as discussed previously in an email.

Additionally there are a few miscellaneous tests added, and some minor unrelated cleanup.

[quark17]

This all looks good now! This is ready to merge when the BSC PR is merged.

I am temped to squash this into a single commit. Unless you think there are parts of the history worth preserving as individual commits?

[quark17]

Did you ever add a test like this (perhaps to bsc.evaluator/prims/impcondof/):

typedef struct {
  function b f(a x) field;
  // a field;
} T;

(* synthesize *)
module mkM ();
  Reg #(Bool) cond <- mkReg(False);
  T x = T { field: when (cond, ?) };
  Bool b = impCondOf (x);
  Reg #(Bool) rg <- mkRegU;
  rule r;
    rg <= b;
  endrule
endmodule

This demonstrates polymorphism of type a -> b and just a (depending on which you uncomment). You added support for these with SPolyWrap, but I wasn't sure if tests had been added.

[krame505]

I am temped to squash this into a single commit. Unless you think there are parts of the history worth preserving as individual commits?

I'm not aware of any, sounds reasonable to me.

Did you ever add a test like this (perhaps to bsc.evaluator/prims/impcondof/):

Ah, good catch. I think I tested this in debugging by tweaking my Classic test cases, but nothing actually got committed. I added the function version as a BSV test case. However the a field; version gives an error:

Error: Unknown position: (T0033)
  There is not enough explicit type information to deduce the types of all
  expressions. Consider adding more type declarations to help resolve this
  ambiguity.
    An ambiguous type was introduced at Unknown position
    This type resulted from:
      The proviso Prelude.PrimDeepSeqCond a__ introduced in or at the
      following locations:
	Unknown position

This is seemingly arising from the PrimDeepSeqCond instance on the generated struct:

struct T_field = {
    val :: a
};

instance Prelude.PrimDeepSeqCond T_field where {
    primDeepSeqCond _x =  Prelude.primDeepSeqCond _x.val;
};

I'm not really sure what the right thing to do is in this case.

[quark17]

This is seemingly arising from the PrimDeepSeqCond instance on the generated struct

Yes, I believe you are incorrectly constructing the instance -- and am sorry that I wasn't reviewing your bsc PR close enough to catch that! This line in mkGenericRepWrap:

(CApply (CVar idPrimDeepSeqCond) [CSelect (CVar id_x) id_val])] []]]]

is applying the typeclass member and not the evaluator primitive (as the comment on the function says is the intention). This is what is introducing the proviso. I think you should be using idPrimSeqCond instead. And I confirm that when I make this change, the example compiles.

(Also, at the place at the top where the primitives for the classes are imported, the comment says "internal class members", but of course two -- soon to be three -- of them are the evaluator primitives, not the class members -- the names do not make that clear and don't even follow a consistent form between the three, unfortunately. I think I already have an open comment on the bsc PR, suggesting that comments and groupings on the Id importing can be improved.)

[krame505]

@nanavati pointed out that the PrimDeepSeqCond instance that was being derived on the poly wrapper structs wasn't actually doing anything - unlike the primitives for uninitialized and undefined values, primSeqCond doesn't "close the loop" in the evaluator and call the instance. And primSeqCond can already handle function values properly, so simply using primSeqCond in place of id in the generic instance for ConcPoly was sufficient to get the desired behavior, and I believe that there is no need to even be generating these instances.

The only way that not having these PrimDeepSeqCond instances for poly wrapper structs could possibly make a difference is if someone in implementing another class with generics for some reason decided to call primDeepSeqCond on the value inside ConcPoly. But that is a strange thing to do, and @nanavati and I think that not supporting this is acceptable.

[quark17]

unlike the primitives for uninitialized and undefined values, primSeqCond doesn't "close the loop" in the evaluator

Indeed. However, what we want to call primSeqCond on is the value inside the wrapper, not on the wrapper itself. So we still need to call the instance, to do the word of applying the appropriate .val field selector (which we unfortunately can't do in the generic typeclass).

This also explains why your tests are currently seeing the wrong behavior (even though I know I had prototyped this in the generics branch before). See my comment just now that I attached to the previous commit, at the same time that you were force-pushing a new commit :)

The Prelude instance needs to look like this:

instance (PrimDeepSeqCond a) => PrimDeepSeqCond' (ConcPoly a) where
  primDeepSeqCond' (ConcPoly x) = primDeepSeqCond x

Where you have primSeqCond, this needs primDeepSeqCond (and its proviso). Once I add that, and correct the Deriving-generated instance to use primSeqCond (on the field), things move forward ... although now I'm seeing the assertion in IExpand fail. I'll keep investigating.

[quark17]

although now I'm seeing the assertion in IExpand fail. I'll keep investigating

Ah. The issue is that my scheme calls primSeqCond on the field, so when IExpand checks that primSeqCond is only being called on Abstract or SPolyWrap types, that will fail because the field is not of SPolyWrap type.

We could fix the check. Or... we could keep your scheme (where there is no Deriving-generated DeepSeqCond instance and the ConcPoly instance directly calls the primitive on the polywrap type) and just change IExpand to do something different when it see a polywrap type: specifically, it should add the field selector to the given expression, and then proceed applying to that.

Do you and @nanavati have an opinion on which of these is better?

[quark17]

FYI, I was able to get your code to work by adding this code to IExpand's handling of PrimSeqCond (replacing evalUH e1):

   flags <- getFlags
   symt <- getSymTab
   let e1' = case t1 of
               (ITCon wrapname _ ti@(TIstruct (SPolyWrap _ _ _) _)) ->
                 case (findFieldInfo symt wrapname idPolyWrapField) of
                   Nothing -> internalError ("PrimSeqCond wrapper field not found: " ++ ppReadable wrapname)
                   Just (FieldInfo { fi_assump = _ :>: fieldscheme@(Forall ks _) }) ->
                     let fieldty = iConvSc flags symt fieldscheme
                         selty = itFun t1 fieldty
                         selfn = ICon idPolyWrapField (ICSel selty 0 1)
                         tyargs = map (const itPrimUnit) ks
                     in  IAps (IAps selfn [] [e1]) tyargs []
               _ -> e1
   (_, P p e) <- evalUH e1'

I had a hard time constructing the .val selection, mostly because I had to construct the type, and then I had to replace the polymorphic variables with something, so I picked PrimUnit (and I'm not sure if that's safe to always do?). It might be easier to construct the CSyntax expression (with generic variables) and then do something similar to what IExpand.evalCExpr is doing -- it typechecks the CExpr and then converts it to ISyntax, but then it goes ahead and evaluates it, which we don't want to do. Maybe the CSyntax-to-ISyntax stuff could be pulled out, into its own function, so that we could call it for our purposes.

However, all of this complication can be avoided by using a PrimDeepSeqCond instance, created in Deriving, to do the field selection. But then IExpand's assertion for PrimSeqCond wouldn't be able to identify that it's being called on the field of a PolyWrap struct -- I don't think. In the example with a the variable is defaulted to PrimUnit and so the type that IExpand sees is just PrimUnit. I think we'd have to settle for removing the assertion. Maybe that's OK?

[krame505]

@nanavati pointed out that always surfacing the implicit condition on higher-rank members doesn't seem to match the current behavior. We added https://github.com/B-Lang-org/bsc-testsuite/blob/master/bsc.typechecker/higherrank/DeepSeqCond.bs to document the compiler's current behavior, which is to not surface the condition for f4. To be clear, is changing this behavior to always surface the condition (breaking this test) what we want to do?

If so, then just deriving a PrimDeepSeqCond instance and ditching the assert is probably the most straightforward. But if keeping this the assert is indeed important, then adding your special-case behavior in the evaluator could also be reasonable - I don't know.

[quark17]

To be clear, is changing this behavior to always surface the condition (breaking this test) what we want to do?

What I've been talking about would not change this behavior.

But thank you for bringing it up. It seems there are three situations:

1. Polymorphism with provisos (the f4 example)
2. Polymorphic types with arrows (a -> b)
3. Polymorphic types without arrows (which could possibly be broken into several situations, but includes at least a; might be worth testing Maybe a and the like, too, but I suspect it's handled the same)

Existing BSC has the following behaviors on DeepSeqCond:

1. No lifting
2. Lifting
3. Internal error

Your changes have the following behaviors:

1. No lifting
2. No lifting
3. No lifting

My two proposals (in the earlier comment) were about getting your Generic branch to have this behavior (matching existing BSC on 1 and 2 and doing better on 3):

1. No lifting
2. Lifting
3. Lifting

Lifting is possible in situations 2 and 3 because no polymorphism exists when it gets to ISyntax: BSC has defaulted the generic variables to PrimUnit. In situation 1, the generated ISyntax looks like this:

DeepSeqCond.f4 :: DeepSeqCond.Foo
DeepSeqCond.f4 =
  DeepSeqCond.Foo
    (let _tcdict1010 :: Prelude.Literal (Prelude.UInt 8)
	 _tcdict1010 = Prelude.Prelude.Literal~Prelude.UInt~n= ?8
	 -- IdProp: _tcdict1010[IdP_bad_name,IdPDict]
     in  .Prelude.fromInteger ?(Prelude.UInt 8) _tcdict1010 42)
    (/\ (_tctyvar1020 :: *) .
     \ (_tcdict1021 :: Prelude.Literal _tctyvar1020) .
     Prelude._when_=
       ?_tctyvar1020
       (Prelude.False Prelude.PrimUnit)
       (.Prelude.fromInteger ?_tctyvar1020 _tcdict1021 114))

You can see that the call to when has been wrapped with lambda (a type lambda for tyvar1020 and a value lambda for the Literal dictionary). As long as these are surrounding lambdas are there, the IExpand handling of PrimSeqCond (which evaluates to WHNF) will not lift conditions, even with my proposals. (If BSC could push the lambdas inside the when, then my proposals would allow lifting to happen.)

It seems like we have two options here: (A) be backwards compatible with the existing behavior ("no lift/lift/lift", per my proposals) or (B) we decide what the right thing to do is and we implement that (for example "no lift/no lift/no lift", per your original PR behavior).

I'm OK with either A or B. It's a bit easier to go with A, knowing that we're not breaking behavior for anyone; but also I doubt that anyone is relying on the current behavior -- and arguably they shouldn't (and the defaulting to PrimUnit is maybe something that shouldn't be happening in this code, anyway, and therefore not not relied on for behavior) -- so maybe B is worth doing.

As long as we have test cases for all the situations, to document the chosen behavior and catch if anything changes, then I'm fine with either. (I did wonder whether a second version of the f4 example would be needed, where the type had an arrow, like (Arith a) => a -> a, but I suspect that it's the same behavior. Similarly, I suspect that a variant of the test for just a, changed to be Maybe a, would have the same behavior. Though maybe it wouldn't hurt to add these variants.)

[krame505]

I already have option (B) working via deriving a DeepSeqCond instance for the generated poly wrapper structs, so I'll go with that for now. I will go and update the tests.

[krame505]

Wait, with my changes I am actually seeing the condition being lifted for f4. I am getting the same ISyntax as you posted above:

DeepSeqCond.f4 :: DeepSeqCond.Foo
DeepSeqCond.f4 =
  DeepSeqCond.Foo
    (let _tcdict1010 :: Prelude.Literal (Prelude.UInt 8)
	 _tcdict1010 = Prelude.Prelude.Literal~Prelude.UInt~n= �8
	 -- IdProp: _tcdict1010[IdP_bad_name,IdPDict]
     in  .Prelude.fromInteger �(Prelude.UInt 8) _tcdict1010 42)
    (/\ (_tctyvar1020 :: *) .
     \ (_tcdict1021 :: Prelude.Literal _tctyvar1020) .
     Prelude._when_=
       �_tctyvar1020
       (Prelude.False Prelude.PrimUnit)
       (.Prelude.fromInteger �_tctyvar1020 _tcdict1021 114))

I have no idea why this is working if IExpand can't handle conditions from inside lambdas.

But I feel that we have already spent way to much time on this corner-case, so I am tempted to just update the test to reflect this behavior and move on.