GaloisInc / llvm-pretty

An llvm pretty printer inspired by the haskell llvm binding
Other
28 stars 15 forks source link

Define `Lift` instances for AST types #81

Closed RyanGlScott closed 3 years ago

RyanGlScott commented 3 years ago

In GaloisInc/crucible#672, I found myself wanting Lift instances for the Symbol, PrimType, and Ident data types from Text.LLVM.AST to decrease the code size of TH splices involving these types. I'd like to add such instances to the llvm-pretty library, and the most straightforward way to do this would be to attach deriving Lift clauses to each of these data types. This proof-of-concept commit shows how it could be done.

I have two questions I'd want to answer before I would feel comfortable submitting a patch for this:

  1. While I only have a direct need for Lift instances for the three data types above, we could just as well define Lift instances for all other types in Text.LLVM.AST. Would you have any objections to this idea?
  2. One complication is that the DeriveLift extension was introduced in GHC 8.0, but llvm-pretty.cabal's version bounds on base seems to indicate that it supports pre-8.0 versions of GHC. In light of this, we could:

    i. Tighten up the version bounds to only allow 8.0 or later ii. Use CPP so that deriving Lift is only used when building with 8.0 or later iii. Define the Lift instances by hand to support all versions of GHC

    I'm not fond of option (iii), as defining Lift instances by hand is tedious. I don't have a strong opinion between options (i) and (ii). For what it's worth, the current version of llvm-pretty doesn't seem to actually support pre-8.0 in practice, since if you try to compile it with GHC 7.10.3, it will error out:

    Building library for llvm-pretty-0.11.0..
    
    src/Text/LLVM/AST.hs:22:8:
       Could not find module ‘Data.Semigroup’
       It is a member of the hidden package ‘semigroups-0.19.1@2v1dfSOvrzj1tEQS2HrG7N’.
       Perhaps you need to add ‘semigroups’ to the build-depends in your .cabal file.
       It is a member of the hidden package ‘semigroups-0.19.1@FDpEROrPB105hC2RCShNv5’.
       Perhaps you need to add ‘semigroups’ to the build-depends in your .cabal file.
       Use -v to see a list of the files searched for.

    So perhaps option (i) is needed anyway. Which option would you prefer?

elliottt commented 3 years ago

Given how old 8.0 is now, I think it would be very reasonable to tighten the bounds and say that 8.0+ is required to build. Do you know of any uses that would break if a 7.x GHC wasn't supported anymore?

RyanGlScott commented 3 years ago

Do you know of any uses that would break if a 7.x GHC wasn't supported anymore?

Not that I'm aware of. I couldn't find an example of a project using llvm-pretty that was relying on a pre-8.0 version of GHC, at least.

elliottt commented 3 years ago

Thanks for looking, let's go ahead with tightening the bounds 👍

RyanGlScott commented 3 years ago

Sounds good to me.

As far as an answer to question (1) from https://github.com/elliottt/llvm-pretty/issues/81#issue-977022908 goes, I experimented to see what would happen if I gave everything in Text.LLVM.AST a Lift instance. Unfortunately, that doesn't quite work out of the box. That's because a handful of data types (e.g., Define) have fields with Map types, and Map doesn't have a Lift instance. There does exist a "canonical" orphan Lift instance for Map in the th-lift-instances package, but using that in llvm-pretty would require incurring an extra dependency.

In light of this, I think there are three possible options:

  1. Define a Lift instance for everything by adding a dependency of th-lift-instances.
  2. Define a Lift instance for all data types except those with Map fields.
  3. Only define Lift instances for Symbol, PrimType, and Ident, which are the three instances that are directly needed in GaloisInc/crucible#672.
elliottt commented 3 years ago

Why don't we see how far option 3 gets you, and we can revisit adding additional dependencies and instances if it turns out you need more?

RyanGlScott commented 3 years ago

Sounds good to me. See #82.