Fix error position for switch-statement condition and for-statement initializer #7305

daxian-dbw · 2018-07-18T01:03:29Z

PR Summary

Make switch-statement report correct error position when it fails to evaluate the condition.
Make for-statement report correct error position when it fails to evaluate the initializer.

Before:

PS> $test = 1; switch ($null[0]) { default {} }
Cannot index into a null array.
At line:1 char:1
+ $test = 1; switch ($null[0]) { default {} }
+ ~~~~~~~~~
+ CategoryInfo          : InvalidOperation: (:) [], RuntimeException
+ FullyQualifiedErrorId : NullArray

PS> $test = 1; for ($null[0];;) {}
Cannot index into a null array.
At line:1 char:1
+ $test = 1; for ($null[0];;) {}
+ ~~~~~~~~~
+ CategoryInfo          : InvalidOperation: (:) [], RuntimeException
+ FullyQualifiedErrorId : NullArray

After:

PS> $test = 1; switch ($null[0]) { default {} }
Cannot index into a null array.
At line:1 char:20
+ $test = 1; switch ($null[0]) { default {} }
+                    ~~~~~~~~
+ CategoryInfo          : InvalidOperation: (:) [], RuntimeException
+ FullyQualifiedErrorId : NullArray

PS> $test = 1; for ($null[0];;) {}
Cannot index into a null array.
At line:1 char:17
+ $test = 1; for ($null[0];;) {}
+                 ~~~~~~~~
+ CategoryInfo          : InvalidOperation: (:) [], RuntimeException
+ FullyQualifiedErrorId : NullArray

Break point check

For switch-statement condition, retain the same behavior as is:

Script Content

PS> cat .\about2.ps1
$Test = 1..2
switch ($Test)
{
    default {'a'}
}

PS> .\about2.ps1
Entering debug mode. Use h or ? for help.

Hit Line breakpoint on 'F:\tmp\about2.ps1:1'

At F:\tmp\about2.ps1:1 char:1
+ $Test = 1..2
+ ~~~~~~~~~~~~
PS>> v
At F:\tmp\about2.ps1:2 char:9
+ switch ($Test)
+         ~~~~~
PS>> v
At F:\tmp\about2.ps1:4 char:14
+     default {'a'}
+              ~~~
PS>> v
a
At F:\tmp\about2.ps1:2 char:9
+ switch ($Test)
+         ~~~~~
PS>> v
At F:\tmp\about2.ps1:4 char:14
+     default {'a'}
+              ~~~
PS>> v
a
At F:\tmp\about2.ps1:2 char:9
+ switch ($Test)
+         ~~~~~
PS>> v

For for-statement initializer, fix the incorrect breaking point position when the initializer is a simple command expression:

Script Content

PS> cat .\about2.ps1
$i = 1
for ('str'.Length;
     $i -gt 0; $i--)
{
    "v"
}

Before:

PS> .\about2.ps1
Hit Line breakpoint on 'F:\tmp\about2.ps1:1'

At F:\tmp\about2.ps1:1 char:1
+ $i = 1
+ ~~~~~~
PS>> v
At F:\tmp\about2.ps1:3 char:6
+      $i -gt 0; $i--)
+      ~~~~~~~~
PS>> v
At F:\tmp\about2.ps1:5 char:5
+     "v"
+     ~~~
PS>> v
v
At F:\tmp\about2.ps1:3 char:16
+      $i -gt 0; $i--)
+                ~~~~
PS:40>> v
At F:\tmp\about2.ps1:3 char:6
+      $i -gt 0; $i--)
+      ~~~~~~~~
PS>> v

After:

PS> .\about2.ps1
Hit Line breakpoint on 'F:\tmp\about2.ps1:1'

At F:\tmp\about2.ps1:1 char:1
+ $i = 1
+ ~~~~~~
PS>> v
At F:\tmp\about2.ps1:2 char:6
+ for ('str'.Length;
+      ~~~~~~~~~~~~
PS>> v
At F:\tmp\about2.ps1:3 char:6
+      $i -gt 0; $i--)
+      ~~~~~~~~
PS>> v
At F:\tmp\about2.ps1:5 char:5
+     "v"
+     ~~~
PS>> v
v
At F:\tmp\about2.ps1:3 char:16
+      $i -gt 0; $i--)
+                ~~~~
PS>> v
At F:\tmp\about2.ps1:3 char:6
+      $i -gt 0; $i--)
+      ~~~~~~~~
PS>> v

PR Checklist

…tatement initializer

daxian-dbw · 2018-07-18T01:04:56Z

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

        }

-        internal Expression UpdatePosition(Ast ast)
+        private int AddSequencePoint(IScriptExtent extent)


Note: this method is added to make sure that one extent can only be added to the sequence list once.

daxian-dbw · 2018-07-19T23:38:14Z

@BrucePay Can you please review this PR when you have time? Thanks!

SteveL-MSFT · 2018-07-21T14:51:01Z

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

+        $errorRecord.ScriptStackTrace | Should -Match "SwitchError2.ps1: line 3"
+    }
+
+    It "for statement initializer evaluation failure should report correct error position" {


There should also be a test for while/do loops

More tests added.

SteveL-MSFT · 2018-07-21T14:51:05Z

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

+
+$script3 = @'
+$test = 2
+for ("string".Length;


In the product code, there's a special check for while/do loops, but I don't see a test here for that?

More test added.

daxian-dbw · 2018-07-23T22:28:20Z

@SteveL-MSFT I corrected the condition sequence point update for while/do-while/do-until loops and if-statement as well. More tests are added. Please take another look when you have time. Thanks!

PaulHigin

LGTM

PaulHigin · 2018-07-24T16:57:54Z

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

            }
+            else
+            {
+                // For switch statement, we don't want the debugger to stop at 'stmt.Condition' before evaluating it.


From this comment it is not clear how the code prevents these two undesirable behaviors. Can you clarify?

The argument checkBreakpoints: false when calling UpdatePositionExpr means not checking breaking point for this sequence point update.

PaulHigin · 2018-07-24T17:24:16Z

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

-"line 1"
-"line 2"
-"line 3"
+            $path = Setup -PassThru -File BasicTest.ps1 -Content $script


Should we add debugger stop position tests as well as hit count (similar to error position tests)?

That sounds good. Can you please show an example of how to check for debugger stop position?

You can create a type (Add-Type) that has a method to handle the runspace.debugger.DebuggerStop event and then check the event argument InvocationInfo extent (I think). Contact me offline for more info.

OK. I will play with it, and add more tests in a separate PR. Thanks!

[Feature] Fix error position for switch-statement condition and for-s…

0b4bae4

…tatement initializer

daxian-dbw requested a review from BrucePay July 18, 2018 01:03

daxian-dbw commented Jul 18, 2018

View reviewed changes

daxian-dbw requested review from PaulHigin and SteveL-MSFT July 20, 2018 18:00

daxian-dbw self-assigned this Jul 21, 2018

daxian-dbw added WG-Interactive-Debugging debugging PowerShell script WG-Engine core PowerShell engine, interpreter, and runtime labels Jul 21, 2018

SteveL-MSFT requested changes Jul 21, 2018

View reviewed changes

[Feature] Correct sequence point update for while/do-while/do-until

755153e

SteveL-MSFT approved these changes Jul 24, 2018

View reviewed changes

PaulHigin approved these changes Jul 24, 2018

View reviewed changes

daxian-dbw merged commit 87d8fc9 into PowerShell:master Jul 25, 2018

daxian-dbw deleted the error branch July 25, 2018 16:56

scps-github mentioned this pull request Jul 9, 2021

InvocationInfo reports incorrect position of exception generated by a key or property #15742

Open

5 tasks

Fix error position for switch-statement condition and for-statement initializer #7305

Fix error position for switch-statement condition and for-statement initializer #7305

Uh oh!

Conversation

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

Uh oh!

PR Summary

Break point check

For switch-statement condition, retain the same behavior as is:

For for-statement initializer, fix the incorrect breaking point position when the initializer is a simple command expression:

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Jul 19, 2018

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!

daxian-dbw commented Jul 23, 2018

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Jul 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!

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!

daxian-dbw Jul 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

daxian-dbw commented Jul 18, 2018 •

edited

Loading

daxian-dbw Jul 24, 2018 •

edited

Loading

daxian-dbw Jul 24, 2018 •

edited

Loading