Normalize ProjectService.currentDirectory #24139
Conversation
|
FYI @paulvanbrenk |
|
👍 |
src/server/editorServices.ts
Outdated
| @@ -1817,8 +1817,8 @@ namespace ts.server { | |||
| if (!info) { | |||
| const isDynamic = isDynamicFileName(fileName); | |||
| Debug.assert(isRootedDiskPath(fileName) || isDynamic || openedByClient, "", () => `${JSON.stringify({ fileName, currentDirectory, hostCurrentDirectory: this.currentDirectory, openKeys: arrayFrom(this.openFilesWithNonRootedDiskPath.keys()) })}\nScript info with non-dynamic relative file name can only be open script info`); | |||
| Debug.assert(!isRootedDiskPath(fileName) || this.currentDirectory === currentDirectory || !this.openFilesWithNonRootedDiskPath.has(this.toCanonicalFileName(fileName)), "", () => `${JSON.stringify({ fileName, currentDirectory, hostCurrentDirectory: this.currentDirectory, openKeys: arrayFrom(this.openFilesWithNonRootedDiskPath.keys()) })}\nOpen script files with non rooted disk path opened with current directory context cannot have same canonical names`); | |||
| Debug.assert(!isDynamic || this.currentDirectory === currentDirectory, "", () => `${JSON.stringify({ fileName, currentDirectory, hostCurrentDirectory: this.currentDirectory, openKeys: arrayFrom(this.openFilesWithNonRootedDiskPath.keys()) })}\nDynamic files must always have current directory context since containing external project name will always match the script info name.`); | |||
| Debug.assert(!isRootedDiskPath(fileName) || toNormalizedPath(this.currentDirectory) === toNormalizedPath(currentDirectory) || !this.openFilesWithNonRootedDiskPath.has(this.toCanonicalFileName(fileName)), "", () => `${JSON.stringify({ fileName, currentDirectory, hostCurrentDirectory: this.currentDirectory, openKeys: arrayFrom(this.openFilesWithNonRootedDiskPath.keys()) })}\nOpen script files with non rooted disk path opened with current directory context cannot have same canonical names`); | |||
There was a problem hiding this comment.
This doesnt seem like correct fix. You want to make sure this.currentDirectory is normalized when storing itself instead.
There was a problem hiding this comment.
https://github.com/Microsoft/TypeScript/blob/master/src/server/editorServices.ts#L453 this where you want to shore normalized current directory
minestarks
force-pushed
the
dynamicprojectassert
branch
from
be6bf5b to
298d23d
Compare
src/server/editorServices.ts
Outdated
| @@ -450,7 +450,7 @@ namespace ts.server { | |||
| if (this.host.realpath) { | |||
| this.realpathToScriptInfos = createMultiMap(); | |||
| } | |||
| this.currentDirectory = this.host.getCurrentDirectory(); | |||
| this.currentDirectory = normalizePath(this.host.getCurrentDirectory()); | |||
There was a problem hiding this comment.
Is it feasible to set currentDirectory: NormalizedPath now so that the type expresses that it's already been normalized?
There was a problem hiding this comment.
Sure. Wasn't familiar with NormalizedPath (still don't quite get it, as it doesn't seem to offer any interesting typechecking?)
There was a problem hiding this comment.
I think it's pretty underpowered right now but I'd like it to be more helpful in the future; seems like many of the path-related bugs I've had to fixed would have been caught by more consistent usage.
minestarks
force-pushed
the
dynamicprojectassert
branch
from
298d23d to
0ab3c1f
Compare
minestarks
changed the title
|
I'm not very familiar with this code so I'm not too confident I'm testing it effectively. @sheetalkamat @uniqueiniquity counting on your judgment to see if this is a safe change. |
|
@minestarks You want to accept the baseline changes to public API. |
This was causing spurious asserts in dynamic file projects like:
"False expression: Verbose Debug Information: {"fileName":"^ScriptDocument104 .txt","currentDirectory":"E:/src/TypeScript-VS/VS/LanguageService/Vsix/bin/Debug","hostCurrentDirectory":"E:\\src\\TypeScript-VS\\VS\\LanguageService\\Vsix\\bin\\Debug","openKeys":[]} Dynamic files must always have current directory context since containing external project name will always match the script info name."And preventing dynamic project creation in VS.