FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Number literals: parse integer values too large to fit into Decimal as BigInteger (bigint), not Double · Issue #13212 · PowerShell/PowerShell · GitHub

Repository navigation

Number literals: parse integer values too large to fit into Decimal as BigInteger (bigint), not Double #13212

Closed

Description

Note: This is technically a breaking change, but arguably one that falls into Bucket 3: Unlikely Grey Area.

Summary of the new feature/enhancement

Currently and historically, an integer number literal in source code that is too big (in absolute terms) to fit into [decimal] is parsed as a [double] - this invariably leads to a loss of accuracy:

# 99999999999999999999999999999 is greater than [decimal]::MaxValue, so it is parsed as a [double]
PS> [bigint] 99999999999999999999999999999
99999999999999991433150857216

Given that 7.0 introduced the n suffix to explicitly request a [bigint] and that suffixes are generally optional, with PowerShell picking the appropriate type, it would make sense to parse integer values outside the range of [decimal] as [bigint].

In other words: it would make sense to parse 99999999999999999999999999999 the same as 99999999999999999999999999999n.

Activity

  1. vexx32 commented on Jul 19, 2020

    Collaborator

    I'm personally for this, but to my memory this option was pretty quickly vetoed when I was initially implementing the n suffix for BigInteger.

  2. iSazonov commented on Jul 20, 2020

    Collaborator

    this option was pretty quickly vetoed

    Rain Sallow (/u/ta11ow) (@vexx32) Could you find the conclusion?

  3. vexx32 commented on Jul 20, 2020

    Collaborator

    Looks like it was here-ish: #7993 (comment)

    Looking back at the discussion, the question posed was a little narrower in scope - asking whether we should have hex/byte number literals automatically use BigInteger and leave decimal literals alone (I suppose I assumed it was too much of a reach to change decimal literals as well). Even that wasn't accepted, so we'd need the committee's approval to go ahead and make an even larger change here, for sure.

    IMO it would be a good change, but if it does break something it'll be quite confusing and hard to pin down exactly what had changed and why.

  4. mklement0 commented on Jul 20, 2020

    ContributorAuthor

    Thanks for digging, Rain Sallow (/u/ta11ow) (@vexx32).

    True, hypothetically something could break, but (a) number literals that large are unusual, (b) in case where they were used, it strikes me as likely that the loss of accuracy was undesired and simply went unnoticed, (c) PowerShell's flexibility around handling numbers of different types makes it unlikely that something truly breaks (as opposed to simply now operating on a more accurate value).

    (That in calculations the same type promotion as in number literals isn't applied is a separate problem: [long]::MaxValue + 1 becomes a [double](!), and [decimal]::MaxValue + 1 curiously breaks).

  5. iSazonov commented on Jul 20, 2020

    Collaborator

    I should say that BigInteger is not full "native" numeric in PowerShell and in .Net too. So I'd do not force the issue.

  6. mklement0 commented on Jul 20, 2020

    ContributorAuthor

    I'd say that with the introduction of the n suffix [bigint] has gone native - PowerShell-native, that is.

    True, [bigint] isn't a .NET primitive type ([bigint].IsPrimitive yields $false), but I'm not sure why that distinction matters, given that PowerShell handles it just fine, and we've chosen to support literals with suffix n.

  7. vexx32 commented on Jul 20, 2020

    Collaborator

    The issue with how calculations behave in terms of widening is because all calculations are handled there basically the same as they are in .net from what I can tell.

    Double is the next option up from integer types, but decimal doesn't get widened in the same way. I assume that's probably because it's already designed to be a fairly precise type, and promoting in would be a huge loss of precision.

  8. iSazonov commented on Jul 20, 2020

    Collaborator

    I'd say that with the introduction of the n suffix [bigint] has gone native - PowerShell-native, that is.

    We did not update all code paths in PowerShell.
    I asked in .Net Runtime and they said they saw no reason to make this type native and I agreed because it is still an edge case.

  9. mklement0 commented on Jul 20, 2020

    ContributorAuthor

    Ilya (@iSazonov), what code paths weren't updated and how does that affect the end user?

    Why should it matter whether it is a primitive type or not?

    Clearly, we've decided to support this type in a PowerShell-idiomatic fashion, by allowing it to be used in number literals.

    The current behavior amounts to a subtle source of bugs: the number you think you'r expressing in your source code quietly turns into something else, with loss of accuracy.

    If, for some reason, the implicit promotion to [bigint] is deemed too risky (again: why?), the proper solution would be to report an error (and force users to use n explicitly) - but that would be a problematic breaking change.

  10. mklement0 commented on Jul 20, 2020

    ContributorAuthor

    Rain Sallow (/u/ta11ow) (@vexx32), from what I can tell .NET (at least via C#) doesn't have implicit type promotions (update: in calculations) at all, so you either get an overflow exception (long.MaxValue + 1) or you let the overflow happen with an unchecked statement - but then you stay within the original type (and therefore the calculation doesn't work as intended).

    So I assume that PowerShell chose those widening rules, and unfortunately it chose different rules for number literals vs. calculations.

    Note that even going beyond [long] can result in loss of accuracy:

    # LHS is converted from [double], resulting in loss of accuracy: 922337203900225945*6*
    # RHS is accurate, because the entire operation is [bigint]: 922337203900225945*4*
    PS> [bigint] ([long]::MaxValue + [int]::MaxValue) -eq ([bigint] [long]::MaxValue) + [int]::MaxValue
    False
  11. SeeminglyScience commented on Jul 20, 2020

    Contributor

    Michael Klement (@mklement0)

    Rain Sallow (/u/ta11ow) (@vexx32), from what I can tell .NET (at least via C#) doesn't have an implicit type promotion at all, so you either get an overflow exception (long.MaxValue + 1) or you let the overflow happen with an unchecked statement - but then you stay within the original type (and therefore the calculation doesn't work as intended).

    For parsing literals it does:

    https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQBMQGoA+ABATARgFgAobAZgAI8KBhCgbxIuasuwBYKBZACgEoGTFsIBuEBBRgBPAA5wKAXgoAGAB65lmrcoDcQ4czETpc3IpXrtV63uLCAviXtA

    Anything higher than long and it throws though.

    C#/VB/F# compiler playground.
  12. mklement0 commented on Jul 20, 2020

    ContributorAuthor

    Thanks for the demonstration, Patrick Meinecke (@SeeminglyScience), but we were talking about calculations, not number-literal parsing.

    With respect to number-literal parsing PowerShell already does its own thing (never picking unsigned types, and allowing values larger than [long]), so we're not tied to C#'s behavior.

  13. 6 remaining items

  14. mklement0 commented on Nov 12, 2020

    ContributorAuthor

    Sometimes the tradeoff of loss of accuracy is preferable to the loss of functionality.

    I think the current quiet loss of accuracy is insidious - a source of subtle bugs, so the change is worth making for that reason alone.

    There is no loss of functionality - you can always down-cast to [decimal] or [double] if you want to call a [math] method.

    As stated in the OP, the change is technically breaking, but I still see this as a bucket 3 change, especially given that it's likely that code that relies on the current behavior is conceptually flawed (lack of awareness of the loss of accuracy).

  15. vexx32 commented on Nov 12, 2020

    Collaborator

    Yeah, that's a very good point as well. BigInteger is a more specialized type, and it doesn't have the same common support throughout .NET's math or conversion classes.

    It does have some alternate math methods on BigInteger's class itself as static methods, but it isn't a complete 1:1 compatibility with everything that's supported with the more general-purpose number types, and still requires some rewriting of scripts.

  16. mklement0 commented on Nov 12, 2020

    ContributorAuthor

    Rain Sallow (/u/ta11ow) (@vexx32):

    If you write code with number literals that you know to result in loss of accuracy - yet you need the [double] or [decimal] type for library calls - you should either signal that with, say, [double] 99999999999999999999999999999 or, preferably, run [bigint] [double] 99999999999999999999999999999 and then use the true resulting integer as the source-code literal: [double] 99999999999999991433150857216 - accompanied by an explanatory comment.

    Also note that the [math] problem would only arise for number literals too large to fit into a [decimal], only in which case [bigint] is called for.

    The current behavior has no redeeming qualities that I can see.

  17. vexx32 commented on Nov 12, 2020

    Collaborator

    Sure, but most folks don't know / necessarily need to be aware of that at the moment; it "just works" in many ways (excepting if you need precision, which I'd like to think concerned folks would have noticed anyway, I suppose).

    Currently if you have a script that uses a large literal and somewhere along the line you're using a [math]::Pow() or any other [math] static method, changing this would break that. Whether that's worth doing / a bit too confusing for folks to run into / etc, I don't really know. I don't think the impact would be huge but it would be an unexpected and somewhat inexplicable break for the folks that do hit it. The errors you encounter there tend to be pretty opaque (e.g., could not find an overload for "MethodName" and the argument count: "X" -- no mention of mismatching types in some cases) and so finding what's actually changed is a bit tricky at best.

  18. mklement0 commented on Nov 12, 2020

    ContributorAuthor

    Subverting the user expectation that a number literal such as 99999999999999999999999999999 isn't used as such, accurately is something that should never have been implemented: it is counter-intuitive, happens quietly, and can lead to subtle bugs - I am not at all optimistic about concerned folks having awareness.

    • Fixing that by parsing error would be a problematic breaking change - and needlessly limiting.

    • By contrast, widening the type automatically - to [bigint], if [decimal] is too small - is in the spirit of PowerShell and is useful.

      • As noted, the mere fact that we provide a [bigint] type suffix, n, indicates that we consider [bigint] a first-class citizen, and just like you don't have to use, say, 2147483648L with the L explicitly to get a [long] - just 2147483648 is sufficient - you shouldn't be required to use n - 99999999999999999999999999999n - to get a [bigint].

    Yes, existing code can break, but the benefit of the change to me far outweighs the risk, especially considering that:

    • only number literals starting at 79228162514264337593543950336 ([decimal]::MaxValue + 1, on the order of 8e28) are affected.

    • existing code may still continue to work, depending on how the numbers are handled; yes, [math] calls would break, but not regular arithmetic operations such as * and +.

  19. TimCurwick commented on Nov 12, 2020

    Contributor

    I take it back. Sorry. I was thinking of something else.
    I misinterpreted the title of the issue and, despite reading the full issue and comments, failed to notice what the proposal is actually about. I don't have a preference for parsing the number literals discussed.
    (Just don't change the behavior of [int64]::MaxValue+1.) (Well, you can, but I'll whine about it a little bit.)

  20. mklement0 commented on Nov 12, 2020

    ContributorAuthor

    Thanks, Tim Curwick (@TimCurwick).

    I find [int64]::MaxValue+1 returning a [double] - i.e., the behavior in calculations as opposed to the parsing of number literals - quite unfortunate as well, but it is indeed a separate issue.

    Making calculations use the same type-widening rules as the parsing of literals would make much more sense, but I do realize that this would be a more severe breaking change.

  21. changed the title [-]Number literals: parse integer values too large to fit into Decimal as BigInteger, not Double[/-] [+]Number literals: parse integer values too large to fit into Decimal as BigInteger (bigint), not Double[/+] on Nov 27, 2020
  22. microsoft-github-policy-service commented on Nov 16, 2023

    Contributor

    This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

  23. microsoft-github-policy-service commented on Nov 16, 2023

    Contributor

    This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

  24. microsoft-github-policy-service commented on Nov 16, 2023

    Contributor

    This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

  25. microsoft-github-policy-service commented on Nov 23, 2023

    Contributor

    This issue has been marked as "No Activity" as there has been no activity for 6 months. It has been closed for housekeeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Breaking-Changebreaking change that may affect usersIssue-Enhancementthe issue is more of a feature request than a bugResolution-No ActivityIssue has had no activity for 6 months or moreWG-Languageparser, language semantics

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions


      Back | FazBrowse Home | New Git URL