Skip to content

Conversation

Soupstraw
Copy link
Collaborator

@Soupstraw Soupstraw commented Aug 21, 2025

This PR makes it possible to specify custom CBOR generators for specific rules. This can be useful when it is not possible to specify all of the constraints using CDDL's control operators, but you still want to have the generator generate valid data for your test cases.

@Soupstraw Soupstraw force-pushed the jj/generator-override branch 3 times, most recently from 130f5d3 to 2618ae7 Compare August 22, 2025 09:42
@Soupstraw Soupstraw force-pushed the jj/generator-override branch from 2618ae7 to 503019d Compare August 22, 2025 11:44
@Soupstraw Soupstraw changed the title WIP Add generator overriding Aug 22, 2025
@Soupstraw Soupstraw force-pushed the jj/generator-override branch from a0b2f87 to 2a82948 Compare August 22, 2025 12:49
@Soupstraw Soupstraw force-pushed the jj/generator-override branch from d146220 to 60b5593 Compare August 26, 2025 12:22
@Soupstraw Soupstraw requested review from lehins, nc6 and jasagredo August 26, 2025 12:23
@Soupstraw Soupstraw marked this pull request as ready for review August 26, 2025 12:23
Copy link
Member

@lehins lehins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is some of my feedback. I'll think more about what can we do about the problem that we now store a function in a type that has Show and Eq instances, whcih should never be the case

- name: Install fourmolu
run: |
FOURMOLU_VERSION="0.18.0.0"
FOURMOLU_VERSION="0.15.0.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for the downgrade?

This caused a whole bunch of unnecessary diff. Could you please revert it and undo all of the unrelated changes?

Comment on lines +73 to +75
= SingleTerm Term
| PairTerm Term Term
| GroupTerm [WrappedTerm]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just name the actual constructors? I don't see a point in pattern synonyms

Suggested change
= SingleTerm Term
| PairTerm Term Term
| GroupTerm [WrappedTerm]
= S Term
-- ^ Single Term
| P Term Term
-- ^ Pair Term
| G [WrappedTerm]
-- ^ Group Term

| MRuleRef Name
deriving (Functor, Show)

deriving instance Show (CTree MonoRef)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inability to derive Show, Eq and Generic is very error prone and it would be better to avoid if possible. Looking at Hashable instance makes me cry.
Unfortunately, I do not have a suggestion how to avoid it. 😞

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be possible to replace CBORGenerator with a type parameter in the definition of CTree and then use some dummy type for implementing these instances.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicer than this would be to wrap the generator function into a synthetic type that has overridden Eq, Show, Ord instances

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, make CBORGenerator a (bad) instance of these things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, make CBORGenerator a (bad) instance of these things

I did think about that, but that just moves a problem to a different place.

Problem with this approach in general is that you cannot provide a lawful instance for Eq when type contains a function and thus you could easily break referential transparency, which is one of the things we really like Haskell for 😁
It could be argued that it is not a big deal in this case, and it is true, but it does make me cringe looking at it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make sense to add some index type to the AST anyways, because that would let us use trees that grow to have a different AST representation for each purpose. For example we don't need generators at all if we only use that AST to pretty-print, so the pretty printing AST could have a unit type in place of the generator. Similarly we can get rid of the comments field if we're using the AST for verification, so the verification logic doesn't need to deal with comments at all.

@Soupstraw Soupstraw force-pushed the jj/generator-override branch from 78bda12 to 53e5540 Compare August 29, 2025 10:48
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

Successfully merging this pull request may close these issues.

3 participants