Code review feedback

Author: nojaf

server/src/bsc-args/rewatch.ts

@@ -24,7 +24,7 @@ async function getRuntimePath(
 export async function getRewatchBscArgs(
   send: (msg: p.Message) => void,
   bscBinaryLocation: utils.NormalizedPath | null,
-  projectsFiles: Map<string, projectFiles>,
+  projectsFiles: Map<utils.NormalizedPath, projectFiles>,
   entry: IncrementallyCompiledFileInfo,
 ): Promise<RewatchCompilerArgs | null> {
   const rewatchCacheEntry = entry.buildRewatch;

server/src/incrementalCompilation.ts

@@ -263,24 +263,21 @@ function triggerIncrementalCompilationOfFile(
       if (debug()) console.log("Did not find open project for " + filePath);
       return;
     }
-    // projectRootPath is already the correct normalized key format
-    const actualProjectRootPath = projectRootPath;
 
     // computeWorkspaceRootPathFromLockfile returns null if lockfile found (local package) or if no parent found
-    const computedWorkspaceRoot = utils.computeWorkspaceRootPathFromLockfile(
-      actualProjectRootPath,
-    );
+    const computedWorkspaceRoot =
+      utils.computeWorkspaceRootPathFromLockfile(projectRootPath);
     // If null, it means either a lockfile was found (local package) or no parent project root exists
     // In both cases, we default to actualProjectRootPath
     const workspaceRootPath: NormalizedPath =
-      computedWorkspaceRoot ?? actualProjectRootPath;
+      computedWorkspaceRoot ?? projectRootPath;
 
     // Determine if lockfile was found for debug logging
     // If computedWorkspaceRoot is null and actualProjectRootPath is not null, check if parent exists
     const foundRewatchLockfileInProjectRoot =
       computedWorkspaceRoot == null &&
-      actualProjectRootPath != null &&
-      utils.findProjectRootOfFile(actualProjectRootPath, true) != null;
+      projectRootPath != null &&
+      utils.findProjectRootOfFile(projectRootPath, true) != null;
 
     if (foundRewatchLockfileInProjectRoot && debug()) {
       console.log(
@@ -306,14 +303,14 @@ function triggerIncrementalCompilationOfFile(
         : moduleName;
 
     const incrementalFolderPath: NormalizedPath = path.join(
-      actualProjectRootPath,
+      projectRootPath,
       INCREMENTAL_FILE_FOLDER_LOCATION,
     ) as NormalizedPath;
 
     let originalTypeFileLocation = path.resolve(
-      actualProjectRootPath,
+      projectRootPath,
       c.compilerDirPartialPath,
-      path.relative(actualProjectRootPath, filePath),
+      path.relative(projectRootPath, filePath),
     );
 
     const parsed = path.parse(originalTypeFileLocation);
@@ -333,7 +330,7 @@ function triggerIncrementalCompilationOfFile(
       },
       project: {
         workspaceRootPath,
-        rootPath: actualProjectRootPath,
+        rootPath: projectRootPath,
         callArgs: Promise.resolve([]),
         bscBinaryLocation,
         incrementalFolderPath,

server/src/lookup.ts

@@ -18,15 +18,24 @@ const getCompiledFolderName = (moduleFormat: ModuleFormat): string => {
   }
 };
 
-export const replaceFileExtension = <T extends string>(
-  filePath: T,
+export const replaceFileExtension = (filePath: string, ext: string): string => {
+  let name = path.basename(filePath, path.extname(filePath));
+  return path.format({ dir: path.dirname(filePath), name, ext });
+};
+
+export const replaceFileExtensionWithNormalizedPath = (
+  filePath: NormalizedPath,
   ext: string,
-): T => {
+): NormalizedPath => {
   let name = path.basename(filePath, path.extname(filePath));
   const result = path.format({ dir: path.dirname(filePath), name, ext });
-  // If input was NormalizedPath, result is still normalized
-  // If input was string, result is string
-  return result as T;
+  // path.format() doesn't preserve normalization, so we need to normalize the result
+  const normalized = normalizePath(result);
+  if (normalized == null) {
+    // Should never happen, but handle gracefully
+    return result as NormalizedPath;
+  }
+  return normalized;
 };
 
 // Check if filePartialPath exists at directory and return the joined path,

server/src/server.ts

@@ -1181,7 +1181,10 @@ async function createInterface(msg: p.RequestMessage): Promise<p.Message> {
   let result = typeof response.result === "string" ? response.result : "";
 
   try {
-    let resiPath = lookup.replaceFileExtension(filePath, c.resiExt);
+    let resiPath = lookup.replaceFileExtensionWithNormalizedPath(
+      filePath,
+      c.resiExt,
+    );
     fs.writeFileSync(resiPath, result, { encoding: "utf-8" });
     let response: p.ResponseMessage = {
       jsonrpc: c.jsonrpcVersion,

server/src/utils.ts

@@ -104,22 +104,15 @@ let findProjectRootOfFileInDir = (
 };
 
 // TODO: races here?
-// TODO: this doesn't handle file:/// scheme
 export let findProjectRootOfFile = (
   source: NormalizedPath,
   allowDir?: boolean,
 ): NormalizedPath | null => {
-  // source is already normalized
-  const normalizedSource = source;
-
   // First look in project files (keys are already normalized)
   let foundRootFromProjectFiles: NormalizedPath | null = null;
   for (const rootPath of projectsFiles.keys()) {
     // Both are normalized, so direct comparison works
-    if (
-      normalizedSource.startsWith(rootPath) &&
-      (!allowDir || normalizedSource !== rootPath)
-    ) {
+    if (source.startsWith(rootPath) && (!allowDir || source !== rootPath)) {
       // Prefer the longest path (most nested)
       if (
         foundRootFromProjectFiles == null ||
@@ -742,13 +735,9 @@ let parseFileAndRange = (fileAndRange: string) => {
     // no location! Though LSP insist that we provide at least a dummy location
     const normalizedPath = normalizePath(trimmedFileAndRange);
     if (normalizedPath == null) {
-      return {
-        file: pathToURI(normalizePath("")!),
-        range: {
-          start: { line: 0, character: 0 },
-          end: { line: 0, character: 0 },
-        },
-      };
+      // If we can't normalize the file path, throw an error
+      // This should never happen in practice, but we need to handle it
+      throw new Error(`Failed to normalize file path: ${trimmedFileAndRange}`);
     }
     return {
       file: pathToURI(normalizedPath),
@@ -797,10 +786,9 @@ let parseFileAndRange = (fileAndRange: string) => {
   }
   const normalizedFile = normalizePath(file);
   if (normalizedFile == null) {
-    return {
-      file: pathToURI(normalizePath("")!),
-      range,
-    };
+    // If we can't normalize the file path, throw an error
+    // This should never happen in practice, but we need to handle it
+    throw new Error(`Failed to normalize file path: ${file}`);
   }
   return {
     file: pathToURI(normalizedFile),