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

Manually Defining Lens with Type Constraints #3971

Open
Xinlu-Y opened this issue Sep 26, 2024 · 7 comments
Open

Manually Defining Lens with Type Constraints #3971

Xinlu-Y opened this issue Sep 26, 2024 · 7 comments

Comments

@Xinlu-Y
Copy link
Collaborator

Xinlu-Y commented Sep 26, 2024

I use makeClassyFor to generate lenses for a data structure OldCodeSpec. One of the fields, _authors, has a constraint HasName a => [a]. I encountered the following error:
image
Here is the relevant part of my code:

data OldCodeSpec where
  OldCodeSpec :: (HasName a) => {
    _pName :: Name,
    _authors :: [a],
    ...
  } -> OldCodeSpec

makeClassyFor "HasOldCodeSpec" "oldCodeSpec"
  [ ("_pName", "pNameO"),
    ("_authors", "authorsO"),
    ...
  ] ''OldCodeSpec

data CodeSpec = CS {
  _sysInfo :: SI.SystemInformation,
  _oldCode :: OldCodeSpec
}
makeLenses ''CodeSpec

instance HasOldCodeSpec CodeSpec where
  oldCodeSpec :: Lens' CodeSpec OldCodeSpec
  oldCodeSpec = oldCode

  authorsO :: HasName a => Lens' CodeSpec [a]
  authorsO = oldCode . authorsO
  -- other lenses...

It seems like Template Haskell cannot infer or generate the lens for _authors because of the HasName constraint on the type a. Should I manually define the lens for authorsO with the HasName constraint while still using makeClassyFor for the other fields, and resolve this issue with the type constraint in both OldCodeSpec and CodeSpec?

@JacquesCarette
Copy link
Owner

Yes, you have diagnosed the problem correctly and proposed the best solution.

@Xinlu-Y
Copy link
Collaborator Author

Xinlu-Y commented Sep 26, 2024

The type variable a is existentially quantified within the OldCodeSpec constructor, which makes it inaccessible outside of the OldCodeSpec. Because of this, I'm unable to define functions or lenses that expose [a] outside of OldCodeSpec.

When pattern matching on OldCodeSpec, the type a is treated as a fresh, independent type, not connected to any external type variable. This limitation prevents me from generically accessing or manipulating the authors field ([a]).

I'm looking for a solution that allows me to keep the authors field generic while also being able to define functions and lenses that interact with it, but without exposing the internal type variable in an accessible way outside of OldCodeSpec. However, the current structure of OldCodeSpec makes it difficult to work with this field effectively.

@balacij
Copy link
Collaborator

balacij commented Sep 27, 2024

Was this resolved? Reed told me that you both discussed this in person, but I'm not sure where things ended. I can have a look at this tomorrow if needed.

@Xinlu-Y
Copy link
Collaborator Author

Xinlu-Y commented Sep 27, 2024

Was this resolved? Reed told me that you both discussed this in person, but I'm not sure where things ended. I can have a look at this tomorrow if needed.

Yes, I do need help. Although ScopedTypeVariables is enabled, it only allows type variables to be shared within the same scope (e.g., a function). It cannot make the a type variable from the OldCodeSpec constructor visible outside that context. This causes a mismatch between the type expected by authorsO and the type within the OldCodeSpec constructor.

image

drasil-code   >     • Relevant bindings include
drasil-code   >         authors :: [a1] (bound at lib/Language/Drasil/CodeSpec.hs:104:27)
drasil-code   >         getter :: OldCodeSpec -> [a]
drasil-code   >           (bound at lib/Language/Drasil/CodeSpec.hs:104:5)
drasil-code   >         setter :: OldCodeSpec -> [a] -> OldCodeSpec
drasil-code   >           (bound at lib/Language/Drasil/CodeSpec.hs:108:5)
drasil-code   >         authorsO :: ([a] -> f [a]) -> OldCodeSpec -> f OldCodeSpec
drasil-code   >           (bound at lib/Language/Drasil/CodeSpec.hs:100:1)

@Xinlu-Y
Copy link
Collaborator Author

Xinlu-Y commented Sep 27, 2024

After discussing this problem with Jason, we have identified two potential solutions.

Upon reviewing the code, we noticed the following:

$ grep -rn "instance HasName"
grep: drasil-lang/.hie/Language/Drasil/People.hie: binary file matches
drasil-lang/lib/Language/Drasil/People.hs:79:instance HasName Person where

The HasName typeclass has only one instance: Person. Moreover, in practice, the authors field in OldCodeSpec is always a list of Person objects. So I think maybe using a generic type for authors in OldCodeSpec is unnecessary.

Solution 1: Use a Concrete Type for authors in OldCodeSpec

  • Modify the type of authors from HasName a => [a] to simply [Person].
  • Remove the HasName constraint from the OldCodeSpec data structure since it's no longer needed.
  • Automatically generate lenses for OldCodeSpec, including authorsO.

Solution 2: Remove authors from OldCodeSpec and Use SystemInformation

  • Remove the authors field from OldCodeSpec to simplify its data structure.
  • Retrieve author information through the existing SystemInformation data structure instead of OldCodeSpec.

I think Solution 1 is a more straightforward approach. Since the authors field is always [Person] in practice, specifying the concrete type removes unnecessary complexity. I'd like to hear Dr. Carette's @JacquesCarette opinion on this.

@balacij
Copy link
Collaborator

balacij commented Sep 27, 2024

I'm not sure if Solution 2 will resolve anything anymore, because I think we still expect to create a lens for authors for SystemInformation either way. Is that right?

Regarding solution 1, the HasName typeclass only defines one 1 function: nameStr :: a -> String, so the only thing we can retrieve from the current authors field appears to be the "name strings." So, like you say, we can either switch to [People], but we can also change it to a [String] (I don't think that this is the right solution), or even a [UID] (if we add People to the ChunkDB.

@Xinlu-Y
Copy link
Collaborator Author

Xinlu-Y commented Sep 27, 2024

I'm not sure if Solution 2 will resolve anything anymore, because I think we still expect to create a lens for authors for SystemInformation either way. Is that right?

Regarding solution 1, the HasName typeclass only defines one 1 function: nameStr :: a -> String, so the only thing we can retrieve from the current authors field appears to be the "name strings." So, like you say, we can either switch to [People], but we can also change it to a [String] (I don't think that this is the right solution), or even a [UID] (if we add People to the ChunkDB.

Yes, after reconsidering, I agree that Solution 2 might not fully address the issue. That said, switching to People is a viable and straightforward approach and and I will upload the code changes to issue #3950 . As for using [String] or [UID], I feel that switching to [String] may simplify things too much, potentially losing some of the detailed information about the authors. On the other hand, adopting [UID] could introduce additional complexity, but it might still be worth considering for future discussion.

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

No branches or pull requests

3 participants