Skip to content

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

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

Add Prim.Int.ToString compiler-solved class#4267
JordanMartinez merged 6 commits intopurescript: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
4 tasks
Copy link
Copy Markdown
Member

@purefunctor purefunctor left a comment

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

would there be any merit to a FromString type class as well?

I am not sure. For one, if it's needed, we can add it later without making a breaking change. For two, I'm not sure what situations may arise where it would be desirable to do that. Also, a FromString that works on Boolean/Ordering can be defined in PureScript without a huge performance hit (unlike Int's FromString), so I don't think it needs compiler support.

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented Mar 21, 2022

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.

Copy link
Copy Markdown
Member

@rhendric rhendric left a comment

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

IntToString.purs passes locally.

@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