Normalize ProjectService.currentDirectory - #24139
Conversation
|
👍 |
| 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
Sheetal Nandi (sheetalkamat)
left a comment
Sheetal Nandi (sheetalkamat)
left a comment
There was a problem hiding this comment.
Doing the normalization when storing currentDirectory in projectService is correct solution.
be6bf5b to
298d23d
Compare
| 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.
298d23d to
0ab3c1f
Compare
Mine Starks (minestarks)
changed the title
|
I'm not very familiar with this code so I'm not too confident I'm testing it effectively. Sheetal Nandi (@sheetalkamat) Ben Lichtman (@uniqueiniquity) counting on your judgment to see if this is a safe change. |
|
Mine Starks (@minestarks) You want to accept the baseline changes to public API. |
4 participants
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.