Fix tab completions for a hash table by iSazonov · Pull Request #6839 · PowerShell/PowerShell

Ilya (iSazonov) · 2018-05-08T15:02:48Z

PR Summary

Now working:

Get-Date | Sort-Object @{Expression=<tab>
Get-Date | Sort-Object @{Expression=...;<tab>

PR Checklist

Jason Shirk (lzybkr) · 2018-05-08T15:33:41Z

                        replacementLength = 0;
                        break;

+                    case TokenKind.Semi:


Does adding this case affect get-something; e<TAB> - in other words, no hashtable, just simple command completion?

No, I don't see the side affect.
Maybe add the test?

I re-check get-something; e<Tab> - in Windows PowerShell 5.1 and in PowerShell Core 6.0 simple command completion works - nothing new added in the PR.

Ilya (iSazonov) · 2018-05-14T06:41:19Z

Steve Lee (@SteveL-MSFT) Dongbo Wang (@daxian-dbw) Could you please review too?

Dongbo Wang (daxian-dbw) · 2018-05-14T20:52:07Z


+                    case TokenKind.Semi:
+                        // Handle scenarios such as 'gci | Format-Table @{Label=...;<tab>'
+                        result = GetResultForHashtable(completionContext);


get-something;<tab> does go through this path. It's better to check if (lastAst is HashtableAst) before calling GetResultForHashtable.

Dongbo Wang (daxian-dbw) · 2018-05-14T20:56:38Z

+                                }
+                                // Handle scenarios such as 'gci | Format-Table @{Label=<tab>'
+                                return null;
+                            }


This duplicates the call to GetResultForEnumPropertyValueOfDSCResource.
How about changing to this:

case TokenKind.LParen: { if (lastAst is AttributeAst) { ... } else if (lastAst.Parent is DynamicKeywordStatementAst) // <-- change here { ... } else if (lastAst is HashtableAst hashTableAst && CheckForPendingAssignment(hashTableAst)) // <-- change here { return result; } break; }

Second condition doesn't work for configuration foo { File ab { Attributes =( test - really lastAst.Parent is CommandExpressionAst.

Fixed.

Dongbo Wang (daxian-dbw) · 2018-05-14T21:02:35Z

+        $result.CompletionMatches.Count | Should -Be 3
+        $result.CompletionMatches[0].CompletionText | Should -Be 'Expression'
+        $result.CompletionMatches[1].CompletionText | Should -Be 'Ascending'
+        $result.CompletionMatches[2].CompletionText | Should -Be 'Descending'


This test succeeds even without changes in this PR. You need to check ReplacementIndex and ReplacementLength, as they were -1 previously and changes in this PR should fix them.

Dongbo Wang (daxian-dbw) · 2018-05-15T19:48:20Z

                            {
-                                //
-                                // Handle auto completion for enum/dependson property of DSC resource,
+                                // 1. Handle scenarios such as 'configuration foo { File ab { Attributes ='


Handle auto completion for enum/dependson property of DSC resource

The example configuration foo { File ab { Attributes = is a completion for enum property, so it should not be marked as 1..

Dongbo Wang (daxian-dbw) · 2018-05-15T19:54:49Z

                                result = GetResultForAttributeArgument(completionContext, ref replacementIndex, ref replacementLength);
                            }
-                            else
+                            else if (lastAst.Parent is DynamicKeywordStatementAst || lastAst.Parent is CommandExpressionAst)


We are making two special checks here. Maybe we should only have one special check, like the following. What do you think?

if (lastAst is AttributeAst) { // the original attribute handling } else if (lastAst is HashtableAst hashTableAst && !(lastAst.Parent is DynamicKeywordStatementAst) && CheckForPendingAssignment(hashTableAst)) { return null; } else { // the original else block }

Looks good. Thanks!

Fixed.

Dongbo Wang (daxian-dbw) · 2018-05-15T19:56:44Z

+        $cmd = "Get-Date | Sort-Object @{Expression=...;"
+        $result = TabExpansion2 -inputScript $cmd -cursorColumn $cmd.Length
+        $result.CurrentMatchIndex | Should -Be -1
+        $result.ReplacementIndex | Should -Be 40


And ReplacementLength?

Dongbo Wang (daxian-dbw) · 2018-05-15T19:56:46Z

+        $result.CompletionMatches[2].CompletionText | Should -Be 'Descending'
+        $result.CompletionMatches[0].CompletionText | Should -Be 'Expression'
+        $result.CompletionMatches[1].CompletionText | Should -Be 'Ascending'
+        $result.CompletionMatches[2].CompletionText | Should -Be 'Descending'


$result.CompletionMatches[2].CompletionText is checked twice.

It was for ReplacementLength :-)

Fixed.

Ilya (iSazonov) · 2018-05-16T14:08:54Z

The while code is very tricky and sensetive 😕 and crush PowerShell if something wrong. Maybe we should make it more robust.

Dongbo Wang (daxian-dbw)

LGTM.
Ilya (@iSazonov) What while code were you referring to?

Ilya (iSazonov) · 2018-05-16T17:41:28Z

I means our Completion code - if I return wrong index PowerShell is crashed. Asts can be combined in many ways. Can we be sure that we tested all their? The code is complex and tricky - can we create simple algorithm? Using visitors?

Ilya (iSazonov) · 2018-05-17T06:15:38Z

Jason Shirk (@lzybkr) Steve Lee (@SteveL-MSFT) Do you have any comments?

Dongbo Wang (daxian-dbw) · 2018-05-21T23:08:55Z

Ilya (@iSazonov) Sorry for the late reply. I agree the tab completion code is a bit hard to organize now. Many changes have been made that are very specific to the scenarios they try to solve, so you can see checks in CompletionAnalysis that are tied to one particular scenario which is hard to understand just by reading the code.

However, the crash is possibly not related to tab completion, but the code rending results to console. If you mess up the index, the rendering code might do weird bad things. I don't think a visitor can help much here, as many checks need to inspect ancestors of an ast.

Ilya (iSazonov) · 2018-05-22T03:05:26Z

Dongbo Wang (@daxian-dbw) Thanks for great comment!

Now working: - Get-Date | Sort-Object @{Expression=<tab> - Get-Date | Sort-Object @{Expression=...;<tab>

[Feature] Fix tab completions for a hash table

8d226ea

Ilya (iSazonov) self-assigned this May 8, 2018

Ilya (iSazonov) requested review from Steve Lee (SteveL-MSFT) and Jason Shirk (lzybkr) May 8, 2018 15:02

Ilya (iSazonov) requested review from Bruce Payette (BrucePay) and Dongbo Wang (daxian-dbw) as code owners May 8, 2018 15:02

Jason Shirk (lzybkr) reviewed May 8, 2018

View reviewed changes

Dongbo Wang (daxian-dbw) requested changes May 14, 2018

View reviewed changes

Address feedback

0fbf2c3

Dongbo Wang (daxian-dbw) requested changes May 15, 2018

View reviewed changes

[Feature] Correct test and 'if' logic

c047bd5

Dongbo Wang (daxian-dbw) approved these changes May 16, 2018

View reviewed changes

Ilya (iSazonov) merged commit 626a83b into PowerShell:master May 21, 2018

Ilya (iSazonov) deleted the fix-tabcompletion branch May 21, 2018 04:06

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026

Fix tab completions for a hash table (PowerShell#6839)

6377589

Now working: - Get-Date | Sort-Object @{Expression=<tab> - Get-Date | Sort-Object @{Expression=...;<tab>

Uh oh!

Conversation

Ilya (iSazonov) commented May 8, 2018

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) May 8, 2018 • 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!

Ilya (iSazonov) commented May 14, 2018

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!

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!

Ilya (iSazonov) commented May 16, 2018

Uh oh!

Dongbo Wang (daxian-dbw) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented May 16, 2018

Uh oh!

Ilya (iSazonov) commented May 17, 2018

Uh oh!

Dongbo Wang (daxian-dbw) commented May 21, 2018

Uh oh!

Ilya (iSazonov) commented May 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ilya (iSazonov) May 8, 2018 •

edited

Loading