Skip to content

Unable to format document in special circumstances #1994

Closed

Description

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

Activity

  1. ninmonkey commented on Apr 10, 2024

    @ninmonkey

    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
  2. andyleejordan commented on Apr 10, 2024

    @andyleejordan
    Member

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

  3. PrzemyslawKlys commented on Apr 10, 2024

    @PrzemyslawKlys
    ContributorAuthor

    No I didn't, sorry.

  4. andyleejordan commented on Apr 10, 2024

    @andyleejordan
    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.

  5. PrzemyslawKlys commented on Apr 10, 2024

    @PrzemyslawKlys
    ContributorAuthor

    image

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

  6. andyleejordan commented on Apr 10, 2024

    @andyleejordan
    Member
  7. liamjpeters commented on Apr 12, 2024

    @liamjpeters
    Contributor

    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.

    Christoph Bergmeister (@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

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions