Skip to content

Improve type inference of array literals and foreach statement variables#8100

Merged
TravisEz13 merged 10 commits intoPowerShell:masterfrom
SeeminglyScience:foreach-iterator-type-inference
Mar 19, 2019
Merged

Improve type inference of array literals and foreach statement variables#8100
TravisEz13 merged 10 commits intoPowerShell:masterfrom
SeeminglyScience:foreach-iterator-type-inference

Conversation

@SeeminglyScience
Copy link
Copy Markdown
Contributor

@SeeminglyScience SeeminglyScience commented Oct 21, 2018

PR Summary

Improve type inference for foreach statement variables by:

  1. Inferring strongly typed arrays from explicit array and array literal expressions when elements are of the same inferred type

  2. Fix detection of foreach variable declaration. The previous logic was to check if the variable expression's start offset was after the end offset of the foreach statement, which will never be true in the body

  3. Improve inference of what type the "Condition" of a foreach statement will enumerate as

Here's some examples of scenarios that now work correctly:

foreach ($i in 0, 1, 2) { $i.<tab> }
# CompareTo    GetType      ToByte       ToDecimal    ToInt32      ToSingle     ToUInt16     TryFormat
# Equals       GetTypeCode  ToChar       ToDouble     ToInt64      ToString     ToUInt32
# GetHashCode  ToBoolean    ToDateTime   ToInt16      ToSByte      ToType       ToUInt64
foreach ($i in @{}.GetEnumerator()) { $i.<tab> }
# Key          Name         Value        Deconstruct  Equals       GetHashCode  GetType      ToString
$dict = [System.Collections.Generic.Dictionary[string, string]]::new()
foreach ($i in $dict.GetEnumerator()) { $i.<tab> }
# Key          Value        Deconstruct  Equals       GetHashCode  GetType      ToString
foreach ($i in $dict.GetEnumerator()) { $i.Key.<tab> }
# Length            GetType           PadLeft           ToChar            ToLowerInvariant  ToUpperInvariant
# Clone             GetTypeCode       PadRight          ToCharArray       ToSByte           Trim
# CompareTo         IndexOf           Remove            ToDateTime        ToSingle          TrimEnd
# Contains          IndexOfAny        Replace           ToDecimal         ToString          TrimStart
# CopyTo            Insert            Split             ToDouble          ToType
# EndsWith          IsNormalized      StartsWith        ToInt16           ToUInt16
# Equals            LastIndexOf       Substring         ToInt32           ToUInt32
# GetEnumerator     LastIndexOfAny    ToBoolean         ToInt64           ToUInt64
# GetHashCode       Normalize         ToByte            ToLower           ToUpper

Note: I specifically avoided testing for IEnumerable<> directly because some types like string are not enumerated by PowerShell. I know there's more types than string, if someone could point me to a full list of types that implement IEnumerable but PowerShell doesn't enumerate, I'll add in testing for that as well. Found it, I used PSEnumerableBinder for reference.

PR Checklist

Fix an issue where the assignment of a foreach statement would not be
detected during type inference. This issue was caused by a check
that the end offset of the foreach statement was before the start
offset of the variable expression. This was never true inside the
foreach statement body, which caused the assignement never to be
included.
Added the ability to infer an explicit array or array literal
expression as a strongly typed array based on the inferred type
of it's elements.
Inferring the type of the current enumerated value variable now has
better detection of how the foreach condition would enumerate.
@iSazonov iSazonov added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Nov 16, 2018
Copy link
Copy Markdown
Collaborator

@BrucePay BrucePay left a comment

Choose a reason for hiding this comment

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

LGTM.

@stale
Copy link
Copy Markdown

stale bot commented Dec 26, 2018

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

@stale stale bot added the Stale label Dec 26, 2018
@vexx32
Copy link
Copy Markdown
Collaborator

vexx32 commented Dec 26, 2018

@BrucePay @TravisEz13 (when you guys get back from the holiday break) was there something pending on this PR? 🙂

@stale stale bot removed the Stale label Dec 26, 2018
@stale
Copy link
Copy Markdown

stale bot commented Feb 4, 2019

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

@stale stale bot added the Stale label Feb 4, 2019
@SeeminglyScience
Copy link
Copy Markdown
Contributor Author

I got caught up in other projects. I'm planning to address feedback this weekend.

@stale stale bot removed the Stale label Feb 10, 2019
@TravisEz13
Copy link
Copy Markdown
Member

@SeeminglyScience I think this is complete, but I'm going to hold this PR until 6.3. We are really close to starting lock down for release and I want to make sure a change like this has plenty of time to get feedback before the release.

My primary reasoning is that there maybe script that make assumptions about our type inferences and break because of this change. Although this change shouldn't be a breaking change, I think we should be a little cautious.

@TravisEz13 TravisEz13 added this to the 6.3.0-preview.1 milestone Feb 11, 2019
@stale
Copy link
Copy Markdown

stale bot commented Mar 13, 2019

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

@stale stale bot added the Stale label Mar 13, 2019
@SeeminglyScience
Copy link
Copy Markdown
Contributor Author

@TravisEz13 @iSazonov Is it possible to disable stale bot since this is waiting on 6.2 GA?

@stale stale bot removed the Stale label Mar 14, 2019
@iSazonov
Copy link
Copy Markdown
Collaborator

Nevermind. Any comment removes the mark.

@TravisEz13 TravisEz13 merged commit b4ebf63 into PowerShell:master Mar 19, 2019
@TravisEz13
Copy link
Copy Markdown
Member

We have already forked for GA.

@SeeminglyScience SeeminglyScience deleted the foreach-iterator-type-inference branch March 19, 2019 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants