Add Prim.Int.ToString compiler-solved class by JordanMartinez · Pull Request #4267 · purescript/purescript · GitHub
Skip to content

Add Prim.Int.ToString compiler-solved class - #4267

Merged
JordanMartinez merged 6 commits into
purescript:masterfrom
JordanMartinez:addIntToString
Apr 21, 2022
Merged

JordanMartinez merged 6 commits into
purescript:masterfrom
JordanMartinez:addIntToString

Conversation

@JordanMartinez

Copy link
Copy Markdown
Contributor

Description of the change

Fixes #4263. I looked through #4207 to guide me on how to implement this.


Checklist:

  • Added a file to CHANGELOG.d for this PR (see CHANGELOG.d/README.md)
  • Added myself to CONTRIBUTORS.md (if this is my first contribution)
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@purefunctor purefunctor mentioned this pull request Mar 17, 2022
3 of 4 tasks

@purefunctor purefunctor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This LGTM, but would there be any merit to a FromString type class as well?

Similarly, would generalizing ToString/FromString such that they can be used for Boolean/Ordering be a good idea? I figured not really, considering that unlike Int they don't have an arbitrary number of members.

@JordanMartinez

Copy link
Copy Markdown
Contributor Author

@paf31

paf31 commented Mar 21, 2022

Copy link
Copy Markdown
Contributor

Isn't FromString just the functional dependency run backwards?

@JordanMartinez

Copy link
Copy Markdown
Contributor Author

Isn't FromString just the functional dependency run backwards?

Yes.

@JordanMartinez

Copy link
Copy Markdown
Contributor Author

Isn't FromString just the functional dependency run backwards?

Actually, no, because an integer could be rendered in a couple of different ways. In PureScript syntax, we support both 123 and 0x0aF as valid integers. Should a type-level conversion also support that?

In other words, should ToString actually be something like this?

-- another compiler kind
data IntBase
foreign import data Base10 :: IntBase
foreign import data Base16:: IntBase

class ToString :: Int -> Base -> Symbol -> Constraint
class ToString int base string | int base -> string, string base -> int

-- compiler solved instances
instance ToString 123 Base10 "123"
instance ToString 123 Base16 "0x7B"

@JordanMartinez

Copy link
Copy Markdown
Contributor Author

I've spent some time on updating this PR. However, there's a number of factors to consider. I'm going to open an issue where this can be discussed further.

@JordanMartinez

Copy link
Copy Markdown
Contributor Author

Ha, there's already an issue. I'll further discuss there.

@JordanMartinez

Copy link
Copy Markdown
Contributor Author

Per the discussion in #4263, I believe this is ready for a final review.

@rhendric rhendric left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Everything here looks solid, but I'd like to see the following tested (could be the same test):

  • A case where the integer being ToString'd is not a literal—use Add or Mul to compute an integer that doesn't appear in the source, and then ToString that.
  • A case where the integer is big (larger than 2^64), given the initial motivation for this class.

@JordanMartinez

Copy link
Copy Markdown
Contributor Author

On it!

@JordanMartinez

Copy link
Copy Markdown
Contributor Author

@JordanMartinez
JordanMartinez merged commit f374270 into purescript:master Apr 21, 2022
@JordanMartinez
JordanMartinez deleted the addIntToString branch April 21, 2022 18:52
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.

Proposal: Add compiler-solved class for converting type-level Int to type-level String (Symbol)

4 participants