Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

copilot-core: Facilitate use of structs #564

Open
ivanperez-keera opened this issue Nov 11, 2024 · 7 comments · May be fixed by #566
Open

copilot-core: Facilitate use of structs #564

ivanperez-keera opened this issue Nov 11, 2024 · 7 comments · May be fixed by #566
Assignees
Labels
CR:Status:Implementation Admin only: Change request that is currently being implemented CR:Type:Feature Admin only: Change request pertaining to new features requested
Milestone

Comments

@ivanperez-keera
Copy link
Member

Description

Currently, using structs in Copilot requires defining instances of several type classes. Defining those instances is unnecessarily cumbersome, and it exposes the user to a substantial amount of Haskell even when they want to stay at the level of the Copilot language.

It would be helpful to facilitate generating instances of Typed and Struct for struct types / record types, by providing functions that implement the default behavior and only requiring users to write their own functions if they wanted to deviate from those defaults.

Type

  • Feature: Improve usability of the language.

Additional context

None.

Requester

  • Ivan Perez.

Method to check presence of bug

Not applicable (not a bug).

Expected result

Introduce functions that implement the default behavior for the methods in the Typed and Struct type classes, so that users can rely on them when implementing their own instances.

Desired result

Introduce functions that implement the default behavior for the methods in the Typed and Struct type classes, so that users can rely on them when implementing their own instances.

Proposed solution

Introduce four definitions in copilot-core:Copilot.Core.Type that provide default implementations for the fields in the Typed and Struct type classes for types that are Generic. Let users use those types so long as they make their structs Generic.

Add an example to the examples directory in copilot that shows how the new implementations can be used.

Further notes

Discussion #540 proposes multiple alternative implementations. Although the preferred solution long-term is 2, the mechanism proposed is not backwards compatible, so it helps to first introduce a mechanism to simplify the implementation without breaking existing code. We can, upon release of this feature, communicate to users that Copilot will move in that direction if appropriate, and encourage users to transition their code so that we can introduce breaking changes 3 versions later per our deprecation policy.

@ivanperez-keera ivanperez-keera added CR:Type:Feature Admin only: Change request pertaining to new features requested CR:Status:Initiated Admin only: Change request that has been initiated labels Nov 11, 2024
@ivanperez-keera
Copy link
Member Author

Change Manager: Confirmed that the issue exists.

@ivanperez-keera ivanperez-keera added CR:Status:Confirmed Admin only: Change request that has been acknowledged by the change manager and removed CR:Status:Initiated Admin only: Change request that has been initiated labels Nov 11, 2024
@ivanperez-keera
Copy link
Member Author

Technical Lead: Confirmed that the issue should be addressed.

@ivanperez-keera ivanperez-keera added CR:Status:Accepted Admin only: Change request accepted by technical lead and removed CR:Status:Confirmed Admin only: Change request that has been acknowledged by the change manager labels Nov 11, 2024
@ivanperez-keera
Copy link
Member Author

Technical Lead: Issue scheduled for fixing in Copilot 4.2

Fix assigned to: @RyanGlScott .

@ivanperez-keera ivanperez-keera added this to the 4.2 milestone Nov 12, 2024
@ivanperez-keera ivanperez-keera added CR:Status:Scheduled Admin only: Change requested scheduled and removed CR:Status:Accepted Admin only: Change request accepted by technical lead labels Nov 12, 2024
@RyanGlScott
Copy link
Contributor

Just to be explicit about what I am planning to do for the next release (and to relate it back to the original discussion in #540), my plan is to add the following four functions to Copilot.Core.Type:

  • typeNameDefault :: (Generic a, ...) => a -> String
  • toValuesDefault :: (Generic a, ...) => a -> [Value a]
  • updateFieldDefault :: (Generic a, ...) => a -> Value t -> a
  • typeOfDefault :: (Typed a, Struct a, Generic a, ...) => Type a

Note the following differences from the proposal in #540:

  1. The naming convention is slightly different: I've moved "default" to the end of each function instead of at the beginning.
  2. There is now a default for typeName. This differs from RFC: Derive `Struct` and `Typed` instances for structs using `GHC.Generics` #540, where I envisioned that users would always supply the generated struct name manually. After talking about this with @ivanperez-keera, however, we came to the consensus that most of the time, it would be more convenient just to generate a typeName that converts the name of the Haskell datatype to snake_case (e.g., convert the name FooBar to foo_bar). This is not the only possible way to do it, but we can always add alternative ways to do it later.
  3. In light of having a dedicated typeNameDefault function, I propose not adding a GenericStruct newtype. It was mainly useful to allow users to easily specify the generated type name in conjunction with deriving, but now that we have a reasonable default for typeName, users won't need to perform this extra step in the first place.

@ivanperez-keera
Copy link
Member Author

ivanperez-keera commented Nov 12, 2024

Thank you, @RyanGlScott . This looks good.

Please let me know when you start working on this so that I switch it to "Implementation".

@RyanGlScott
Copy link
Contributor

I have a partial implementation of this (based on the work in #516). I'm currently cleaning it up and documenting it, so feel free to switch this to "Implementation".

@ivanperez-keera ivanperez-keera added CR:Status:Implementation Admin only: Change request that is currently being implemented and removed CR:Status:Scheduled Admin only: Change requested scheduled labels Nov 12, 2024
RyanGlScott added a commit to GaloisInc/copilot-1 that referenced this issue Nov 12, 2024
…ods. Refs Copilot-Language#564.

This adds functionality for implementing the class methods of `Struct` and
`Typed` using `GHC.Generics`. As such, one can now easily define instances of
these classes by deriving a `Generic` instance for the struct data type, i.e.,

```hs
data MyStructType = ...
  deriving Generic

instance Struct MyStructType where
  typeName = typeNameDefault
  toValues = toValuesDefault
  updateField = updateFieldDefault

instance Typed MyStructType where
  typeOf = typeOfDefault
```

This work is based off of an initial implementation by Marten Wijnja (@Qqwy)
from Copilot-Language#561. Unlike Copilot-Language#561, I do not yet change any of the default implementations
of any `Struct` or `Typed` methods. This is because several `Struct` instances
in the wild currently do not define implementations of `updateField`, and
moreover, they also do not define `Generic` instances for the corresponding
data types. As such, changing the default implementation of `updateField` to
use a `Generic`-based default would cause these instances in the wild to no
longer compile. We will explore changing the default implementations after a
suitable transition period.

Co-authored-by: Marten Wijnja <[email protected]>
RyanGlScott added a commit to GaloisInc/copilot-1 that referenced this issue Nov 12, 2024
…opilot-Language#564.

In the future, we plan to change the default implementation for `updateField`
such that it will require a `Generic` instance. This will break any existing
`Struct` instance that omits an implementation of `updateField` and also does
not define a `Generic` instance for the struct data type. Unfortunately, there
does not appear to be a way for GHC to warn about this combination of
properties, but we can at least warn about this in the `updateField` Haddocks.
RyanGlScott added a commit to GaloisInc/copilot-1 that referenced this issue Nov 12, 2024
…. Refs Copilot-Language#564.

Using recently added `Generic`-based implementations, much of the boilerplate
code needed to define `Struct` and `Typed` in the struct-related `copilot`
examples can be replaced with much simpler implementations. Note that I have
intentionally _not_ used `updateFieldDefault` in the `StructsUpdateField.hs`
example, as that example is intended to demonstrate how one would implement
`updateField` by hand.

This work is based off of an initial implementation by Marten Wijnja (@Qqwy)
from Copilot-Language#561.

Co-authored-by: Marten Wijnja <[email protected]>
RyanGlScott added a commit to GaloisInc/copilot-1 that referenced this issue Nov 12, 2024
RyanGlScott added a commit to GaloisInc/copilot-1 that referenced this issue Nov 13, 2024
@RyanGlScott
Copy link
Contributor

Implementor: Solution implemented, review requested.

RyanGlScott added a commit to GaloisInc/copilot-1 that referenced this issue Nov 13, 2024
…ods. Refs Copilot-Language#564.

This adds functionality for implementing the class methods of `Struct` and
`Typed` using `GHC.Generics`. As such, one can now easily define instances of
these classes by deriving a `Generic` instance for the struct data type, i.e.,

```hs
data MyStructType = ...
  deriving Generic

instance Struct MyStructType where
  typeName = typeNameDefault
  toValues = toValuesDefault
  updateField = updateFieldDefault

instance Typed MyStructType where
  typeOf = typeOfDefault
```

This work is based off of an initial implementation by Marten Wijnja (@Qqwy).

Note that I do not yet change any of the default implementations of any
`Struct` or `Typed` methods. This is because several `Struct` instances in the
wild currently do not define implementations of `updateField`, and moreover,
they also do not define `Generic` instances for the corresponding data types.
As such, changing the default implementation of `updateField` to use a
`Generic`-based default would cause these instances in the wild to no longer
compile. We will explore changing the default implementations after a suitable
transition period.

Co-authored-by: Marten Wijnja <[email protected]>
RyanGlScott added a commit to GaloisInc/copilot-1 that referenced this issue Nov 13, 2024
…opilot-Language#564.

In the future, we plan to change the default implementation for `updateField`
such that it will require a `Generic` instance. This will break any existing
`Struct` instance that omits an implementation of `updateField` and also does
not define a `Generic` instance for the struct data type. Unfortunately, there
does not appear to be a way for GHC to warn about this combination of
properties, but we can at least warn about this in the `updateField` Haddocks.
RyanGlScott added a commit to GaloisInc/copilot-1 that referenced this issue Nov 13, 2024
…. Refs Copilot-Language#564.

Using recently added `Generic`-based implementations, much of the boilerplate
code needed to define `Struct` and `Typed` in the struct-related `copilot`
examples can be replaced with much simpler implementations. Note that I have
intentionally _not_ used `updateFieldDefault` in the `StructsUpdateField.hs`
example, as that example is intended to demonstrate how one would implement
`updateField` by hand.

This work is based off of an initial implementation by Marten Wijnja (@Qqwy).

Co-authored-by: Marten Wijnja <[email protected]>
RyanGlScott added a commit to GaloisInc/copilot-1 that referenced this issue Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR:Status:Implementation Admin only: Change request that is currently being implemented CR:Type:Feature Admin only: Change request pertaining to new features requested
Development

Successfully merging a pull request may close this issue.

2 participants