PSScriptAnalyzer icon indicating copy to clipboard operation
PSScriptAnalyzer copied to clipboard

Unable to suppress PSAvoidLongLines warning in a method

Open floh96 opened this issue 4 years ago • 12 comments

I'm unable to suppress the PSAvoidLongLines warning in a method inside a class.

Steps to reproduce

Run Invoke-ScriptAnalyzer -Path . -Recurse while the following PSScriptAnalyzerSettings.psd1 file is present in the folder:

@{
  Severity     = @('Error', 'Warning', 'Information')
  ExcludeRules = @('PSUseBOMForUnicodeEncodedFile',
    'PSAvoidUsingConvertToSecureStringWithPlainText',
    'PSAvoidUsingPlainTextForPassword')
  Rules        = @{
    PSAlignAssignmentStatement = @{
      Enable         = $true
      CheckHashtable = $true
    }
    PSPlaceCloseBrace          = @{
      Enable             = $true
      NoEmptyLineBefore  = $false
      IgnoreOneLineBlock = $true
      NewLineAfter       = $true
    }
    PSPlaceOpenBrace           = @{
      Enable             = $true
      OnSameLine         = $true
      NewLineAfter       = $true
      IgnoreOneLineBlock = $true
    }
    PSAvoidLongLines           = @{
      Enable            = $true
      MaximumLineLength = 80
    }
  }
}
Class foo {
  [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidLongLines", "")]
  [String]output () {
    $output = "a book is a book is a book. but this report on the activities of a company is"
    return $output
  }
}

Expected behavior

0 warnings

Actual behavior

RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
PSAvoidLongLines                    Warning      test.ps1   4     Line exceeds the configured maximum length of 80 characters
1 rule violations found.    Severity distribution:  Error = 0, Warning = 1, Information = 0

If an unexpected error was thrown then please report the full error details using e.g. $error[0] | Select-Object *

Environment data

> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      7.0.3
PSEdition                      Core
GitCommitId                    7.0.3
OS                             Darwin 17.7.0 Darwin Kernel Version 17.7.0: Wed May 27 17:00:02 PDT 2020; root:xnu-4570.71.80.1~1/RELEASE_X86_64
Platform                       Unix
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

> (Get-Module -ListAvailable PSScriptAnalyzer).Version | ForEach-Object { $_.ToString() }
1.19.0

See also #1586 for initial report since this issue was closed i opened a new one and provided clear repo steps.

floh96 avatar Oct 05 '20 13:10 floh96

Debugging this, the suppression is properly created but not applied by the suppression logic, which is here:

https://github.com/PowerShell/PSScriptAnalyzer/blob/09b69296753e6127d517b6c0f6a9035a1d53d11b/Engine/Helper.cs#L1357-L1391

It looks like the suppression is not applied because the offset is incorrect, which is because the AvoidLongLines rule creates its own ScriptPositions, which set offsets to 0:

https://github.com/PowerShell/PSScriptAnalyzer/blob/09b69296753e6127d517b6c0f6a9035a1d53d11b/Rules/AvoidLongLines.cs#L66-L78

To fix this, the AvoidLongLines rule needs to create an InternalScriptPosition using reflection, with a correctly calculated offset.

rjmholt avatar Oct 05 '20 18:10 rjmholt

cc @thomasrayner

bergmeister avatar Oct 25 '20 15:10 bergmeister

cc @thomasrayner

I definitely want to take this on but I might need a slightly larger push in the right direction. Any link to an example or doc should (hopefully) be enough.

thomasrayner avatar Oct 25 '20 16:10 thomasrayner

@thomasrayner We'll have to go to a level that is so low, the only docs is the PowerShell source code itself😅 What @rjmholt is saying is that the ScriptPosition class that this rules use to create an IScriptPosition has the its readonly offset property always return zero, which we can see in the code here:

https://github.com/PowerShell/PowerShell/blob/9b39f05557ef1c4fe51b14376a7c3abbcfaebff8/src/System.Management.Automation/engine/parser/Position.cs#L669-L672

However, there is also exist the InternalScriptPosition class that implements IScriptPosition, which is defined here:

https://github.com/PowerShell/PowerShell/blob/9b39f05557ef1c4fe51b14376a7c3abbcfaebff8/src/System.Management.Automation/engine/parser/Position.cs#L430

As we can see, that class is not publicly exposed, therefore using reflection (something like this) one is still able to create this class, which allows passing in the offset via its constructor here: https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/parser/Position.cs#L434 An alternative to reflection (which is expensive from a performance point of view) would of course be create our own implementation of InternalScriptPosition but given this special case, using reflection is OK and means at least less work/code to write write :-)

bergmeister avatar Oct 25 '20 18:10 bergmeister

@bergmeister thank you for a great breakdown! If you folks don't mind this fix taking a little while (fitting it into my spare time), please assign this to me. I'd love to take it on.

thomasrayner avatar Oct 25 '20 18:10 thomasrayner

That would be great but there is also no pressure, PSSA release cycles are quite long anyway and I don't think this issue is that much of a problem. My explanation only covers the Position part, you probably have to do some experiment to find what the right offset would be. The complex suppression logic that Rob linked above was created by someone else many years above and I luckily never had to touch it or look at it but looking at Rob's comment, you might be able to just ignore it and assume it does its job correctly so that it would just be the case of finding the right offset programmatically.

bergmeister avatar Oct 25 '20 18:10 bergmeister

Just FYI, you can also implement an IScriptPosition/IScriptExtent yourself, but then you must calculate the offset, which is error prone. I've written code that does it a few times though. Ideally this should be something that PowerShell fixes.

rjmholt avatar Mar 11 '21 18:03 rjmholt

Just FYI, you can also implement an IScriptPosition/IScriptExtent yourself, but then you must calculate the offset, which is error prone. I've written code that does it a few times though. Ideally this should be something that PowerShell fixes.

@rjmholt do you happen to have a reference for this that I can check out?

thomasrayner avatar Apr 27 '21 21:04 thomasrayner

Here you go.

We should really add that code to PowerShell itself with tests. Or perhaps to a NuGet package that we can pull in in different places.

rjmholt avatar Apr 27 '21 21:04 rjmholt

Considering that it's gonna be tough for me to prioritize working this issue into my work, do you think it'd be prudent for me to just wait for PS to add it?

thomasrayner avatar Apr 27 '21 21:04 thomasrayner

do you think it'd be prudent for me to just wait for PS to add it?

Well it's not going to be added to PS 3, 4, 5.1, 7.0 or 7.1 in any case, which PSSA also supports. But there's not exactly a plan to add it to 7.2 either — I'm just thinking out loud.

rjmholt avatar Apr 27 '21 22:04 rjmholt

I'll try and work out some time to poke at it, then. 😁

thomasrayner avatar Apr 28 '21 18:04 thomasrayner