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

Unable to format document in special circumstances #1994

Open
6 tasks done
PrzemyslawKlys opened this issue Apr 10, 2024 · 7 comments · May be fixed by #1995
Open
6 tasks done

Unable to format document in special circumstances #1994

PrzemyslawKlys opened this issue Apr 10, 2024 · 7 comments · May be fixed by #1995

Comments

@PrzemyslawKlys
Copy link
Contributor

Prerequisites

  • I have written a descriptive issue title.
  • I have searched all open and closed issues to ensure it has not already been reported.
  • I have read the troubleshooting guide.
  • I am sure this issue is with the extension itself and does not reproduce in a standalone PowerShell instance.
  • I have verified that I am using the latest version of Visual Studio Code and the PowerShell extension.
  • If this is a security issue, I have read the security issue reporting guidance.

Summary

Extension works perfectly most of the time in recent months. Today I've noticed weird issue

Where a simple code like this format mostly everything with the exception of lines with Get-Item

    $Files = Get-ChildItem -Path $Directory -File -Recurse -Include "*$ExtensionFrom"
    foreach ($File in $Files) {
        if ($File.Extension -eq $ExtensionFrom) {
            if ($OnlyNewerThan -and $File.LastWriteTime -lt (Get-Date).AddDays(-$OnlyNewerThan)) {
                #Write-Color -Text '[i]', "[TheDashboard] ", "Skipping $($File.FullName) as it's older than $OnlyNewerThan days" -Color Yellow
                continue
            }
            Write-Color -Text '[i]', "[TheDashboard] ", "Processing fixes $($File.FullName) / $($File.LastWriteTime)" -Color Yellow
            # Store original dates
            $originalCreationTime = $File.CreationTime
            $originalLastWriteTime = $File.LastWriteTime

            $Encoding = Get-FileEncoding -Path $File.FullName
            $FileContent = Get-Content -Raw -Path $File.FullName -Encoding $Encoding
            if ($FileContent -match $SearchString) {
                Write-Color -Text '[i]', "[TheDashboard] ", "Processing fixes $($File.FullName) for ($SearchString)" -Color Green
                $FileContent -replace $SearchString, $ReplaceString | Set-Content -Path $File.FullName -Encoding $Encoding

                # Restore original dates
                (Get-Item $File.FullName).CreationTime = $originalCreationTime
              (Get-Item $File.FullName).LastWriteTime = $originalLastWriteTime
            }
        }
    }

image

Trying to format it, leaves it as is, at the same time it formats everything else around it. So it seems there's something special about it.

PowerShell Version

Name                           Value
----                           -----
PSVersion                      7.4.1
PSEdition                      Core
GitCommitId                    7.4.1
OS                             Microsoft Windows 10.0.22631
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0.}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

Visual Studio Code Version

1.89.0-insider
x64

Extension Version

ms-vscode.powershell@2024.3.2

Steps to Reproduce

  1. Add provided code to code
  2. Make sure lines with Get-Item are not formatted
  3. Try to format them
  4. No formatting applies

Visuals

No response

Logs

No response

@ninmonkey
Copy link

ninmonkey commented Apr 10, 2024

I've tried a bunch of variations. You can reproduce it with almost any expression that starts with a (

You can reproduce it with this

    ($x = 10)
    $y = 30

formats as

    ($x = 10)
$y = 30

contrary, working case

     1, ($x = 10)

formats correctly

1, ($x = 10)

Another fail case

    ($x = 10), 1

# formats as the same
    ($x = 10), 1

@andyleejordan
Copy link
Member

Oh weird, thanks for the report. Did you try it under PSSA directly too?

@PrzemyslawKlys
Copy link
Contributor Author

No I didn't, sorry.

@andyleejordan
Copy link
Member

The latest version of the extension is using PSSA 1.22, which just came out. It would be helpful to know if this repros directly with Invoke-Formatter and if so we can move the bug over there and get it fixed.

@PrzemyslawKlys
Copy link
Contributor Author

image

I would say it's PSScriptAnalyzer issue. Will you move it?

@andyleejordan andyleejordan transferred this issue from PowerShell/vscode-powershell Apr 10, 2024
@andyleejordan
Copy link
Member

Thanks so much @PrzemyslawKlys!

@liamjpeters
Copy link
Contributor

liamjpeters commented Apr 12, 2024

Stepping through Formatter.Format() this behaviour arises from the PSUseConsistentIndentation rule.

Specifically from:

case TokenKind.LParen:
// When a line starts with a parenthesis and it is not the last non-comment token of that line,
// then indentation does not need to be increased.
if ((tokenIndex == 0 || tokens[tokenIndex - 1].Kind == TokenKind.NewLine) &&
NextTokenIgnoringComments(tokens, tokenIndex)?.Kind != TokenKind.NewLine)
{
onNewLine = false;
lParenSkippedIndentation.Push(true);
break;
}
lParenSkippedIndentation.Push(false);
AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine);
break;

The issue seems to be that, if the left paren ( is:

  • the first token (first non-whitespace character in the file)
    OR
  • the first non-whitespace character on a line

AND

  • isn't followed by a new line or comment

THEN that line's indentation is not checked against what we expect it to be. (Other necessary things still happen however)

Coming at it for the first time, I find the code-flow of the rule to be difficult to follow. The actual evaluation of whether the indentation is correct happens within the AddViolation() function, and only if onNewLine is true. It reads as though at each token parse, we're adding a rule violation - when in reality, checks are being performed and conditionally a violation is being added.

I believe, in the short-term, a change should be made so that in the situation outlined above, we still check the lines indentation is at the level we expect (but we don't increment the indentation level). Longer-term perhaps this rule should be revisited to make it a bit easier to follow and update it's styling slightly.

@bergmeister - this looks to be a rule you've put a lot of time and effort into, and you put together the PR that introduced this LParen check (#1469) - What do you think?

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