[WIP] Support ListLiteralExpression '[]' in PowerShell by daxian-dbw · Pull Request #5761 · PowerShell/PowerShell

Dongbo Wang (daxian-dbw) · 2017-12-29T21:53:54Z

PR Summary

An attempt to address #5643

This design adds the ListLiteralExpression '[]' to PowerShell, which is similar to the ArrayLiteralExpression. They are different in mainly two ways:

ListLiteralExpression always returns a List<object>
When there is only one element expression, ListLiteralExpression unwraps the value of the expression while ArrayLiteralExpression directly uses the value as the first element of the returned array.

Please review with #5762 as a comparison, where list is supported as a ListExpression '@[]', similar to ArrayExpression '@()'.

Example

# an empty list
> $l = []
> $l.GetType()

IsPublic IsSerial Name                                     BaseType
-------- -------- ----                                     --------
True     True     List`1                                   System.Object

# one element list, the element is a type
> $l = [[string]]
> $l.Count
1
> $l[0]

IsPublic IsSerial Name                                     BaseType
-------- -------- ----                                     --------
True     True     String                                   System.Object

# multi-element list
> $l = [$pid,2,$home,4]
> $l.Count
4

# unwrap the only expression element in a list literal expression, like '@()'
> $l = [(dir)]
> $l.GetType()

IsPublic IsSerial Name                                     BaseType
-------- -------- ----                                     --------
True     True     List`1                                   System.Object

> $l.Count
30

# as a comparison, see the behavior of ArrayLiteralExpression
> $a = ,(dir)
> $a.Count
1

PR Checklist

Note: Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
User facing Documentation needed
- Issue filed - Issue link:
Change is not breaking
Make sure you've added a new test if existing tests do not effectively test the code changed
- Add [feature] if the change is significant or affectes feature tests
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

…) but return a list

Mark Kraus (markekraus) · 2017-12-29T22:33:27Z

~~why [] and not @[]? seeing just [] is confusing compared to types. also [()] is weird... I'd expect something similar to @():~~

Nevermind.. missed the other PR.

Dongbo Wang (daxian-dbw) · 2017-12-29T22:42:03Z

Mark Kraus (@markekraus) My first design attempt is to support @[], behaving like @(), see PR #5762. However, unlike character ')' that always starts a new token, the character ']' doesn't force to start a new token and therefore can be included in GenericToken, meaning command names, arguments, function names and etc. It makes impossible to determine where '@[]' ends and I cannot find an elegant solution without breaking changes.

We need more discussions on the design, and that is why I submitted 2 PRs for the list support: #5762 to add @[], and this one to add [].

Jason Shirk (lzybkr)

Since this is wip, I just did a quick skim.

Jason Shirk (lzybkr) · 2017-12-29T22:23:32Z

            return new NewArrayInitInstruction<T>(elementCount);
        }
+
+        protected internal override Instruction NewListInit(int elementCount)


I don't think we should add this to the interpreter - this way it stays as close as possible to original and/or the one used for dynamic when now JIT is available.

It is possible to add new instructions without touching the interpreter it that's how you want to do it - see PowerShellLoopExpression for an example.

The NewListInitInstruction is a minor optimization which can be removed.
But I need to implement LightCompiler.CompileListInitExpression because it's currently not implemented. Are you suggesting to not use ListInitExpression in our compiler generated code?

I see the method is now implemented: http://source.dot.net/#System.Linq.Expressions/System/Linq/Expressions/Interpreter/LightCompiler.cs,a988c37e1da7a799

I wonder if there is a supported way of invoking the interpreter now? We'd have to make sure they have any of our fixes first, but it'd be nice to not ship two copies of basically the same thing.

Great, I can use their implementation then.

Jason Shirk (lzybkr) · 2017-12-29T22:26:08Z

    }

+    /// <summary/>
+    public interface ICustomAstVisitor3 : ICustomAstVisitor2


I think numbered interfaces made sense when the release cycle spanned years, but I'm worried we'll end up with ICustomAstVistor9 before too long.

One (maybe not awesome) idea is to avoid the problem for this feature is to add a bool flag to ArrayLiteralAst - because they should be structurally the same.

The downside to this approach is that some analysis tools that aren't aware of list literals will analyze the ast as an array, e.g. they may incorrectly give warnings about performance.

I thought about reusing the ArrayLiteralAst, but didn't go that direction for two reasons:

An explicit ListLiteralAst is more discoverable.

ArrayLiteralAst can appear on the left side of an assignment. I didn't plan to make [] also assignable. (but I don't object to the idea to make it capable of the same).

I would like to see more discussion on this before updating the code.

Jason Shirk (lzybkr) · 2017-12-29T22:34:41Z

+            {
+                Expression values = Compile(listLiteralAst.Elements[0]);
+
+                if (values.Type == typeof(List<object>)) { return values; }


I don't understand this. Does this mean the following would be true?

$x = [1] $y = [[1]] $x.Count -eq $y.Count -and $x[0] -eq $y[0]

Reading more of the implementation, I'm confused now. Is this supposed to emulate ArrayExpressionAst or ArrayLiteralAst - it seems like the former, but it's named like the later.

This is a change I made that is different from ArrayLiteral -- when there is only one expression element in the ListLiteral, unwrap the value of the expression element like @() does.
This is because I think it may be better for [(dir)] to return a list containing all files than it to return a list containing an object array.
It's just one of my thoughts, not necessarily a right one 😄

You shouldn't call it a literal if you can't guarantee the number of objects in the result.

Good point. I will remove that part of logic.

Jason Shirk (lzybkr) · 2017-12-29T22:49:17Z

+
+            do {
+                if (commaToken != null) { SkipToken(); }
+                V3SkipNewlines();


It's probably time to get rid of V3SkipNewlines - I was overly cautious introducing that method and it hasn't been used.

Will replace V3SkipNewlines with SkipNewlines

Jason Shirk (lzybkr) · 2017-12-29T22:53:58Z

+            }
+            else if (listValues.Count > 0)
+            {
+                extent = ExtentOf(lBracket, listValues[listValues.Count-1].Extent);


This excludes a trailing comma. The simple approach to tracking the end is to just have a variable endExtent and keep updating it with each token that you consume or ast you add to listValues.

Update to use a endExtent variable.

Dongbo Wang (daxian-dbw) · 2018-01-08T20:07:26Z

Per the discussions in #5643, it's commonly agreed that we want ListExpressionAst which behaves similar to @() instead of a ListLiteralAst. Therefore, I will close this PR.

Dongbo Wang (daxian-dbw) added 2 commits December 28, 2017 17:03

ListLiteral: Parser and Engine changes

9b8b5c7

[Feature] ListLiteral: when there is only one element, behave like @(…

3469218

…) but return a list

Dongbo Wang (daxian-dbw) requested review from Bruce Payette (BrucePay), Jason Shirk (lzybkr) and sergei (vors) as code owners December 29, 2017 21:53

Dongbo Wang (daxian-dbw) changed the title ~~[WIP] Support list literal expression in PowerShell~~ [WIP] Support list literal expression '[]' in PowerShell Dec 29, 2017

Dongbo Wang (daxian-dbw) changed the title ~~[WIP] Support list literal expression '[]' in PowerShell~~ [WIP] Support ListLiteralExpression '[]' in PowerShell Dec 29, 2017

Dongbo Wang (daxian-dbw) mentioned this pull request Dec 29, 2017

[WIP] Support ListExpression '@[]' in PowerShell #5762

Closed

6 of 9 tasks

Jason Shirk (lzybkr) suggested changes Dec 29, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) mentioned this pull request Dec 29, 2017

PowerShell should support creating an List similar to how it supports arrays #5643

Closed

Dongbo Wang (daxian-dbw) added 2 commits December 29, 2017 16:06

Use 'endExtent' to track the end extent

c327e51

Do not unwrap the only expression element value

b58f1b1

Dongbo Wang (daxian-dbw) closed this Jan 8, 2018

Dongbo Wang (daxian-dbw) deleted the literalList branch April 3, 2018 19:24

Conversation

Dongbo Wang (daxian-dbw) commented Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Example

PR Checklist

Uh oh!

Mark Kraus (markekraus) commented Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dongbo Wang (daxian-dbw) commented Dec 29, 2017

Uh oh!

Jason Shirk (lzybkr) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Dongbo Wang (daxian-dbw) commented Dec 29, 2017 •

edited

Loading

Mark Kraus (markekraus) commented Dec 29, 2017 •

edited

Loading

Dongbo Wang (daxian-dbw) Dec 29, 2017 •

edited

Loading

Dongbo Wang (daxian-dbw) commented Jan 8, 2018 •

edited

Loading