Add type restrictions to big by Vici37 · Pull Request #15689 · crystal-lang/crystal

Vici37 · 2025-04-20T04:43:27Z

This is the output of compiling cr-source-typer and running it with the below incantation:

CRYSTAL_PATH="./src" ./typer spec/std_spec.cr --error-trace --exclude src/crystal/ --stats --progress --union-size-threshold 2 --ignore-private-defs src/big

This is related to #15682 .

BlobCodes · 2025-04-20T08:35:19Z

src/big/big_decimal.cr

  # BigDecimal.new(1).div(BigDecimal.new(3), 5) # => BigDecimal(@value=33333, @scale=5)
  # ```
-  def div(other : BigDecimal, precision = DEFAULT_PRECISION) : BigDecimal
+  def div(other : BigDecimal, precision : UInt64 | Int32 = DEFAULT_PRECISION) : BigDecimal


I think any unions exclusively consisting of integral types should just be Int::Primitive

There's no real reason for these two types to be mentioned explicitly

Oh, good call, will make that change.

I don't think Int::Primitive is a good type for that. Most of the time it should be either Int or the biggest supported integer type (making use of autocasting).

I can't think of a reason where a negative value would be provided, so UInt64?

Negative values might not make sense, but even a positive value can be of a signed integer type. And that wouldn't autocast to an unsigned one.
Int should be fine here.

BlobCodes · 2025-04-20T08:36:32Z

src/big/big_decimal.cr

  # Factors out any extra powers of ten in the internal representation.
  # For instance, value=100 scale=2 => value=1 scale=0
-  protected def factor_powers_of_ten
+  protected def factor_powers_of_ten : UInt64?


It's a protected method whose result is never used, should be : Nil

BlobCodes · 2025-04-20T08:37:31Z

src/big/big_float.cr

  end

-  def <=>(other : Float::Primitive)
+  def <=>(other : Float::Primitive) : Int32?


All the <=> methods should normally just return Int32

Agreed, and the Nil return is a bit confusing here as well (though not incorrect when considering you're comparing with a NaN).

All the <=> methods should normally just return Int32

The contract in general is to return nil when not comparable. That is relevant for some types, but not all.

This Float::Primitive overload was indeed added as a breaking change in 1.9 because it needs to return nil. Note that BigFloat itself doesn't have NaN, since we are still using GMP, not MPFR

BlobCodes · 2025-04-20T08:39:18Z

src/big/big_rational.cr

    self
  end

  private def mpq


Why are some of these functions not typed at all?

Should be : LibGMP::MPQ*

The cr-source-typer has a new flag to ignore private defs, as well as type restrictions that are resolved to 3+ union types.

I realize my PR description doesn't show that with the single line command 😬

ysbaddaden · 2025-04-20T13:11:06Z

src/big/big_float.cr

  end

-  def self.default_precision
+  def self.default_precision : UInt64


The LibGMP method actually returns a LibGMP::BitcntT => LibGMP::UI => LibC::ULong, so the method returns a target dependent integer.

I think this outlines of the tool: it infers types for a specific target. It also exposes an issue in the stdlib: it leaks target dependent types.

Any recommendations on what this line should be? I can remove the return statement here, and agreed about the rest of your statement about leaking target dependent types.

Ah, I see the failed build as a consequence of this change. I'll remove the return type at least.

Instead of looking at the type of an expression in the current program, the typer tool could also apply some more generic type inference based on restrictions of the source/sink of these expressions.
In this case, it's pretty obvious that the return type of this method should be equal to the type restriction of LibGMP.mpf_get_default_prec, which is LibGMP::BitcntT.

Going this way ensures using the correct, target-independent aliases.

Makes sense - probably more work than is justified for this kind of edge case, but doing more inspection up and down the call-chain has been something I've been curious to do (for additional static code analysis type things).

ysbaddaden · 2025-04-20T13:11:47Z

src/big/big_float.cr

  end

-  def self.default_precision=(prec : Int)
+  def self.default_precision=(prec : Int) : Nil


This is worrysome: a setter method should always return its argument.

There is the older and abandoned #10083 for this

My own 2-cents: it should be a linting rule rather than a language rule, that the setter return what is handed to it. Seems there are other opinions on what setter methods should return, too (like matching the getter).

Yes, we'd want this to be different. But this change is fine for this PR.

You want it to return what was passed in so that chained assignment is handled. Ruby, for example, ignores returning a value from setters for this reason.

PRECISION = BigFloat.default_precision = 5

@Fryguy Yes, this is tracked in #7707.

src/big/big_decimal.cr

straight-shoota · 2025-04-23T13:48:01Z

src/big/big_float.cr

  end

-  def self.default_precision=(prec : Int)
+  def self.default_precision=(prec : Int) : Nil


Yes, we'd want this to be different. But this change is fine for this PR.

src/big/big_int.cr

src/big/big_rational.cr

src/big/json.cr

src/big/yaml.cr

mverzilli · 2025-08-12T10:08:07Z

src/big/big_decimal.cr

  # BigDecimal.new(1).div(BigDecimal.new(3), 5) # => BigDecimal(@value=33333, @scale=5)
  # ```
-  def div(other : BigDecimal, precision = DEFAULT_PRECISION) : BigDecimal
+  def div(other : BigDecimal, precision : Int = DEFAULT_PRECISION) : BigDecimal


This comes from way before this PR, but I would have expected precision to be a BigInteger in BigDecimal 🤔

FWIW, bigdecimal-rs, which this implementation is based on, does the same

Add type restrictions to big

de3506e

Vici37 mentioned this pull request Apr 20, 2025

Run source-typer on crystal standard library, give or take #15682

Draft

BlobCodes reviewed Apr 20, 2025

View reviewed changes

ysbaddaden reviewed Apr 20, 2025

View reviewed changes

Review comments

e29fa4b

straight-shoota reviewed Apr 23, 2025

View reviewed changes

Vici37 and others added 3 commits April 23, 2025 22:30

Review Comments

13c92e4

Merge branch 'master' into add-type-restrictions-to-big

1631f89

Merge branch 'master' into add-type-restrictions-to-big

76e065b

straight-shoota approved these changes Aug 9, 2025

View reviewed changes

straight-shoota added kind:docs topic:stdlib:numeric labels Aug 9, 2025

mverzilli reviewed Aug 12, 2025

View reviewed changes

mverzilli approved these changes Aug 12, 2025

View reviewed changes

straight-shoota added this to the 1.18.0 milestone Aug 12, 2025

straight-shoota merged commit 357d469 into crystal-lang:master Aug 15, 2025
41 checks passed

Uh oh!

Conversation

Vici37 commented Apr 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HertzDevil Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

HertzDevil Apr 23, 2025 •

edited

Loading