Fix debug tracing error with magic extents #25726
Conversation
Fixes the error when PowerShell attempts to print the debug trace line for functions generated with an invalid extent. For example PowerShell creates an extent with a column and line of 0 for things like the default class constructors. This is an issue as the trace debug line always subtracts by 1 to make the value start at 0 rather than 1. It also adds the DebuggerHidden attribute to the class default constructor to avoid confusing end users from seeing a trace line for something they did not write.
| { | ||
| Set-PSDebug -Trace 1 | ||
| [ClassWithDefaultCtor]::new() | ||
| } | Should -Not -Throw |
There was a problem hiding this comment.
Does the test really throw before the fix?
There was a problem hiding this comment.
Run it yourself and see :)
| if (_context.ShouldTraceStatement && | ||
| !_callStack.Last().IsFrameHidden && | ||
| !functionContext._debuggerStepThrough && | ||
| functionContext.CurrentPosition.StartScriptPosition.ColumnNumber > 0) |
There was a problem hiding this comment.
Both StartScriptPosition and InternalScriptPosition.ColumnNumber are calculated in the getter, so while tracing isn't exactly perf focused hopefully something like this will work:
| functionContext.CurrentPosition.StartScriptPosition.ColumnNumber > 0) | |
| functionContext.CurrentPosition is not EmptyScriptExtent && | |
| (functionContext.CurrentPosition is InternalScriptExtent || | |
| functionContext.CurrentPosition.StartColumnNumber > 0)) |
Thoughts?
There was a problem hiding this comment.
Just tested it locally and looks like it works for this scenario. I wasn't sure whether the fix should have been done here or just a check in the BriefMessage to just treat it as the start column or the change in this PR.
There was a problem hiding this comment.
I see EmptyScriptExtent is used only in scenario where an error is in script file. So it makes no sense to check the both types.
There was a problem hiding this comment.
I see EmptyScriptExtent is used only in scenario where an error is in script file. So it makes no sense to check the both types.
I think you may have followed the wrong reference, it's used in a ton of places including this one. It's typically accessed via PositionUtilities.EmptyExtent so maybe you were just looking for the type itself.
There was a problem hiding this comment.
Just tested it locally and looks like it works for this scenario. I wasn't sure whether the fix should have been done here or just a check in the
BriefMessageto just treat it as the start column or the change in this PR.
Yeah kind of a toss up, realistically any code that accesses IScriptExtent should be way more careful. It tends to assume we're talking about InternalScriptExtent but it could be external code.
Co-authored-by: Patrick Meinecke <SeeminglyScience@users.noreply.github.com>
|
/azp run PowerShell-CI-linux-packaging, PowerShell-Windows-Packaging-CI |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
Any chance for a backport with this? Seems like a simple fix to include. |
We don't typically backport unless it's a regression or a new high impact scenario is discovered. Is it a blocker for you in something? Note that I know it's a simple code change, but when doing a servicing release even an unexpected false positive on an analyzer could cost a day of release time. That can be especially impactful for the servicing releases that are security related. |
|
Not a blocker, just didn’t seem like a big change and would have been nice to see in earlier releases. |
PR Summary
Fixes the error when PowerShell attempts to print the debug trace line for functions generated with an invalid extent. For example PowerShell creates an extent with a column and line of 0 for things like the default class constructors. This is an issue as the trace debug line always subtracts by 1 to make the value start at 0 rather than 1.
It also adds the DebuggerHidden attribute to the class default constructor to avoid confusing end users from seeing a trace line for something they did not write.Edit: Removed this due to build issues.
PR Context
Fixes: #16874
I can remove the added attribute on the scriptblock and the fix will still be present but I thought it best to keep it there anyway. It doesn't hurt to be present and have that particular scenario skip the tracing altogether.Edit: Was removed due to build issues.
Also Pester code coverage and Profiler uses this tracing so it's commonly seen even if people don't enable the tracing themselves.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header