New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix parser to continue parsing key-value pairs after an If-Statement value in a HashExpression #7002
Conversation
…Statement value in a HashExpression
// a = if (1) {} | ||
// b = 10 | ||
// } | ||
var restorePoint = PeekToken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use PeekToken()
instead of _tokenizer.GetRestorePoint()
here because it's possible there is an unget token (_ungetToken != null
) at this point, and in that case, _tokenizer.GetRestorePoint()
will return the offset after the unget token, and we will lose the unget token when resyncing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decide to directly check _ungotToken == null
. I think this way will deliver the intent more clearly. And also, in more common cases, _ungotToken
would be null when reaching here.
$result = @{ | ||
a = if (1) {'a'} | ||
b = 'b' | ||
c = if (0) {2} elseif (1) {'c'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also test:
- newlines after
}
- if/elseif/else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks!
Is a conditional item supposed to work over newlines? I recall seeing some documentation that used that somewhere, but I don't think the implementation supports that at the moment. EDIT: Like: $x = @{
b = if ($something)
{
75
}
else
{
62
}
} |
@rjmholt I guess you see that use from https://docs.microsoft.com/en-us/powershell/gallery/concepts/module-psedition-support#sample-module-manifest-file-with-compatiblepseditions-key :) The current implementation supports that. |
Yes you're quite right. I was experimenting with it before and must have done something wrong. |
Might be worth including a test with that scenario |
h = 'h' | ||
} | ||
|
||
# With extra new lines after if-statement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the comment should be more stringent
Caution!!! Don't remove extra new lines after if-statement!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when looking at the BeforeAll
block, it's clear enough that the format of those hashtable expressions shouldn't be changed.
PR Summary
Fix #6970
After parsing
if () { }
, new lines are skipped in order to see if eitherelseif
orelse
is present. When neither is present, we should resync back to the pointer before skipping those possibly available new lines, so that the new line tokens can be utilized by the subsequent parsing.PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
to the beginning of the title and remove the prefix when the PR is ready.[feature]
if the change is significant or affects feature tests