Fix several edge cases when the workspace folder is a filesystem root#1
Draft
vanklompf wants to merge 3 commits into
Draft
Fix several edge cases when the workspace folder is a filesystem root#1vanklompf wants to merge 3 commits into
vanklompf wants to merge 3 commits into
Conversation
…stem root The old code stripped the workspace's RootRealPath from a target filename by substring(RootRealPath.length), then prepended RootPath. When RootRealPath is a filesystem root (e.g. "/" or "C:\\"), the trailing separator that is part of that root is part of the prefix, so substring removes it and the resulting path is missing a separator (e.g. RootPath="/symlink", targetFileName="/foo.h" becomes "/symlinkfoo.h"). It also did a naive startsWith descendant check, which would falsely match unrelated siblings such as "/foo" vs "/foobar". Use path.relative + path.join so the descendant check and path reconstruction work correctly for any workspace root, including filesystem roots and on Windows drive roots. This mirrors the spirit of the fix in microsoft/vscode#317637. Co-authored-by: Franek Korta <vanklompf@users.noreply.github.com>
Several code paths in CppProperties built paths by manually concatenating
rootUri.fsPath with path.sep:
rootUri.fsPath + path.sep + something
When the workspace folder is a filesystem root such as "/" or "C:\\",
rootUri.fsPath already ends in (and is) a separator, so this concatenation
produces malformed paths like "//something" or "C:\\\\something". On
POSIX this is tolerated as equivalent to "/something", but it surfaces
literally in user-visible error messages like
"Cannot find: //usr/include/foo".
Replace the manual concatenations with path.join, which normalizes correctly
for any base directory including filesystem roots. checkPathExistsSync is
refactored to also use path.join internally; its parameter that used to
require a trailing-separator base directory ("relativePath") is renamed
"baseDir" and no longer requires a trailing separator. Callers updated.
Co-authored-by: Franek Korta <vanklompf@users.noreply.github.com>
Three related issues affect ${workspaceFolderBasename} when the workspace
folder is a filesystem root (path.basename("/") and path.basename("C:\\")
both return ""):
1. expand.ts treated a defined-but-empty variable value as an invalid
reference ("if (!repl)") and emitted an "Invalid variable reference"
warning while leaving the literal ${...} in the output. Distinguish
undefined (not defined) from empty string (defined as "") so empty
substitutions are accepted.
2. CppProperties.ExtendedEnvironment computed workspaceFolderBasename as
path.basename(rootUri.fsPath), yielding "" for root workspaces. Fall
back to the fsPath itself so the value is never empty for a defined
workspace folder.
3. DefaultClient.AdditionalEnvironment computed workspaceFolderBasename
from this.Name (the WorkspaceFolder display name). For non-root
workspaces this typically differs from VS Code's defined semantics for
${workspaceFolderBasename}, and is also "" for chroot-style root
workspaces in many cases. Use path.basename(RootPath) with a fallback to
the fsPath, and only fall back to this.Name when no RootPath is set, so
the value is consistent with VS Code's own ${workspaceFolderBasename}
and never empty for a root workspace.
DebugConfigurationProvider.expand() applies the same path.basename fallback
so debug configs see a non-empty substitution for root workspaces.
Co-authored-by: Franek Korta <vanklompf@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
microsoft/vscode#317637 fixes a bug in the built-in
gitextension where a customnormalizePath()helper unconditionally stripped trailing path separators, turning the filesystem root/(e.g. in a chroot) into"."and breaking descendant / equality / relative-path checks. While reviewing this repo for the same pattern I found that the exactnormalizePathbug is not present here, but several other code paths misbehave when the workspace folder is a filesystem root (/on POSIX, drive root likeC:\on Windows).This PR fixes three such issues. They are split into three logical commits.
1.
onSwitchHeaderSourcesubstring-based path stripThe old code rewrote the symlink-resolved target filename via:
If
RootRealPathis a filesystem root (e.g."/"),substring("/".length)drops the leading separator, producing a malformed result such as"/symlinkfoo.h"whenRootPath = "/symlink"andtargetFileName = "/foo.h". The descendant check viastartsWith(RootRealPath)also matches unrelated siblings like/foovs/foobar.Replaced with
path.relative+path.join, which handles filesystem roots correctly and avoids the false prefix match. This is the spirit of the fix in microsoft/vscode#317637.2.
rootUri.fsPath + path.sep + …concatenationsCppPropertieshad four sites that built paths via:For a workspace at
/, this yields"//something". POSIX tolerates the double slash, but it surfaces verbatim in user-visible diagnostics likeCannot find: //usr/include/foo. On Windows with workspaceC:\, the result is similarly malformed.Replaced with
path.join.checkPathExistsSyncis updated to usepath.joininternally too; its second parameter is renamed fromrelativePathtobaseDirand no longer requires a trailing separator. All callers updated.3.
${workspaceFolderBasename}for root workspacespath.basename("/")andpath.basename("C:\\")both return"". This caused two related problems:expand.tstreats a defined-but-empty variable value the same as an undefined variable (if (!repl)) and emits anInvalid variable reference ${workspaceFolderBasename}warning while leaving the literal${...}in the output. Changed to distinguishundefinedfrom"".workspaceFolderBasename(CppProperties.ExtendedEnvironmentandDefaultClient.AdditionalEnvironment) disagreed (path.basename(fsPath)vsworkspaceFolder.name) and both could yield empty values at root. Both now usepath.basename(fsPath) || fsPathso the substitution is never empty for a defined workspace folder. The debugger's${workspaceFolderBasename}substitution gets the same fallback.Testing
yarn build(full TypeScript build) passes.eslinton all changed files passes with no warnings.Notes
checkPathExistsSync(inputPath, baseDir, …)(renamed parameters), a non-exported convenience helper used only within this extension.