Fix String Parameter Binding for Biginteger numeric literals

[vexx32]

PR Summary

When passing a bigint literal, e.g., 7n, as a bare string to a parameter of type [string], the parameter binder was incorrectly using the direct ToString() conversion (unlike other numeric literals), which
store the original TokenText in order to properly retrieve the original string that was entered at the command line.

Fix is to ensure that we're capturing and storing the TokenText value when the value is of type BigInteger (since this type's TypeCode is simply object and is not read as being a numeric type by those APIs).

I also needed to add BigInteger to the array of numeric types stored in LanguagePrimitives in order for the conversion methods to recognise that BigInteger literals need to be converted to string more carefully.

Quick test case (interactive only):

function Test-StringValue([string]$Value) { $value }

Test-StringValue -Value 11n

# `11n` should be returned, but you will see `11` returned before these changes

PR Context

This PR resolves #11626

/cc @SteveL-MSFT
I know y'all are probably up to your necks with the v7 release -- I'm hoping this can pass under the wire for that; would definitely like to have it fixed for the upcoming release. 🙂

I'm not 100% sure the included tests are the best way to test this, but they certainly seem to do the job.

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
  • OR
  • I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
    • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode (which runs in a different PS Host).
    • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
    • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
    • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).

[iSazonov]

Makes sense to review all using LanguagePrimitives.IsNumeric() and make the fix more general?

[vexx32]

EDIT: Apparently I was dreaming or something, I'm not seeing a second overload of IsNumeric() now 😕

So yeah, the long and the short of it is that we can't determine from the type code alone whether it's numeric, we'd need to also pass the actual type into the method.

Given that, I think the most elegant solution is to add an overload of IsNumeric() which takes a Type directly, and it can handle the BigInteger case specifically as well as calling the existing method which operates on type code instead.

Probably better for a follow up PR than one to take place here though, I wanna get this fixed for the 7.0 release and not hold it up any with extensive changes. 😁

The fix can have a side effects. We should be careful. It is not for 7.0 GA. I think we should review all code paths.

[vexx32]

@iSazonov can you elaborate on what side effects you're seeing? 🙂

[vexx32]

If we'd like to instead do a thorough review of the IsNumeric usages and ensure they all align, we can do that too. I have a second branch live here with a more complete set of changes. If you'd prefer I can PR that instead; was just trying to keep changes minimal to simplify review. 🙂

[iSazonov]

can you elaborate on what side effects you're seeing?

I see above 20 references to IsNumeric() and after first look I have questions for every such reference.
It would be great to create a list of the references and comment every one whether we fix or no and why.
If there are references where BigInteger is important (but not all) probably we should create IsNumericOrBigInteger() method.

[vexx32]

@iSazonov I agree we should do something along those lines, but I don't think I'd be the right person for that job. Ideally I'd ask someone on the PS team (possibly multiple people) who is familiar with the code path(s) in question.

From a brief look through, I would guess we shouldn't see any adverse effects from always handling BigInteger the same way as we do other numeric types; from what I can see, handling it in this way doesn't break anything immediately obvious, and it also solves the issue raised in #11628

The other code paths using IsNumeric(), as you'd expect, all deal with handling numeric values in various scenarios (I see formatting code, remoting code, and parameter binding code, among a couple others). At least intuitively, I think we're best off continuing to handle them uniformly, and adding a standard case in for BigInteger, but as I said... I lack the familiarity others may have to really make a solid call on that.

Ultimately, I agree the right solution is to review and handle them all. If we have time to do so prior to 7.0 GA, that'd be my preference. If not, this more minimal change will prevent breaking existing scripts in at least this specific scenario. It mightn't be perfect, but it's a better state than we're sitting in currently. 🙂

[gurnec]

I've been looking at @vexx32's two branches to see if I could locate any bugs or departures from PS5/6 behavior, and I've found a couple.

First, for the more extensive branch with the new IsNumeric overload, many arithmetic expressions have issues, producing different results compared to PS5/6 (NewIsNumeric output is below):

PS> ([BigInt]7 + [BigInt]7).GetType().Name
Int32

PS> +[BigInt]3000000000
OperationStopped: Arithmetic operation resulted in an overflow.

PS> [BigInt]3000000000 -gt 0
OperationStopped: Arithmetic operation resulted in an overflow.

PS> [BigInt]0 -eq ''
True

These are caused by calling the new overload from ExtensionMethods.cs. If that change is reverted, the above is fixed.

This next issue exists in both branches:

PS> [string][BigInt]0
InvalidArgument: Cannot convert value "0" to type "System.String". Error: "Object must implement IConvertible."

PS> [BigInt]$null
OperationStopped: Invalid cast from 'System.Int32' to 'System.Numerics.BigInteger'.

In PS5/6, the first cast converts to a string as expected, while the second cast also fails though with a more clear exception. Treating a BigInteger as though it were a Numeric primitive in LanguagePrimitives.RebuildConversionCache seems to be the culprit. One way to fix this could be to create a new converter method such as in this update to the WrapBigintLiterals branch which treats BigInteger as any other object when converting it to a string (which eventually causes IFormattable.ToString to be called), but has the .TokenText special case from ConvertNumericToString.

I wish I were qualified to comment on whether or not other issues exist... but I thought I'd at least comment on what I'd found so far.

[vexx32]

@gurnec very much appreciate the thorough review on all points!

I'll check out your update there this weekend and see if can get it to break or not 😄

I have my doubts any of this will make it in time for v7 but I'll do what I can to get it sorted out properly either way. 💖

[iSazonov]

@vexx32 ping :-)

[vexx32]

I think I have some idea of what to do here... it'll need some modifications to the type converters for numeric types as well as adding biginteger to the types list. The issues @gurnec is finding probably come about because while we told PS that biginteger is a numeric type, we didn't tell it how to handle it as a numeric type.

I'm going to tinker a bit in my side branch and when I have everything working I'll merge it back into this branch. Thanks for the ping @iSazonov! 🙂

EDIT: Some tinkering later, and I think @gurnec has the right idea; having PS treat BigInteger as a regular number in all circumstances is not the way to go. It doesn't implement some of the interfaces necessary for those conversion paths to work correctly (most notably IConvertible). Since most conversions with it are currently behaving OK, we just need to add a few additional ones to iron out the edge cases. That shouldn't be too tricky to do. 🙂

[vexx32]

@gurnec @iSazonov looks like that should sort everything out.

Note: [bigint]0 -eq '' is supposed to return True, just like 0 -eq '' returns true (and similar for other standard numeric types) and will work correctly with the latest commit. It was not doing this previously.

Other fixed conversions:

• [string][bigint]0
• [bigint]$null
• [bigint]$true
• [bigint]$false

The other conversion failures @gurnec noted from the other branch never arose in this branch and will continue to work correctly. 🙂

EDIT: Noticed that bool->bigint was failing, pushed a fix and added them to the list.

[iSazonov]

@vexx32 I see many style issues.

It is not clear about IsNumeric() method. Should we fix it too?

[vexx32]

@iSazonov Yeah, I'm not inclined to go fixing too many style issues in these code sections at the moment. They really need a thorough pass in a separate PR and I'd rather not complicate the commits here too much.

A few things seem to have been caught by my autoformatter, I can remove those if we need to, but I've already got plans to do a good style pass on LanguagePrimitives anyway 🙂

I don't think we should incorporate this into IsNumeric() directly. The code paths that are calling this seem to pretty commonly assume that a result of true from that method means that the type they're asking about implements certain interfaces, which usually includes IConvertible -- and BigInteger does not implement this interface. Changing the IsNumeric() method now without thoroughly examining all the code paths using it is asking for a whole heap of exceptions to get thrown all over the place.

The changes as they are allow things to keep treating BigInteger as kind of its own thing, which it is, while also adding the necessary conversion paths for it to appear like a more normal number to users and allow it to be used as such.

[iSazonov]

I mean such style issues. We should either fix or revert.

[vexx32]

I'll trim these out for now to keep things simple. 👍

[iSazonov]

I don't think we should incorporate this into IsNumeric() directly.

Really there are some IsNumeric() methods and I guess ignoring its does not fix even parameter bindings.
We could split this on some PRs but we should have a full view.

[vexx32]

@iSazonov The current branch here as it is resolves the binding issues with BigInteger values. Reviewing IsNumeric() usages is likely to be a bit of a dead end in my opinion, but might still be valuable to do for some edge cases.

EDIT: I've rebased and trimmed the changes down to just the necessary ones for this PR. I think I'll do a style pass in this file afterwards, it really needs it 😁

[gurnec]

@vexx32 I'm afraid there's a regression in your current patch:

PS> [bigint]'0'
InvalidArgument: Cannot convert value "0" to type "System.Numerics.BigInteger". Error: "Object reference not set to an instance of an object."

ConvertStringToInteger() just can't handle most string to BigInt conversions. I'd suggest that you either remove the line below (and its related changes) to return to the default string-to-object-via-Parse() conversion:

PowerShell/src/System.Management.Automation/engine/LanguagePrimitives.cs

Line 4529 in 757a07c

CacheConversion<object>(typeofString, typeof(BigInteger), ConvertStringToInteger, ConversionRank.NumericString);

or if we want string-to-BigInt conversions to act closer to other string-to-numeric ones, something more complicated will be required like this commit over here.

That will fix the immediate issue of [bigint]'0' and also permit conversions such as [bigint]'0.0', [bigint]'0L', etc. (which are currently valid for other numeric types, such as [int]'0L'), but it still has an issue with [int]'0N'.

This second commit should fix the latter. Whether it makes sense to start down the road of making BigInts more like the other simple numeric types as opposed to keeping things simpler isn't really my call....

[vexx32]

Huh, really? That's quite odd. I thought I tested that & had that working. Must've broken it again... ah, yeah, I see what you're saying. I don't think it should require quite that complex of a solution though; you'll note the original ConvertStringToInteger() essentially does the same thing, just doesn't support converting to the BigInteger value itself. Shouldn't be too complex to work that in a little more effectively in there.

EDIT: Yeah the issue is essentially that GetIntegerSystemConverter() doesn't have a way to handle BigInteger and just returns null, which isn't expected in the method. Added some extra handling for when the target type is BigInteger that sorts it out reasonably neatly. 🙂

[vexx32]

@iSazonov @daxian-dbw is there anything still pending on this one? 💖

[gurnec]

The statement

BigInt should work the same as other numeric suffixes. This means that [bool]0n should be $false like other numbers.

was intended to be narrow in scope, is that correct?

Because otherwise, it should be pointed out that BigInts today behave quite differently than other numerics.

Expression	$x=65 or type=int64	$x=65n or type=bigint
[char]$x	'A'	InvalidArgument
[type][char]'A'	65	InvalidArgument
[type]''	0L	InvalidArgument
[type]'0L'	0L	InvalidArgument
[type]'0n'	InvalidArgument	InvalidArgument
[type]0.9	1L	0n
1/[type]2	0.5	0n
[type]1/2	0.5	0n

(The patches in the PR don't change any of the above, just the [bool]0n.)

My apologies for the noise if that statement was indeed intended to be narrow in scope.

[vexx32]

@gurnec expecting [bigint] to behave precisely like [long] is problematic. For one, even [decimal] doesn't support all the conversions you list.

Some of those I would like to fix (especially the ones involving rounding to bigint), but they will likely have to be addressed on an individual basis. For instance, I don't think it's a good idea (or really possible) for bigint to be falling back to double like the lower integer types tend to when you create fractional values via division, since double's range is significantly narrower than bigint's. I do appreciate the concise summary of the differences, though!

[gurnec]

For one, even [decimal] doesn't support all the conversions you list.

Actually it does, or rather it supports all the conversions that the other real-number type double supports. Considering that decimal is a .NET built-in numeric type (as opposed to BigInt), it makes sense that it's treated as a "real" numeric type in PowerShell too.

I'm not trying to be argumentative though... I completely agree with what you're saying.

[daxian-dbw]

And [string]1n should return 1n instead of just 1.

I think this was described incorrectly, [string]1n should return 1 just like all other literal numbers.
I believe what the committee meant was Test-StringValue -Value 11n in the example should return 11n instead of 11.

This means that [bool]0n should be $false like other numbers.

The committee commented on this as a reply to the ask, but this particular issue seems to be out of the scope of the initial purpose of this PR.
It looks to me most of the changes in LanguagePrimitives.cs are not needed for the original purpose, but please let me know if I misunderstood the changes.
Can you please separate the extra changes out to a new PR (maybe a new issue first to discuss the issues you are fixing)?

[vexx32]

I'll have to take a thorough look at the diffs. I agree that [string]1n should just return 1 (and it does in fact behave that way in this PR currently), I'm sure I just got myself confused with that comment.

I suppose I can pare that part out if necessary.

@daxian-dbw see #12623 for the main casting issues that were addressed here in the second commit. If we need to split it out I think I can manage that well enough, just let me know.

[daxian-dbw]

Yes please, it could be good to separate that fix to another PR.

Also, are the changes in ConvertStringToInteger needed for #12623 or #11626? It seems to me the changes to ConvertNullToNumeric and the update to conversion cache for (BigInt, string) is all that need in LanguagePrimitives.cs for #11626.

[vexx32]

@daxian-dbw I actually am not entirely sure. I will give it a look without those and get back to you on that this evening. 🙂

[vexx32]

@daxian-dbw yeah so if I take out the changes in the actual ConvertStringToInteger method, the following will happen:

1. The Assert() call will fail, because the TypeCode for BigInteger is the same as the type code for object.
2. If we only bypass that, the Convert.ChangeType() call can fail since BigInteger is not IConvertible, so the conversion cannot be handled by that method. This is a fallback method (when the number can't be parsed directly) so I'm not sure how often this path would be hit, but it's still a potential concern.

Looking at pulling off the latest commit now, might need some parts, will need to verify.

[vexx32]

@daxian-dbw I've split out the casting-related changes to #12629

[daxian-dbw]

@vexx32 Some tab completion tests about Get-CimInstance were failing. I restarted the windows CI and SSH CI, but if they fail again, can you please take a look to see if it's related to this change? Also, it would be great if you can address the code factor issue 😄

[vexx32]

@PoshChan please get failures

@daxian-dbw not sure how I can address the codefactor issue tbh. The using directive is in the right place. I think it's a false positive from the fact that there are some additional using directives further down behind some compiler flags.

[daxian-dbw]

@vexx32 Thanks for the clarification on the code factor issue.
As for the windows CI failures, they failed in master as well, so it's not related to your changes here.

[ghost]

🎉v7.1.0-preview.3 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes