Fix bad pssa settings path crashes PSES #588
Conversation
| logger.WriteException( | ||
| $"Invalid Script Analyzer settings path - '{settingsPath}'.", | ||
| ex); | ||
| } | ||
|
|
||
| this.SettingsPath = settingsPath; |
There was a problem hiding this comment.
Do we still want to set this.SettingsPath to settingsPath even if settingsPath is an Invalid Script Analyzer settings path?
There was a problem hiding this comment.
Doh! No. Thanks for catching this. I'll fix it later tonight.
There was a problem hiding this comment.
Looks good. The only optional suggestion I would make would be to try to make the try/catch block smaller.
The comment in the catch block says that GetFullPath() throws so I wonder if we could just wrap that line in a try/catch:
try {
settingsPath = Path.GetFullPath(Path.Combine(workspaceRootPath, settingsPath));
} catch (Exception ex) when (ex is NotSupportedException) {
logger.WriteException( $"Invalid Script Analyzer settings path - '{settingsPath}'.", ex);
settingsPath = null;
}
A smaller try/catch helps to understand what is actually throwing the errors we are catching IMO.
While that is true, my concern is that any future changes to this method skip providing such error recovery. By having an encompassing try/catch we "handle" PSSA settings path errors by logging an error instead of crashing the lang server. The line we have to walk is to not hide "developer" errors which is why I don't like a naked FWIW I fixed a number of these bugs lately (particularly PathTooLongExc) where either a user supplied setting value OR an environmental factor like a path that is too long where the fix is to do just this - catch these exceptions (where we can't do anything useful but shouldn't crash - if possible). |
|
Fair point! I agree with you, then. I sign off. 👍 |
* Fix PSES crash when pssa settings path has invalid chars * Alway log (verbose) the pssa settings path * Set SettingsPath to null in catch handler * GetFullPath can also throw PathTooLongExc and SecurityExc
This change adds a new setting that controls whether focus changes to the integrated console when scripts are executed or debugged: - `powershell.integratedConsole.focusConsoleOnExecute` When true, it causes the focus to get set to the console on any script execution. It is set to true by default to ensure that screen readers pick up the execution output once it starts. Resolves PowerShell#588.
Crash happens if user tries to set path to something like
${env:HOME}\pssettings.psd1which looks like it might be valid but isn't because the variable doesn't get expanded.It seems the GH diff view has gotten dumber or something. This is the diff I see in VS: