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

Use culture invariant to-string convertor for lval in replace operator by iSazonov · Pull Request #10954 · PowerShell/PowerShell · GitHub

Use culture invariant to-string convertor for lval in replace operator - #10954

Merged
Dongbo Wang (daxian-dbw) merged 7 commits into
PowerShell:masterfrom
iSazonov:make-replace-invariantculture
Dec 19, 2019
Merged

Use culture invariant to-string convertor for lval in replace operator#10954
Dongbo Wang (daxian-dbw) merged 7 commits into
PowerShell:masterfrom
iSazonov:make-replace-invariantculture

Conversation

Ilya (iSazonov) commented Oct 31, 2019
edited
Loading

Copy link
Copy Markdown
Collaborator

PR Summary

Fix #10948

PR Context

Options for the fix:

  • lval.ToString()
    That is before the fix.
  • Convert.ToString(lval, CultureInfo.InvariantCulture)
    Makes lval conversion being culture invariant.
  • PSObject.ToStringParser(context, lval)
    Current fix. Makes lval conversion being culture invariant and add PowerShell magic conversions if lval is PSObject (ex., follow ETS) See
    In the case behavior is as for -match operator
  • PSObject.ToStringParser(context, PSObject.AsPSObject(lval))
    The same as previous but always use PowerShell magic conversions (follow ETS).
    In the case behavior is as for -join operator (with performance issue because of mandatory wrapping to PSObject)

PR Checklist

Ilya (iSazonov) added Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Oct 31, 2019
Ilya (iSazonov) added this to the 7.0.0-preview.6 milestone Oct 31, 2019

Copy link
Copy Markdown
Contributor

Thanks for taking this on, Ilya (@iSazonov).

Generally, it sounds we should follow the example of -match and -split (assuming they work consistently among them), which is PSObject.ToStringParser(context, lval), which, from what I gather, is like using "$var" for a non-string value $var.

But I don't understand how PSObject.ToStringParser(context, PSObject.AsPSObject(lval)) differs in practice from PSObject.ToStringParser(context, lval).
Can you explain in more detail and give an example?

Ilya (iSazonov) commented Nov 1, 2019
edited
Loading

Copy link
Copy Markdown
Collaborator Author

But I don't understand how PSObject.ToStringParser(context, PSObject.AsPSObject(lval)) differs in practice from PSObject.ToStringParser(context, lval).
Can you explain in more detail and give an example?

In second case we could fall in fast way - TryFastTrackPrimitiveTypes() or IFormattable. In first case a code path is another - follow ETS at first.
You can see the logic in internal static string ToString(ExecutionContext context, object obj, string separator, string format, IFormatProvider formatProvider, bool recurse, bool unravelEnumeratorOnRecurse)

Copy link
Copy Markdown
Contributor

I see - but, speaking from observation (haven't looked a the code):

Apart from [pscustomobject] instances, ETS properties seem to only affect output formatting, not stringification (in expandable strings). Or are there cases where including ETS properties makes a difference for stringification? If so, what are they, and can you give a specific example?

Ilya (iSazonov) commented Nov 1, 2019
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Michael Klement (@mklement0) The example is in #10389 (comment)

ETS properties seem to only affect output formatting

You confuse ETS (Extended Type System) with extensions for Formatting System (Update-TypeData vs Update-FormatData).

Michael Klement (mklement0) commented Nov 1, 2019
edited
Loading

Copy link
Copy Markdown
Contributor

I see - you mean an ETS-overridden .ToString() method (I was thinking properties in general, which would only matter in formatting) - that's a helpful example.

So it sounds like -join is currently the only string-coercing operator that goes the ETS route.

-match, -split, -like do not.

In the case of #10389 the ruling was to prevent a behavioral -join change, even if that means forgoing a possible performance optimization.

Here, it comes down to this:

  • PSObject.ToStringParser(context, lval) means aligning -replace with -match, -split, -like and results in no behavior modification (except for the culture-sensitivity being fixed)

    • However, that means that an ETS .ToString() method is ignored.
  • PSObject.AsPSObject(lval)) is the conceptually correct thing to do, as it corresponds to what "$var" does (and you would expect the ETS to be honored consistently):

    • It would result in a behavioral change - arguably for the better - in that an ETS .ToString() method then would be respected, unlike before.

    • It would impact performance, as you state - I don't know to what extent.

I don't know what the performance implications are, but my personal recommendation would be to pick one of the following two options:

  • Make all string operators use PSObject.AsPSObject(lval)) to ensure that ETS .ToString() methods are consistently honored.

    • Technically, a breaking change, but in spirit it is more of a bug fix.
  • Fix -replace only and align with the majority behavior there, not least because -match, -split, -like are not only string operators but involve string matching (unlike -join), i.e., use PSObject.ToStringParser(context, lval) (and continue to have ETS .ToString() methods ignored).

Copy link
Copy Markdown
Collaborator Author

Michael Klement (@mklement0) Great investigations! Thanks!

Steve Lee (@SteveL-MSFT) Perhaps PowerShell-Committee should weight these Michael Klement (@mklement0) 's proposed options.

Rob Holt (rjmholt) added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Nov 25, 2019
Travis Plunk (TravisEz13) removed their assignment Dec 7, 2019

Copy link
Copy Markdown
Contributor

@PowerShell/powershell-committee reviewed this. It's definitely a breaking change (Bruce Payette (@BrucePay) is calling it Bucket 2: Reasonable Gray Area), but we think it's the right thing to do.

Given where we're at in the release cycle, and our new snap to .NET's 1-year release cycle, we want to take this as an experimental feature into 7.1-preview.1 so we can understand the breakage and whether we need to revert to old behavior.

Joey Aiello (joeyaiello) added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Dec 11, 2019
Ilya (iSazonov) force-pushed the make-replace-invariantculture branch from 034cc01 to 57e588b Compare December 12, 2019 12:57

Copy link
Copy Markdown
Collaborator Author

If you prefer to put the experimental feature name in a variable, then you can declare an internal const string field in the Const Member section to hold your experimental feature name.

It makes no sense because we can not use strong typed names out of SMA.
I reverted the change.

Copy link
Copy Markdown
Member

It makes no sense because we can not use strong typed names out of SMA.

If the experimental feature is not in SMA.dll, then it's not an engine feature and should not be declared in ExperimentalFeature.cs. Instead, it should be declared in the module where that dll is in.

Ilya (iSazonov) force-pushed the make-replace-invariantculture branch from fb3d4a5 to eca5759 Compare December 18, 2019 07:22

Copy link
Copy Markdown
Collaborator Author

Dongbo Wang (@daxian-dbw) Could you please continue?

Dongbo Wang (daxian-dbw) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM except for a comment.

Dongbo Wang (daxian-dbw) removed the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Dec 18, 2019

Copy link
Copy Markdown
Collaborator Author

Should we add CL-Experimental label?

Copy link
Copy Markdown
Member

I think we can only use 1 CL-xx label, otherwise the script to generate the change log will be confused.

Ilya (iSazonov) commented Dec 18, 2019
edited
Loading

Copy link
Copy Markdown
Collaborator Author

I think we can only use 1 CL-xx label, otherwise the script to generate the change log will be confused.

It seems Travis said me that CL-Breaking can be used with another CL- label.

From releaseTools.psm1

    # Array of PRs with multiple labels. The label "CL-BreakingChange" is allowed with some other "CL-*" label.

Dongbo Wang (daxian-dbw) commented Dec 18, 2019
edited
Loading

Copy link
Copy Markdown
Member

I don't know about that, but what would you expect the script to do then? placing the same PR under both Breaking Changes and Experimental Features sections?
I think CL-BreakingChange wins in this case. But thanks for the quote, I added the experimental label.

Dongbo Wang (daxian-dbw) added the CL-Experimental Indicates that a PR should be marked as an Experimental Feature in the Change Log label Dec 18, 2019

Dongbo Wang (daxian-dbw) commented Dec 18, 2019
edited
Loading

Copy link
Copy Markdown
Member

Hmm, by looking at the following code, it seems the script doesn't handle multiple labels even if CL-breakingchange is one of them, as the switch will fall into the default case when $clLabel contains more than 1 item.

if ($clLabel.count -gt 1 -and $clLabel.Name -notcontains 'CL-BreakingChange') {
$multipleLabelsPRs += $pr
}
elseif ($clLabel.count -eq 0) {
$unlabeledPRs += $pr
}
else {
switch ($clLabel.Name) {
"CL-BreakingChange" { $clBreakingChange += $commit }
"CL-BuildPackaging" { $clBuildPackage += $commit }
"CL-CodeCleanup" { $clCodeCleanup += $commit }
"CL-Docs" { $clDocs += $commit }
"CL-Engine" { $clEngine += $commit }
"CL-Experimental" { $clExperimental += $commit }
"CL-General" { $clGeneral += $commit }
"CL-Performance" { $clPerformance += $commit }
"CL-Test" { $clTest += $commit }
"CL-Tools" { $clTools += $commit }
"CL-Untagged" { $clUntagged += $commit }
"CL-NotInBuild" { continue }
Default { throw "unknown tag '$cLabel' for PR: '$($commit.PullRequest)'" }
}

Copy link
Copy Markdown
Collaborator Author

switch ($clLabel.Name) {

Here we will have an array and the switch will run for every value from the array.

Copy link
Copy Markdown
Member

Ah, you are right, I totally missed that.

Dongbo Wang (daxian-dbw) merged commit 54e6199 into PowerShell:master Dec 19, 2019
Ilya (iSazonov) deleted the make-replace-invariantculture branch December 19, 2019 18:03

Copy link
Copy Markdown

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

Handy links:

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-Experimental Indicates that a PR should be marked as an Experimental Feature in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

-replace operator stringifies its LHS operand culture-sensitively

7 participants


Back | FazBrowse Home | New Git URL