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

Explicit numeric conversions #1624

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

edmundnoble
Copy link
Contributor

@edmundnoble edmundnoble commented Mar 20, 2023

This PR just adds type applications to uses of int, which is our alias to fromIntegral. Before merge, I recommend that we audit these conversions to make sure they're correct (especially correct w.r.t dimensions) and safe. An expansion to this would be using witch or something similar (perhaps vendored) to check that conversions are safe.

@imalsogreg
Copy link
Contributor

It's a big improvement, making all these casts explicit.

I bet there are also some readability wins to be had by scanning the int @_ @_'s for cases where a purpose-named function can take the place of a generic conversion function. E.g.

case window ver of
     Just (WindowWidth w)
         | int @BlockHeight @Natural (_blockHeight h) <= w -> foo

->

case window ver of
  Just win | windowContainsBlock win h -> foo

@jwiegley
Copy link
Collaborator

@edmundnoble What prompted this work?

@edmundnoble
Copy link
Contributor Author

@jwiegley A bad numeric conversion caused the catchup issue from 2.17.1, fixed in #1571. A function used to produce cuts for nodes that were catching up started to take a BlockHeight instead of a CutHeight, causing the /cut GET endpoint to interpret its parameter as a BlockHeight because the numeric conversion was not annotated with its result type. As a result, despite the /cut GET endpoint code not changing, it was broken by downstream code without a type error. We have an internal issue for this, I will make a backlink here.

@jwiegley
Copy link
Collaborator

Would it be better to have separate newtypes for block heights and cut heights?

@edmundnoble
Copy link
Contributor Author

edmundnoble commented Mar 24, 2023

We do, they just... Both have Num instances. Maybe we can remove those? That could also help, we do use them a lot though.

@jwiegley
Copy link
Collaborator

How often do we need to transparently use them as "numbers"? What if we removed those instances and made the conversions explicit?

@edmundnoble
Copy link
Contributor Author

That is a possibility too. But it will be more verbose. For example we frequently convert though between (MaxRank, MinRank) (TreeDB types) and BlockHeight, but MaxRank and MinRank wrap Natural, so we convert MaxRank -> Natural -> Word64 -> BlockHeight, three conversions.

@edmundnoble edmundnoble marked this pull request as ready for review April 13, 2023 18:18
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.

None yet

3 participants