Skip to content

Make switch statement report correct error position when it fails to evaluate the condition #7151

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.

Sign up for GitHub

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

Merged
daxian-dbw merged 3 commits into from
Jun 25, 2018
Merged

Make switch statement report correct error position when it fails to evaluate the condition #7151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c0abba5
Fix error position for switch
daxian-dbw Jun 22, 2018
62bf97f
[Feature] Add tests
daxian-dbw Jun 22, 2018
1c7d9cc
Address comments
daxian-dbw Jun 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions src/System.Management.Automation/engine/parser/Compiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4189,10 +4189,7 @@ private Expression GenerateIteratorStatement(VariablePath iteratorVariablePath,
// $foreach/$switch = GetEnumerator $enumerable
var enumerable = NewTemp(typeof(object), "enumerable");
temps.Add(enumerable);
if (generatingForeach)
{
exprs.Add(UpdatePosition(stmt.Condition));
}
exprs.Add(UpdatePosition(stmt.Condition));
Copy link
Collaborator

@BrucePay BrucePay Jun 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So simple 👍

Copy link
Collaborator

@rjmholt rjmholt Jun 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daxian-dbw what was the bug here btw? (As in why does this change fix it?)

Copy link
Member Author

@daxian-dbw daxian-dbw Jun 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generate code to update position in the script that is executing at various places, so when an error happens, the error can be associated with a position in the script. Here we didn't update the position for the condition when evaluating the condition from a switch statement.

Copy link
Collaborator

@rjmholt rjmholt Jun 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that the previous code went out of its way to only work for foreach loops

Copy link
Collaborator

@iSazonov iSazonov Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I wonder why it was so? If we didn't have tests for that previously didn't we break something now?

Copy link
Member Author

@daxian-dbw daxian-dbw Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I did break something 😄
When stepping over the script in debugging, now the debugger stops on switch ($b) twice. I will revert this PR.

Copy link
Collaborator

@iSazonov iSazonov Jun 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't add me to CI list 😄

exprs.Add(
Expression.Assign(enumerable,
GetRangeEnumerator(stmt.Condition.GetPureExpression())
Expand Down
51 changes: 51 additions & 0 deletions test/powershell/Language/Scripting/ErrorPosition.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License.

Copy link
Collaborator

@iSazonov iSazonov Jun 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need new file?
It seems we can use existing.

Update: I see we have tests only for parser. Closest to this https://github.com/PowerShell/PowerShell/blob/master/test/powershell/Language/Parser/Parser.Tests.ps1#L705

Copy link
Member Author

@daxian-dbw daxian-dbw Jun 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used a new file because I couldn't find an existing test file that this category of issues can fall in.
Here we are not testing indexing into a null array, but the scenario where it fails to evaluate the switch statement's condition (it could fail for a different reason, and I just happen to use $null[0]).

As for the test you referenced, it looks more fit in Language/Scripting instead of Language/Parser because the error that's tested there is a runtime error instead of a parsing one.

Copy link
Collaborator

@iSazonov iSazonov Jun 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I don't found a file with runtime error tests too. And I wonder. If we need better code coverage perhaps we should rename the file from ErrorPosition to RuntimeErrors.

Copy link
Member Author

@daxian-dbw daxian-dbw Jun 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Runtime errors usually are tested along with the feature that they are associated with, no need for a separate test file only for validating runtime errors.
Error position is a feature used for reporting errors, so I think it's better to have a test file of its own category.

Describe "Error position Tests" -Tags "CI" {
It "switch condition evaluation failure should report correct error position" {
$testFile = Join-Path $TestDrive "SwitchError1.ps1"
Set-Content -Path $testFile -Encoding Ascii -Value @'
$test = 1
switch ($null[0]) {
"a" {};
}
'@
try { & $testFile } catch { $errorRecord = $_ }
$errorRecord | Should -Not -BeNullOrEmpty
$errorRecord.ScriptStackTrace | Should -Match "SwitchError1.ps1: line 2"
}

It "switch condition MoveNext failure should report correct error position" {
$code = @'
using System;
using System.Collections.Generic;
namespace SwitchTest
{
public class Test
{
public static IEnumerable<string> GetName()
{
yield return "Hello world";
throw new ArgumentException();
}
}
}
'@
$testFile = Join-Path $TestDrive "SwitchError2.ps1"
Set-Content -Path $testFile -Encoding Ascii -Value @'
$test = 1
$enumerable = [SwitchTest.Test]::GetName()
switch ($enumerable) {
"hello world" { $test = 1; $_ }
"Yay" { $test = 2; $_ }
}
'@
if (-not ("SwitchTest.Test" -as [type])) {
Add-Type -TypeDefinition $code
}

try { & $testFile > $null } catch { $errorRecord = $_ }
$errorRecord | Should -Not -BeNullOrEmpty
$errorRecord.ScriptStackTrace | Should -Match "SwitchError2.ps1: line 3"
}
}