From c97fd6b889fb0b0eb0d0e3c4f6ddbd3122d3da32 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Fri, 1 Mar 2019 10:16:40 -0800 Subject: [PATCH 1/6] Fix settings file array parsing when no commas are present --- Engine/Settings.cs | 37 ++++++++++++++++++++------------- Tests/Engine/Settings.tests.ps1 | 4 ++++ 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/Engine/Settings.cs b/Engine/Settings.cs index 326f67118..c768a8cf7 100644 --- a/Engine/Settings.cs +++ b/Engine/Settings.cs @@ -540,24 +540,33 @@ private static object GetSafeValueFromExpressionAst(ExpressionAst exprAst) return new object[0]; } - PipelineAst pipelineAst = arrExprAst.SubExpression.Statements[0] as PipelineAst; - if (pipelineAst == null) + var listComponents = new List(); + foreach (StatementAst statement in arrExprAst.SubExpression.Statements) { - throw CreateInvalidDataExceptionFromAst(arrExprAst); - } + var pipelineAst = statement as PipelineAst; + if (pipelineAst == null) + { + throw CreateInvalidDataExceptionFromAst(arrExprAst); + } - ExpressionAst pipelineExpressionAst = pipelineAst.GetPureExpression(); - if (pipelineExpressionAst == null) - { - throw CreateInvalidDataExceptionFromAst(arrExprAst); + var pipelineExpressionAst = pipelineAst.GetPureExpression(); + if (pipelineExpressionAst == null) + { + throw CreateInvalidDataExceptionFromAst(arrExprAst); + } + + object arrayValue = GetSafeValueFromExpressionAst(pipelineExpressionAst); + // We might hit arrays like @(\n1,2,3\n4,5,6), which the parser sees as two statements containing array expressions + if (arrayValue is object[] subArray) + { + listComponents.AddRange(subArray); + continue; + } + + listComponents.Add(arrayValue); } + return listComponents.ToArray(); - // Array expressions may not really be arrays (like `@('a')`, which has no ArrayLiteralAst within) - // However, some rules depend on this always being an array - object arrayValue = GetSafeValueFromExpressionAst(pipelineExpressionAst); - return arrayValue.GetType().IsArray - ? arrayValue - : new object[] { arrayValue }; case ArrayLiteralAst arrLiteralAst: return GetSafeValuesFromArrayAst(arrLiteralAst); diff --git a/Tests/Engine/Settings.tests.ps1 b/Tests/Engine/Settings.tests.ps1 index ddd4c8156..227c83102 100644 --- a/Tests/Engine/Settings.tests.ps1 +++ b/Tests/Engine/Settings.tests.ps1 @@ -218,6 +218,10 @@ Describe "Settings Class" { @{ Expr = '@("A","B","C")'; Count = 3 } @{ Expr = '@()'; Count = 0 } @{ Expr = '@(7)'; Count = 1 } + @{ Expr = "'1',`n'2',`n'3'"; Count = 3 } + @{ Expr = "@(1`n3`n5`n7)"; Count = 4 } + @{ Expr = "'1',`r`n'2',`r`n'3'"; Count = 3 } + @{ Expr = "@(1`r`n3`r`n5`r`n7)"; Count = 4 } ) $gsvHashtableTests = @( From 51b5fe5c197b9531f49dfaac18c27c91b958a80f Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Fri, 1 Mar 2019 10:18:47 -0800 Subject: [PATCH 2/6] Add extra test --- Tests/Engine/Settings.tests.ps1 | 1 + 1 file changed, 1 insertion(+) diff --git a/Tests/Engine/Settings.tests.ps1 b/Tests/Engine/Settings.tests.ps1 index 227c83102..b7f792757 100644 --- a/Tests/Engine/Settings.tests.ps1 +++ b/Tests/Engine/Settings.tests.ps1 @@ -222,6 +222,7 @@ Describe "Settings Class" { @{ Expr = "@(1`n3`n5`n7)"; Count = 4 } @{ Expr = "'1',`r`n'2',`r`n'3'"; Count = 3 } @{ Expr = "@(1`r`n3`r`n5`r`n7)"; Count = 4 } + @{ Expr = "@(1,2,3`n7,8,9)"; Count = 6 } ) $gsvHashtableTests = @( From f60e129a46749dcbb38c7ac76404ab487d870408 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Fri, 1 Mar 2019 10:22:52 -0800 Subject: [PATCH 3/6] Improve comment --- Engine/Settings.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Engine/Settings.cs b/Engine/Settings.cs index c768a8cf7..57457d5ee 100644 --- a/Engine/Settings.cs +++ b/Engine/Settings.cs @@ -541,6 +541,10 @@ private static object GetSafeValueFromExpressionAst(ExpressionAst exprAst) } var listComponents = new List(); + // Arrays can either be array expressions (1, 2, 3) or array literals with statements @(1 `n 2 `n 3) + // Or they can be a combination of these + // We go through each statement (line) in an array and read the whole subarray + // This will also mean that @(1; 2) is parsed as an array of two elements, but there's not much point defending against this foreach (StatementAst statement in arrExprAst.SubExpression.Statements) { var pipelineAst = statement as PipelineAst; From 5047f87a581f252931c6e6d69dde9000bc0671d6 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Fri, 1 Mar 2019 13:08:28 -0800 Subject: [PATCH 4/6] Add test file --- Engine/Settings.cs | 12 +- Tests/Rules/AllCompatibilityRules.Tests.ps1 | 133 ++++++++++++++++++++ 2 files changed, 140 insertions(+), 5 deletions(-) create mode 100644 Tests/Rules/AllCompatibilityRules.Tests.ps1 diff --git a/Engine/Settings.cs b/Engine/Settings.cs index 57457d5ee..565952c8d 100644 --- a/Engine/Settings.cs +++ b/Engine/Settings.cs @@ -522,6 +522,9 @@ private static object GetSafeValueFromExpressionAst(ExpressionAst exprAst) case "false": return false; + case "null": + return null; + default: throw CreateInvalidDataExceptionFromAst(varExprAst); } @@ -547,13 +550,12 @@ private static object GetSafeValueFromExpressionAst(ExpressionAst exprAst) // This will also mean that @(1; 2) is parsed as an array of two elements, but there's not much point defending against this foreach (StatementAst statement in arrExprAst.SubExpression.Statements) { - var pipelineAst = statement as PipelineAst; - if (pipelineAst == null) + if (!(statement is PipelineAst pipelineAst)) { throw CreateInvalidDataExceptionFromAst(arrExprAst); } - var pipelineExpressionAst = pipelineAst.GetPureExpression(); + ExpressionAst pipelineExpressionAst = pipelineAst.GetPureExpression(); if (pipelineExpressionAst == null) { throw CreateInvalidDataExceptionFromAst(arrExprAst); @@ -660,10 +662,10 @@ private static InvalidDataException CreateInvalidDataExceptionFromAst(Ast ast) throw new ArgumentNullException(nameof(ast)); } - return ThrowInvalidDataException(ast.Extent); + return CreateInvalidDataException(ast.Extent); } - private static InvalidDataException ThrowInvalidDataException(IScriptExtent extent) + private static InvalidDataException CreateInvalidDataException(IScriptExtent extent) { return new InvalidDataException(string.Format( CultureInfo.CurrentCulture, diff --git a/Tests/Rules/AllCompatibilityRules.Tests.ps1 b/Tests/Rules/AllCompatibilityRules.Tests.ps1 new file mode 100644 index 000000000..ff2b7e54d --- /dev/null +++ b/Tests/Rules/AllCompatibilityRules.Tests.ps1 @@ -0,0 +1,133 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +$script:fileContent = @' +$assetDir = 'C:\ImportantFiles\' +$archiveDir = 'C:\Archived\' + +$runningOnWindows = -not ($IsLinux -or $IsMacOS) +$zipCompressionLevel = 'Optimal' +$includeBaseDirInZips = $true + +if (-not (Test-Path $archiveDir)) +{ + New-Item -Type Directory $archiveDir +} + +Import-Module -FullyQualifiedName @{ ModuleName = 'MyArchiveUtilities'; ModuleVersion = '2.0' } + +$sigs = [System.Collections.Generic.List[System.Management.Automation.Signature]]::new() +$filePattern = [System.Management.Automation.WildcardPattern]::Get('system*') +foreach ($file in Get-ChildItem $assetDir -Recurse -Depth 1) +{ + if (-not $filePattern.IsMatch($file.Name)) + { + continue + } + + if ($file.Name -like '*.dll') + { + $sig = Get-AuthenticodeSignature $file + $sigs.Add($sig) + continue + } + + if (Test-WithFunctionFromMyModule -File $file) + { + $destZip = Join-Path $archiveDir $file.BaseName + [System.IO.Compression.ZipFile]::CreateFromDirectory($file.FullName, "$destZip.zip", $zipCompressionLevel, $includeBaseDirInZips) + } +} + +Write-Output $sigs +'@ + +$script:settingsContent = @' +@{ + Rules = @{ + PSUseCompatibleCommands = @{ + Enable = $true + TargetProfiles = @( + 'win-8_x64_10.0.17763.0_5.1.17763.316_x64_4.0.30319.42000_framework' # Server 2019 - PS 5.1 (the platform it already runs on) + 'win-8_x64_6.2.9200.0_3.0_x64_4.0.30319.42000_framework' # Server 2012 - PS 3 + 'ubuntu_x64_18.04_6.1.3_x64_4.0.30319.42000_core' # Ubuntu 18.04 - PS 6.1 + ) + } + PSUseCompatibleTypes = @{ + Enable = $true + # Same as for command targets + TargetProfiles = @( + 'win-8_x64_10.0.17763.0_5.1.17763.316_x64_4.0.30319.42000_framework' + 'win-8_x64_6.2.9200.0_3.0_x64_4.0.30319.42000_framework' + 'ubuntu_x64_18.04_6.1.3_x64_4.0.30319.42000_core' + ) + } + PSUseCompatibleSyntax = @{ + Enable = $true + TargetVersions = @('3.0', '5.1', '6.1') + } + } +} +'@ + +$script:expectedCommandDiagnostics = @( + @{ Command = 'Import-Module'; Parameter = 'FullyQualifiedName'; Line = 13 } + @{ Command = 'Get-ChildItem'; Parameter = 'Depth'; Line = 17 } + @{ Command = 'Get-AuthenticodeSignature'; Line = 26 } +) + +$script:expectedTypeDiagnostics = @( + @{ Type = 'System.Management.Automation.WildcardPattern'; Member = 'Get'; Line = 16 } + @{ Type = 'System.IO.Compression.ZipFile'; Line = 34 } +) + +$script:expectedSyntaxDiagnostics = @( + @{ Line = 15; CorrectionText = "New-Object 'System.Collections.Generic.List[System.Management.Automation.Signature]'" } +) + +Describe "Running all compatibility rules with a settings file" { + BeforeAll { + $testFile = New-Item -Path "$TestDrive/test.ps1" -Value $script:fileContent + $settingsFile = New-Item -Path "$TestDrive/settings.psd1" -Value $script:settingsContent + $diagnostics = Invoke-ScriptAnalyzer -Path $testFile.FullName -Settings $settingsFile.FullName | + Group-Object -Property RuleName -AsHashtable + $commandDiagnostics = $diagnostics.PSUseCompatibleCommands + $typeDiagnostics = $diagnostics.PSUseCompatibleTypes + $syntaxDiagnostics = $diagnostics.PSUseCompatibleSyntax + } + + It "Finds the problem with command on line in the file" -TestCases $script:expectedCommandDiagnostics { + param([string]$Command, [string]$Parameter, [int]$Line) + + $actualDiagnostic = $commandDiagnostics | Where-Object { $_.Command -eq $Command -and $_.Line -eq $Line } + $actualDiagnostic.Command | Should -BeExactly $Command + $actualDiagnostic.Line | Should -Be $Line + if ($Parameter) + { + $actualDiagnostic.Parameter | Should -Be $Parameter + } + } + + It "Finds the problem with type on line in the file" -TestCases $script:expectedTypeDiagnostics { + param([string]$Type, [string]$Member, [int]$Line) + + $actualDiagnostic = $typeDiagnostics | Where-Object { $_.Type -eq $Type -and $_.Line -eq $Line } + $actualDiagnostic.Type | Should -BeExactly $Type + $actualDiagnostic.Line | Should -Be $Line + if ($Member) + { + $actualDiagnostic.Member | Should -Be $Member + } + } + + It "Finds the problem with syntax on line in the file" -TestCases $script:expectedSyntaxDiagnostics { + param([string]$Suggestion, [int]$Line) + + $actualDiagnostic = $syntaxDiagnostics | Where-Object { $_.Line -eq $Line } + $actualDiagnostic.Line | Should -Be $Line + if ($Suggestion) + { + $actualDiagnostic.Suggestion | Should -Be $Suggestion + } + } +} \ No newline at end of file From b513b373b3d8014646ba998a93177b9a8d6d6783 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Fri, 1 Mar 2019 13:37:27 -0800 Subject: [PATCH 5/6] Change test to accept null --- Tests/Engine/Settings.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/Settings.tests.ps1 b/Tests/Engine/Settings.tests.ps1 index b7f792757..4a5969cb1 100644 --- a/Tests/Engine/Settings.tests.ps1 +++ b/Tests/Engine/Settings.tests.ps1 @@ -209,6 +209,7 @@ Describe "Settings Class" { @{ Expr = '0.142' } @{ Expr = '"Hello"' } @{ Expr = '"Well then"' } + @{ Expr = '$null' } ) $gsvArrayTests = @( @@ -240,7 +241,6 @@ Describe "Settings Class" { $gsvThrowTests = @( @{ Expr = '$var' } @{ Expr = '' } - @{ Expr = '$null' } @{ Expr = '3+7' } @{ Expr = '- 2.5'} @{ Expr = '-not $true' } From 17f552e3b86e0fc80aa3f6a93bfe375381ef5f2e Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Fri, 1 Mar 2019 13:50:40 -0800 Subject: [PATCH 6/6] Fix new-item call in PS 4 --- Tests/Rules/AllCompatibilityRules.Tests.ps1 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/Rules/AllCompatibilityRules.Tests.ps1 b/Tests/Rules/AllCompatibilityRules.Tests.ps1 index ff2b7e54d..07f34420f 100644 --- a/Tests/Rules/AllCompatibilityRules.Tests.ps1 +++ b/Tests/Rules/AllCompatibilityRules.Tests.ps1 @@ -87,8 +87,8 @@ $script:expectedSyntaxDiagnostics = @( Describe "Running all compatibility rules with a settings file" { BeforeAll { - $testFile = New-Item -Path "$TestDrive/test.ps1" -Value $script:fileContent - $settingsFile = New-Item -Path "$TestDrive/settings.psd1" -Value $script:settingsContent + $testFile = New-Item -Path "$TestDrive/test.ps1" -Value $script:fileContent -ItemType File + $settingsFile = New-Item -Path "$TestDrive/settings.psd1" -Value $script:settingsContent -ItemType File $diagnostics = Invoke-ScriptAnalyzer -Path $testFile.FullName -Settings $settingsFile.FullName | Group-Object -Property RuleName -AsHashtable $commandDiagnostics = $diagnostics.PSUseCompatibleCommands