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

WIP a preview of what I have been working on #155

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

msftrncs
Copy link
Contributor

As was told to me early on, touching one thing breaks or discovers another, so its been difficult to find a stopping point.

This PR is meant to make it easy to see what I have been doing, and provide feedback, but its not necessarily intended to be an implemented PR.

@msftrncs
Copy link
Contributor Author

Somehow hit submit before I was ready here. There are some problem areas.

  • types/index operator, sub types close together at the first ] - working on a different idea for this, so it doesn't have to be a non-capturing end match.
  • I haven't gotten to the variables section yet so that it can match how the types accessors work, since that is not a totally working idea yet
  • I might have changed a lot of scopes. I was trying to pattern some of them after the C# syntax, since PowerShell is related. Treat these as experiments.

some sample differences (different versions of VSCode & syntax, same theme) (ignore the 'Activate Windows' overlay :))
image

image

image

@TylerLeonhardt
Copy link
Member

Just to check.

Is it "Before -> After" or "After -> Before" in your images?

@TylerLeonhardt
Copy link
Member

lots of good stuff :)

@msftrncs
Copy link
Contributor Author

Sorry, forgot that detail. Due to my existing window placement, its 'After <- Before' … kind of backwards, and I totally missed that someone probably would have trouble making that out.

@msftrncs
Copy link
Contributor Author

Some examples from the commit that will be coming:

  • these two are related
    • Handle complex function names (they can be file names with quotes)
    • consume text that might get incorrectly scoped elsewhere (primarily to prevent 'commentLine' from tripping in arguments
  • handle all variables together, members or not, gives one place to change the system variables
  • handle all assessors/members/indexers in one place (with a hook in each resource that can utilize an accessor, but the scoping is all done in one place) (method call parameters still handled in 'interpolatedStringContent')
    • accessors can span lines
    • methods scope as functions
    • members can be 'quoted'
  • operators can immediately trail variables and members and numbers with scalers or type indicators.

image

image
Nevermind some of the whimsical in the above.

@msftrncs
Copy link
Contributor Author

While working with the above (lower) test file, I noticed I was scoping ${`} as a variable, but I had put a note in 'TheBigTestFile.ps1' indicating it is not, so I added it to the top of this test file for testing fixing it, which then messed with the reference to the first function. It took me a few moments to realize that my initial tests were done without PSReadLine (probably in VS Code console) and so I had thought ${`} was not valid, but actually its the start of a multiline variable name that didn't find its closing } until the ending brace of the function body. I'll have to work on that, might be a good time to get the variable rules reorganized, as so far I have only propped the old rules up to be used by the accessors patterns.

Also note, at microsoft/vscode-textmate#82, is an issue that might affect what I have done here in some places, where I have relied on VS Code's behavior of \G to get the operators to work directly after a variable name or member. If the issue turns out to change VSCode-TextMate's behavior it may also break what I have done here. However, \G anchors only work as well as they do now due to microsoft/vscode-textmate#70.

A better example difference in some typical code: (old on top, this PR on bottom, same custom theme)
image

Old on left, this PR on right:
image

@msftrncs
Copy link
Contributor Author

Latest commit covers:

image

But now looking at it I just noticed that argumentModeEscapes is running before unquotedStrings.

@omniomi
Copy link
Collaborator

omniomi commented Jan 28, 2019

It's going to take me a while to review everything here and comment directly but a couple things just quickly:

handle all variables together, members or not, gives one place to change the system variables

What makes for a legal variable name changes inside strings, outside strings, inside sub-expressions, and using the ${} syntax. The splitting of the with-member/without-member was almost certainly due to these differences allowing you to easily include the relevant one.

Effectively you would need to move the sub-expression logic into the variable definitions and do some sort of "am I in a string" checking which would need to be more than just looking for the quotes as it would need to account for "string" + $Var + "string" style concatenation as well. (Which could also be just "string" + $Var or what if it's "string`" + $Var".) Believe me, for every "why would you ever do that?" there is someone doing it.

I have been playing with redoing the variables for a quite a while and I still don't know what the best course of action is tbh.

I might have changed a lot of scopes. I was trying to pattern some of them after the C# syntax, since PowerShell is related. Treat these as experiments.

Please be sure to read #138

There is substantial debate on what responsibility the syntax has toward accuracy vs. expectation. It also shouldn't be assumed that any other language is correct which raises the question do we aim for consistency with incorrect languages or aim for being "correct" in our own right and encourage other syntax projects to catch up?

Further, while PowerShell is related to C# it is also perl-like in a number of ways so there is value comparing certain elements to Perl highlighting and PHP highlighting. If sigils ($ for example) behave one way in Perl and PHP should PowerShell follow suit?

...

Thanks for submitting this PR, it's great to see a bunch of your work and ideas in one place. Will review as I have time.

@msftrncs
Copy link
Contributor Author

msftrncs commented Jan 28, 2019

handle all variables together, members or not, gives one place to change the system variables

What makes for a legal variable name changes inside strings, outside strings, inside sub-expressions, and using the ${} syntax. The splitting of the with-member/without-member was almost certainly due to these differences allowing you to easily include the relevant one.

Sorry, what I meant, is that this PR demonstrates an optimized approach (but only barely) where the decoding of the scopes of variables is handled in 1 spot, currently as a 'capture with patterns' (using a single regex to capture the entire variable name), and I resolved the logic behind members or not members with the original 'variable' vs 'variableNoProperty'. So now, the logic to scope $true appears in only 1 place, instead of 2 (now in 'variable_inner').

No worries about time. I know I still have some things I want to accomplish. Having said what I have above, I now know that variable names can span multiple lines once the ${} notation is used. I'd like to clean up some places where too much is allowed, such as members to variables in the parameter list of function/method definitions. I also plan to spend time looking at how different themes on VS Code handle what I have changed. To help with scoping I started a couple of scripts so I could investigate all the scopes being called out, and which repository item they appear under. I think this will grow in to a class, so I can create methods that can recursively query scoping through the includes, since sometimes repository items are nothing more than a list of includes.

I also plan to keep researching using a 'statement mode tokenizing process', for both PowerShell and Windows Batch. This would allow for a highlighting more consistent with the actual interpreter, the goal being a bit more accuracy and richness. (reference #153 (comment)) (Now demonstrated in PR #156)

My goal within this PR has been on the side of accuracy and richness, and it has been fairly achievable, but I have worked with languages that TextMate syntax could not achieve the level of accuracy (and thus the richness) I would have liked.

@SetTrend
Copy link

Just by chance: Any news on this PR?

@msftrncs
Copy link
Contributor Author

@SetTrend, no news. Is there anything in particular that this PR is of interest? Have you looked at PR #156?

This PR does contain some work towards enum and improved class syntaxing, that could probably be brought in to the current grammar file without much trouble.

In the long run, I think #156 is a better syntaxing solution. It can give the correct scoping while handling the extreme flexibility that PowerShell offers .

@TylerLeonhardt
Copy link
Member

The only thing I wait for is no "WIP" in the PR title and passing CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants