unary operators not getting spaced

msftrncs commented 5 years ago

Following up from #847 / #948,

PSSA 1.18.0 Invoke-Formatter no longer inserts a space between letter/word based unary operators, such as -split or -not.

If I use the format selection command in VS Code I get a different result, but I am suspecting that VSCode / PowerShell Extension is still using PSSA 1.17.x.

I suspect this is from trying not to space out the unary 'negate' or 'positive' operators. Probably a simple idea might be to look at the length of the operator, length > 2 should be spaced.

bergmeister commented 5 years ago

Interesting, vscode is already using pssa 1.18.0, it ships a backup version of pssa in case it is not installed. I'll have a look at your cases, thanks for raising the issue. I am quite busy atm so it might take some time to go through all your recent issue comments, they are definitely very appreciated and valued!

rjmholt commented 5 years ago

but I am suspecting that VSCode / PowerShell Extension is still using PSSA 1.17.x

You can check your version in the integrated console with gmo PSScriptAnalyzer | % Version.

The latest extension ships with 1.18.0.

bergmeister commented 5 years ago

Sorry that I forgot to come back to this. Can you give specific examples using Invoke-Formatter for expected, versus actual result? This would make it easier for us but for small things like that seem to be rather features than bugs, I highly recommend trying to open a PR.

msftrncs commented 5 years ago

A couple examples

invoke-formatter {-not$a-1}
'-not$a - 1' # spaces added around '-' but not between '-not' and '$a'
# '-not $a - 1' would be the desired outcome
invoke-formatter {-not  $a  -  1} # double spaced
'-not  $a - 1' # spaces were trimmed up around '-' but not between '-not' and '$a'
# '-not $a - 1' would be the desired outcome
invoke-formatter {-split"hello"}
'-split"hello"' # no spacing between operator
# '-split "hello"' would be desired outcome
invoke-formatter {"hello"-split"`n"} # reference a binary operator of the same name
'"hello" -split "`n"' # is formatted as expected.

additional scenarios:

invoke-formatter {-  $a  -  1} # double spaced
'-  $a - 1' # spaces were trimmed around 'minus' but not between 'negate' and '$a'
# '-$a - 1' would be the expected outcome
invoke-formatter {++  $a  -  1} # double spaced
'++  $a - 1' # spaces were trimmed around 'minus' but not between 'increment' and '$a'
# '++$a - 1' would be the expected outcome
invoke-formatter {$a  ++} # double spaced
# no change occurs, '$a++' would be the expected outcome

msftrncs commented 5 years ago

With PSSA 1.18.1, -split unary operator is getting formatting now, but still not -not. Maybe because PS AST treats ! and -not the same? However, now - (negate) unary operator is getting space added after it again.

Also, I find sometimes VS Code format selection fails partially.

In this case I have

hello |
    hello some more tasks | hello some more
some more tasks |
    maybe more

    -split$a

This is at the end of a file, the file is dirty, and I selected this text right up to the end of the file. There is no blank line past the -split$a and if I request format selection the last line will not format. Add a blank line past it, or remove the spaces in front of that line, and it formats it. It seems to be something specific about the last line of text changing. It will cause the entire format to fail. Remove all the leading spaces from all the lines (except the last 1) and it still fails. ~Seems its only if that last line will get shorter, and that last line is the also the last line in the file.~ Correction, not related entirely to the last line in the file, it has to do with the selection ending on the end of the line that needs to get shorter.

msftrncs commented 5 years ago

I see I am getting different results between different methods … which explains why my results in VS Code are different.

If I use invoke-formatter { -split"hello" } I get a different result than when I use invoke-formatter { invoke-formatter { -split"hello" } }.

daviesj commented 3 years ago

I have run into this issue as well in version 1.19.1. Actually as msftrncs discovered, there are really multiple issues related to whitespace around unary operators.

Problem 1: Unary operators such as `-split`, `-join`, and `-not` should require a space between operator and operand.

Example:

PS> Invoke-Formatter {-split"string"}

Result:

-split"string"

Expected:

-split "string"

Problem 2: Unary operators (at least `-join` and `-split`) are detected as binary in some contexts.

This can cause the formatter to add an unnecessary space on one side. This is also the reason that some of msftrncs' examples did get spacing added around an unary operator.

Example:

PS> Invoke-Formatter {ConvertFrom-Json (-join (dotnet gitversion))}

Result:

ConvertFrom-Json ( -join (dotnet gitversion))

Expected:

ConvertFrom-Json (-join (dotnet gitversion))

Using Invoke-ScriptAnalyzer shows what is going on.

PS> Invoke-ScriptAnalyzer -Settings CodeFormatting -Scriptdefinition 'ConvertFrom-Json (-join (dotnet gitversion))' | select RuleName, Extent, Message

RuleName                  Extent Message
--------                  ------ -------
PSUseConsistentWhitespace -join  Use space before and after binary and assignment operators.

Problem 3: Extra spacing around unary operators is not trimmed like it is around binary ones.

Example 1 (Unary operators like -split, -join, and -not should have a single space):

 Invoke-Formatter { -join    @("The", " ", "dog") }

Result:

-join    @("The", " ", "dog")

Expected:

-join @("The", " ", "dog")

Example 2 (Unary operators like -, --, or ++ should have no space):

Invoke-Formatter { $counter ++ }

Result:

$counter ++

Expected:

$counter++

After a glance at the code for UseConsistentWhitespace, it appears that the rule purposefully ignores unary operators altogether (except when they are mistaken for a binary operator - see problem 2 above). I might be willing to work on a PR to fix at least some of this if it would be welcome.

Also, my instinct would be that the decision on whether there should be a space around an unary operator should be made based on whether the operator token contains letters instead of on the length of the token.

nikgibbens commented 3 years ago

@bergmeister , is it possible to get traction on this from the core team? This issues is well over a year old.

rjmholt commented 3 years ago

I believe @bergmeister is fairly busy in other areas at the moment (he gives his time freely to the PSScriptAnalyzer project), and the PowerShell team members who have PSScriptAnalyzer in their portfolio are currently spread thin across various other projects. PSScriptAnalyzer is always accepting PRs though.

bergmeister commented 3 years ago

Thanks for the reminder around this issue. Age of an issue does not matter as much as urgency versus priority matters, i.e. if something is a bug or a missing feature and what the impact is. In this case, most cases are mainly a missing feature and there are a couple of other things as well where PSSA formatter does not help with spaces such as e.g. if ( $tooMuchSpaceInFrontOfMe) {}. However, having said that, most proposals in this issue are reasonable and doable and fit into the past work, in 1.18 we added e.g. the capability to remove or add space around pipes and multi-pipe indentation and refined both in subsequent versions but both seem to have matured now, so adding more things would make sense now. What I see as relatively easy to do is (in order of priority and easiness)

Where there is no space after a unary operator like -not, insert one
Where there is more than one space after a unary operator like -not, remove it. When we did the Checkpipe setting we learned form users that some like alignment of preceding or following variables around operators across multiple lines (especially common with multiple Pester assertions), therefore this might be a non-default option
Where there is one or more spaces after a unary operator like ++ then remove the extraneous space. Some users might not like it so this should be optional. Some might even want an option where one specifies the amount of wanted whitespace, but that would be more complicated and is likely not desired for the first MVP version
The cases where an additional unwanted space is inserted after a negation - or before a binary operator as in the given example ConvertFrom-Json ( -join (dotnet gitversion)) is likely more complicated but I can understand is more annoying and could be considered as a bug to some. I will therefore give it a try and see if there is a quick and easy fix for it.

bergmeister commented 3 years ago

I opened PR #1602, which adds simple functionality by re-using an existing rule logic for binary and assignment operators that now allows the following scenarios to work for unary operators starting with a dash -:

$a-join$b  -->  $a -join $b

$a    -join     $b  -->  $a -join $b

-join$a  -->  -join $a

How would you feel about closing this issue after that PR but open 1 new issue for the cases where an additional unwanted space is inserted after a negation - or before a binary operator as in the given example ConvertFrom-Json ( -join (dotnet gitversion)). Also is the case with the ++/-- operator really that important? I struggle to understand why one would start writing code like $a ++ and expect PSSA to fix that, which is much less likely comapred to the case of $a-join$b where spaces were just omitted out of laziness in typing (and PowerShell not needing whitespaces)

daviesj commented 3 years ago

I am going to start by saying after looking at this issue for a while I think it would be worth changing the implementation of CheckOperator more significantly but I will open a separate issue for that. I ran into a mess of weeds trying to implement a fix for this myself. I have just been trying to figure out what to call the issue and where to start with it.

I suppose, to in order to make this issue just about one thing, I personally would consider it solved if CheckOperator enforces a single space between the operator & operand for all unary operators that contain word characters (such as -not, -split, etc.).

When we did the Checkpipe setting we learned form users that some like alignment of preceding or following variables around operators across multiple lines (especially common with multiple Pester assertions), therefore this might be a non-default option

I can't conjure up examples of this in my head very well. I wouldn't think it would affect unary operators as much as binary ones (since there is only one operand and it is at usually on the right side) but I realize the code I look at doesn't cover all scenarios. If necessary, I suppose it could be a separate option to fix whitespace around unary operators. However, the current behavior of CheckOperator adds whitespace around some unary operators, but not all unary operators, and not all the time.

I think that inserting an extra space on the wrong side of a unary operator is also a bug, but I suppose that could be a separate issue if necessary. I agree that enforcing no space around prefix/postfix probably should be a separate issue and probably optional.

PowerShell / PSScriptAnalyzer