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

Favour error over implicit conversions with generic number literals #7468

Conversation

bishabosha
Copy link
Member

@bishabosha bishabosha commented Oct 29, 2019

Currently, if we do something like val x = 0xcafebabe: BigDecimal, then we get a BigDecimal after some implicit conversion from an Int, because there is no instance of FromDigits.WithRadix[BigDecimal], as decimals only work with base 10. Someone may find this surprising when using the explicit type ascription to implement a custom number type.

  • This PR introduces inline summoner methods used to ensure that a given FromDigits[T] is of the correct subclass, (WithRadix, Decimal or Floating) else issue a compile time error.
  • The desugaring in Typer now calls these summoner methods with the target type T and then calling fromDigits on the result, which will cause a compile time error if the FromDigits[T] is of the incorrect subtype, rather than silently dropping to the default Int or Double interpretation.
  • The summoners are implemented in the standard library so user code may use them for other metaprogramming features such as string interpolators.

Alternatives

  • Implement fully in typer, but can not be used by other library code.
  • Use a macro on summoners with arguments of type FromDigits[T], this relies on macro subtype-checks to get a precise type, so does not appear in the signature - this allows a custom error message without synthesising a temporary given if one does not exist.
  • [Current Implementation] Avoid a macro and use more precise types on the summoners' given parameters, whilst adding implicitNotFound messages to the subtypes of FromDigits and calling the summoners after proving there is already a given instance in scope of just FromDigits[T]. This would mean library code users would have to synthesise a given to get the same error.

@bishabosha bishabosha force-pushed the numerics-suppress-implicit-conversions branch 6 times, most recently from 93a44dc to 8f2ee3d Compare October 30, 2019 00:06
@bishabosha bishabosha force-pushed the numerics-suppress-implicit-conversions branch 4 times, most recently from 2ffab02 to 2819278 Compare October 30, 2019 10:25
@bishabosha bishabosha assigned bishabosha and odersky and unassigned bishabosha Oct 30, 2019
@bishabosha bishabosha force-pushed the numerics-suppress-implicit-conversions branch 2 times, most recently from d739b33 to af43f16 Compare October 31, 2019 09:33
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

The PR should also update the doc page to reflect what is implemented.

@odersky odersky assigned bishabosha and unassigned odersky Nov 1, 2019
@bishabosha bishabosha force-pushed the numerics-suppress-implicit-conversions branch from ea1e00a to 7890726 Compare November 1, 2019 12:38
@bishabosha
Copy link
Member Author

I've updated the docs

@bishabosha bishabosha assigned odersky and unassigned bishabosha Nov 1, 2019
@@ -112,6 +112,36 @@ class NumberTooSmall (msg: String = "number too small") extends FromDigi
class MalformedNumber(msg: String = "malformed number literal") extends FromDigitsException(msg)
```

### Compiler Expansion

The companion object `FromDigits` also defines four methods that may be used to provide a given instance of one of the subclasses of `FromDigits`:
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is not enough. For instance, it is said that the companion object defines a method fromDecimalDigits but it is not specified where and when this method is called.

I believe if we want to this to go ahead, it needs a complete rework of the doc page, and the SIP committee has to decide on the changes. My concern is that once the additions are fully worked in, we might decide that things became too complex.

@odersky
Copy link
Contributor

odersky commented Nov 2, 2019

One possibility is to put this on hold until the SIP committee has had a chance to go over it.

@odersky odersky removed their assignment Nov 2, 2019
@bishabosha
Copy link
Member Author

I've updated the doc page to list when each summoner method is used, as a stand in until rewriting.

@bishabosha
Copy link
Member Author

I've update the documentation again, I believe that it is easier to read, and it also incorporates the motivation for this change as a safety feature.

@bishabosha bishabosha force-pushed the numerics-suppress-implicit-conversions branch from 725f436 to 4155cf2 Compare November 7, 2019 10:59
@rjolly
Copy link
Contributor

rjolly commented Apr 10, 2020

I do not see it as a problem if numeric literals get converted to whatever possible type by implicit conversion. On the contrary, I think it would be a nice feature if the system was re-implemented in terms of implicit conversion. A numeric literal would be encoded as some hidden type (corresponding to FromDigits and subtraits as suited) and there would be an implicit conversion from the hidden type to the target type. This would allow numeric literals to take part in any chain of implicit conversions one could imagine, for instance in cases like:

18446744073709551616 + BigInt(1) // currently fails
BigInt(1) + 18446744073709551616 // succeeds

Edit : implicit conversion from literal to BigDecimal would make implicit conversion from Int to BigDecimal unnecessary. Consequently we could dispose of it, and the problematic example val x = 0xcafebabe: BigDecimal would vanish, because the needed conversion would be from some WithRadix.Literal to BigDecimal and it would not exist.

@odersky
Copy link
Contributor

odersky commented Nov 16, 2020

Closing for now. To be revived when we come to to generic number literals.

@odersky odersky closed this Nov 16, 2020
@SethTisue
Copy link
Member

label t:on-hold?

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.

4 participants