Make switch statement report correct error position when it fails to evaluate the condition by daxian-dbw · Pull Request #7151 · PowerShell/PowerShell

daxian-dbw · 2018-06-22T22:47:17Z

PR Summary

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

PR Checklist

BrucePay · 2018-06-22T23:06:43Z

test/powershell/Language/Scripting/ErrorPosition.Tests.ps1

+    }
+
+    It "swtich condition MoveNext failure should report correct error position" {
+        $code = @'


typo swtich -> switch

BrucePay · 2018-06-22T23:08:10Z

test/powershell/Language/Scripting/ErrorPosition.Tests.ps1

+        Set-Content -Path $testFile -Encoding Ascii -Value @'
+$test = 1
+switch ($nullVar[0]) {
+    "a" {};


could use $null here switch ($null[0]) {

Fixed. Thanks

BrucePay · 2018-06-22T23:11:22Z

src/System.Management.Automation/engine/parser/Compiler.cs

-            {
-                exprs.Add(UpdatePosition(stmt.Condition));
-            }
+            exprs.Add(UpdatePosition(stmt.Condition));


So simple 👍

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

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.

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

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

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.

Don't add me to CI list 😄

SteveL-MSFT · 2018-06-22T23:09:40Z

test/powershell/Language/Scripting/ErrorPosition.Tests.ps1

+        $errorRecord.ScriptStackTrace | Should -Match "SwitchError1.ps1: line 2"
+    }
+
+    It "swtich condition MoveNext failure should report correct error position" {


typo: swtich

rjmholt

@BrucePay and @SteveL-MSFT seem to have addressed all concerns 👍

iSazonov · 2018-06-24T17:42:26Z

test/powershell/Language/Scripting/ErrorPosition.Tests.ps1

@@ -0,0 +1,51 @@
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# Licensed under the MIT License.
+


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

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.

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.

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.

…ails to evaluate the condition (#7151)" This reverts commit c77d0ce.

daxian-dbw added 2 commits June 21, 2018 18:13

Fix error position for switch

c0abba5

[Feature] Add tests

62bf97f

daxian-dbw requested review from BrucePay and rjmholt June 22, 2018 22:47

BrucePay approved these changes Jun 22, 2018

View reviewed changes

SteveL-MSFT requested changes Jun 22, 2018

View reviewed changes

rjmholt approved these changes Jun 22, 2018

View reviewed changes

iSazonov reviewed Jun 24, 2018

View reviewed changes

SteveL-MSFT approved these changes Jun 25, 2018

View reviewed changes

Address comments

1c7d9cc

daxian-dbw merged commit c77d0ce into PowerShell:master Jun 25, 2018

daxian-dbw deleted the error-position branch June 25, 2018 21:07

daxian-dbw added a commit that referenced this pull request Jun 26, 2018

Revert "Make switch statement report correct error position when it f…

384030b

…ails to evaluate the condition (#7151)" This reverts commit c77d0ce.

		@@ -0,0 +1,51 @@
		# Copyright (c) Microsoft Corporation. All rights reserved.
		# Licensed under the MIT License.

Conversation

daxian-dbw commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov Jun 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

daxian-dbw commented Jun 22, 2018 •

edited

Loading

rjmholt Jun 22, 2018 •

edited

Loading

iSazonov Jun 24, 2018 •

edited

Loading

daxian-dbw Jun 25, 2018 •

edited

Loading