Skip to content

Add support for expressions #7

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fflaten
Copy link
Contributor

@fflaten fflaten commented Apr 14, 2021

Experimenting with support for expressions in addition to variables.

Fix #3

  • Support properties
  • Support array index
  • Support dictionary keys
  • Accept variables as index/key in array + dictionary?
  • Exclude expressions using methods ()
  • Add setting to control feature
  • Add tests

@fflaten
Copy link
Contributor Author

fflaten commented Apr 14, 2021

Got a bit excited about the expression idea and played with it. I'm not familiar with typescript/javascript development, so this is only a proof-of-concept and most likely not best practice 😄 . Missing tests atm.

Pros:

  • Works, I think

Cons:

  • Regex is ugly and complex. A powershell-parser to access AST would've been nice.
  • InlineValueEvaluatableExpression returns the string-output (as expected) so every value gets quotes which can be confusing.

Demo:
image

@fflaten
Copy link
Contributor Author

fflaten commented Apr 15, 2021

Would have to reject lines using methods as they can modify the execution when stepping through the code.
methods-issue

@TylerLeonhardt
Copy link
Owner

TylerLeonhardt commented Apr 15, 2021

@fflaten technically it is possible for a property getter to change state.

class Person
{
  private string name; // field
  private int i;

  public string Name   // property
  {
    get { i++; return name; }   // get method
    set { name = value; }  // set method
  }
}

It's horrible and no one should do this... but it is possible. And since it's possible, we'll need setting to control enabling or disabling this feature.

I think the feature can be on by default. Let me know if you need help with that part but the docs on configuration can be found here:

https://code.visualstudio.com/api/references/contribution-points#contributes.configuration

@TylerLeonhardt
Copy link
Owner

Generally, I think what you may want to do is find all of the variables first.... then use their indexes in the file to see if they have a property. That way, instead of one regex that does everything, it's 2 regexes:

  1. to find variables
  2. to find properties on variables

This will be easier to write a setting for too, I think.

@fflaten
Copy link
Contributor Author

fflaten commented Apr 15, 2021

@fflaten technically it is possible for a property getter to change state.

Absolutely. This is actually a "problem"/risk with vscode / vscode-powershell too. If you expand the variable in the variable explorer and step over a line the update of the variable explorer view will trigger the same getter/scriptproperty. With this PoC-feature enabled, we potentially call this bad code twice on each step.

Good call on the setting - a must-have if this feature is implemented. Still on the fence whether or not this PR is worthwhile with these risks.

Another thing is that I originally assumed that vscode would use the range-value to keep track and only update inlinevalues as you step over them (including reruns for lines in a loop). That's not the case. On one hand updating previous inlinevalues are useful for easy visibility, but at the same time it is almost rewriting history. Showing current values are expected in the variable explorer, but scrolling up to previously executed code and seeing inlinevalues with a suddenly changed value can be confusing for users who might reuse variable names in the same local scope.

Generally, I think what you may want to do is find all of the variables first.... then use their indexes in the file to see if they have a property. That way, instead of one regex that does everything, it's 2 regexes:

  1. to find variables
  2. to find properties on variables

This will be easier to write a setting for too, I think.

Not sure about how it would affect the setting, but it would keep the code cleaner for variable-only scenarios. 👍

@fflaten
Copy link
Contributor Author

fflaten commented Jun 3, 2021

Time for an update here. I haven't completely abandoned this PR and it wasn't the configuration-option that scared me. 😄
I'm just reconsidering the scope of the feature as the current regex-based parsing will easily get complicated and harder to maintain if we'd like to support most scenarios.

Also considering #8 it would be very helpful to have access to AST so we can make smarter decisions. Is there any way to access a PowerShell-parser in this extension? Or execute commands in the debugger/PSES session? There's no relevant external APIs provided by vscode-powershell to hook into and AFAIK there's no support for any "evaluate/execute" command we can pass to the debugger (or is it?). My current ideas are:

  • Add PSES? It feels unnecessary complicated and something we should be able to borrow
  • Calling PowerShell sub-processes which is way to heavy and slow

As I'm not experienced in neither JS/TS or vscode extension development I might be missing something here, so please help 🙂

If not, then I'm wondering if it's time to consider moving this feature inside the official vscode-powershell extension where all of this might be easier to implement. I believe this awesome extension has gotten enough popularity and showed it's usefulness to be included as a first-class feature for PowerShell in VSCode just like it is in the Java-extensions.

@TylerLeonhardt
Copy link
Owner

@fflaten tl;dr: Yes this should use PSES. However, I don't think it should use PSES but instead an API provided by vscode-powershell to give you important details like:

  1. AST
  2. Tokens
  3. Variable state (via Get-Variable)
    etc.

or like you said, it becomes a part of the vscode-powershell extension.

Both of these require some work from @andschwa or @rjmholt on the vscode-powershell side to enable us to do this. We could contribute it to vscode-powershell, but right now the current focus of the PowerShell team is on performance of PSES and I don't think adding more complexity to it is what we wanna do right this second.

I think this extension can continue being regex based for the time being and in the future we can work with the PowerShell team on a solution that makes sense.

With all that said, if you wanted to contribute:

  • Just array indexing
  • Just property accessing (not methods)

I think there's a ton of value there since the vscode-powershell stuff won't happen for a while most likely.

@fflaten
Copy link
Contributor Author

fflaten commented Jun 11, 2021

@fflaten tl;dr: Yes this should use PSES. However, I don't think it should use PSES but instead an API provided by vscode-powershell

Couldn't agree more. I do support prioritizing PSES performance and stability in vscode-powershell, so I guess we'll have to be patient about getting this integrated or get access to a suitable external API at some point. Hopefully it's not years away 🙂

Will look into finishing this PR with regex when I get some time.

@fflaten
Copy link
Contributor Author

fflaten commented Jun 7, 2022

On hold until #10 is merged or dismissed + possible PR for #8 (draft incoming soon) due to lots of conflicts.

This PR could also solve #12 if decided to implement .
If so, should env + alias be affected by the configuration setting? Have their own?

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.

Feature request: Show properties for object
2 participants