Skip to content
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

$_ scoped differently between inside and outside string.quoted.double #133

Closed
msftrncs opened this issue Aug 15, 2018 · 5 comments · Fixed by #166
Closed

$_ scoped differently between inside and outside string.quoted.double #133

msftrncs opened this issue Aug 15, 2018 · 5 comments · Fixed by #166
Assignees
Labels

Comments

@msftrncs
Copy link
Contributor

msftrncs commented Aug 15, 2018

Environment

  • Editor and Version: VS Code: 1.26.0
  • Your primary theme: Dark+

Issue Description

Trying to correct theming of '$_', I've noticed that '$_' is scoped as support.constant.automatic while outside a string.quoted.double, and support.variable.automatic when inside a string.quoted.double scope.

Expected Behavior

I would expect both to be consistent. I am not sure that scoping as a constant is correct, its not really a constant. I would consider it more of a read only variable. The same may go for many of the other $ automatic token variables. $_ is really the only one I use regularly.

@msftrncs
Copy link
Contributor Author

A quick check would seem to indicate that most of the other $ automatics are not scoped at all, or not as a whole.

@omniomi omniomi added the bug label Aug 15, 2018
@omniomi
Copy link
Contributor

omniomi commented Aug 15, 2018

The two different scopes is due legacy in the grammar where variables exist in two different repositories one for those that have properties called and one for those that do not. One looks like it was changed in 1.24 or 1.25 and the other in 1.26... I probably missed it. This will be addressed when I completely redo the way variables are captured.

Currently variables are incredibly difficult to work on as they're fragmented and duplicated. The process of redoing the whole thing is quite complicated though so will take some time.

am not sure that scoping as a constant is correct, its not really a constant. I would consider it more of a read only variable.

I agree with you that it's incorrect; However, I'd likely use the scope variable.language.* as it's functionally similar to this or self.

A quick check would seem to indicate that most of the other $ automatics are not scoped at all, or not as a whole.

Can you provide examples? I am not seeing any automatic variables that aren't scoped.

@omniomi omniomi self-assigned this Aug 15, 2018
@msftrncs
Copy link
Contributor Author

msftrncs commented Aug 15, 2018

@omniomi, so I find the following weirdness:

$_ scopes the _ to support.constant

$$ doesn't scope by itself, but does scope to support.constant if I include a '.' and a letter behind it.
$^, ditto
$?, similar, but ? scopes to keyword.control when no '.' and letter follow.

EDIT: to clarify the above, this is my example:

echo $$ $_ $^ $? there

I can see the patterns in the tmLanguage file, I don't see an issue with the regex. Other items of the same match work fine, such as $this, or $foreach. However, I tested the match with PowerShell -Match and I got the same results.

PS C:\Program Files\PowerShell\6.0.3> $matchtext = '(\$)(?i:(\$|\^|\?|_|Args|ConsoleFileName|Event|EventArgs|EventSubscriber|ForEach|Input|LastExitCode|Matches|MyInvocation|NestedPromptLevel|Profile|PSBoundParameters|PsCmdlet|PsCulture|PSDebugContext|PSItem|PSCommandPath|PSScriptRoot|PsUICulture|Pwd|Sender|SourceArgs|SourceEventArgs|StackTrace|Switch|This))((?:\.(?:\p{L}|\d|_)+)*\b)?\b'
PS C:\Program Files\PowerShell\6.0.3> '$$' -match $matchtext
False
PS C:\Program Files\PowerShell\6.0.3> '$$.d' -match $matchtext
True
PS C:\Program Files\PowerShell\6.0.3> '$this' -match $matchtext
True
PS C:\Program Files\PowerShell\6.0.3>

The only ones failing are the ones that need escapes, the $, ^ and (EDIT:) ?. It must be something about the required word boundary at the end of the regex. If I remove the final \b, then it will match.
Looks like this would work:

'(\$)(?i:((?:[$^?])|(?:_|Args|ConsoleFileName|Event|EventArgs|EventSubscriber|ForEach|Input|LastExitCode|Matches|MyInvocation|NestedPromptLevel|Profile|PSBoundParameters|PsCmdlet|PsCulture|PSDebugContext|PSItem|PSCommandPath|PSScriptRoot|PsUICulture|Pwd|Sender|SourceArgs|SourceEventArgs|StackTrace|Switch|This)\b))((?:\.(?:\p{L}|\d|_)+)*\b)?'

(Edit, I removed a \ in front of the $ inside the []'s)

@msftrncs
Copy link
Contributor Author

Just to add an interesting side note:
${$} -> $$
${^} -> $^
${?} -> $?
${_} -> $_
${this} -> $this
…etc.

These rare situations are captured as variable.other.

@msftrncs
Copy link
Contributor Author

@TylerLeonhardt, @omniomi, I will send a PR for the two main items I surfaced here, the mismatch in scopes between the automatic variables being used in expressions vs being used in interpolated strings, and also the error in the regular expression which prevents the punctuation based automatics ($, ^, ?) from scoping at all.

I think that the intended scope was supposed to be 'support.variable`, and not 'support.constant.variable'. Long term, that's not the right scope, but I just want to fix the inconsistency for now. I agree with @omniomi, long term, this particular instance should be 'variable.language', then the current use of 'variable.language' should probably be 'support.variable'.

msftrncs added a commit to msftrncs/EditorSyntax that referenced this issue Apr 27, 2019
corrects scope mismatch between #variable and #variableNoProperty for
automatics and fixes REGEX for $$, $^, and $?
TylerLeonhardt pushed a commit that referenced this issue May 8, 2019
* fixes #133
corrects scope mismatch between #variable and #variableNoProperty for
automatics and fixes REGEX for $$, $^, and $?

* fix tests for automatic variable scope change

* add tests for $$, $^, $?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants