make the compilerPath validation common across JSON and UI

Author: bobbrow

Extension/src/LanguageServer/configurations.ts

@@ -63,7 +63,7 @@ export interface ConfigurationJson {
 export interface Configuration {
     name: string;
     compilerPathInCppPropertiesJson?: string | null;
-    compilerPath?: string | null;
+    compilerPath?: string;
     compilerPathIsExplicit?: boolean;
     compilerArgs?: string[];
     compilerArgsLegacy?: string[];
@@ -982,14 +982,13 @@ export class CppProperties {
             } else {
                 // However, if compileCommands are used and compilerPath is explicitly set, it's still necessary to resolve variables in it.
                 if (configuration.compilerPath === "${default}") {
-                    configuration.compilerPath = settings.defaultCompilerPath;
-                }
-                if (configuration.compilerPath === null) {
+                    configuration.compilerPath = settings.defaultCompilerPath ?? undefined;
                     configuration.compilerPathIsExplicit = true;
-                } else if (configuration.compilerPath !== undefined) {
+                }
+                if (configuration.compilerPath) {
                     configuration.compilerPath = util.resolveVariables(configuration.compilerPath, env);
                     configuration.compilerPathIsExplicit = true;
-                } else {
+                } else if (configuration.compilerPathIsExplicit === undefined) {
                     configuration.compilerPathIsExplicit = false;
                 }
             }
@@ -1596,88 +1595,93 @@ export class CppProperties {
         return result;
     }
 
-    private getErrorsForConfigUI(configIndex: number): ConfigurationErrors {
-        const errors: ConfigurationErrors = {};
-        if (!this.configurationJson) {
-            return errors;
-        }
-        const isWindows: boolean = os.platform() === 'win32';
-        const config: Configuration = this.configurationJson.configurations[configIndex];
-
-        // Check if config name is unique.
-        errors.name = this.isConfigNameUnique(config.name);
-        let resolvedCompilerPath: string | undefined | null;
-        // Validate compilerPath
-        if (config.compilerPath) {
-            resolvedCompilerPath = which.sync(config.compilerPath, { nothrow: true });
-        }
-
+    /**
+     * Get the compilerPath and args from a compilerPath string that has already passed through
+     * `this.resolvePath`. If there are errors processing the path, those will also be returned.
+     *
+     * @resolvedCompilerPath a compilerPath string that has already been resolved.
+     */
+    public static validateCompilerPath(resolvedCompilerPath: string, rootUri?: vscode.Uri): util.CompilerPathAndArgs {
         if (!resolvedCompilerPath) {
-            resolvedCompilerPath = this.resolvePath(config.compilerPath);
+            return { compilerName: '', allCompilerArgs: [], compilerArgsFromCommandLineInPath: [] };
         }
-        const settings: CppSettings = new CppSettings(this.rootUri);
-        const compilerPathAndArgs: util.CompilerPathAndArgs = util.extractCompilerPathAndArgs(!!settings.legacyCompilerArgsBehavior, resolvedCompilerPath);
-        if (resolvedCompilerPath
-            // Don't error cl.exe paths because it could be for an older preview build.
-            && compilerPathAndArgs.compilerName.toLowerCase() !== "cl.exe"
-            && compilerPathAndArgs.compilerName.toLowerCase() !== "cl") {
-            resolvedCompilerPath = resolvedCompilerPath.trim();
-
-            // Error when the compiler's path has spaces without quotes but args are used.
-            // Except, exclude cl.exe paths because it could be for an older preview build.
-            const compilerPathNeedsQuotes: boolean =
-                (compilerPathAndArgs.compilerArgsFromCommandLineInPath && compilerPathAndArgs.compilerArgsFromCommandLineInPath.length > 0) &&
-                !resolvedCompilerPath.startsWith('"') &&
-                compilerPathAndArgs.compilerPath !== undefined &&
-                compilerPathAndArgs.compilerPath !== null &&
-                compilerPathAndArgs.compilerPath.includes(" ");
-
-            const compilerPathErrors: string[] = [];
-            if (compilerPathNeedsQuotes) {
-                compilerPathErrors.push(localize("path.with.spaces", 'Compiler path with spaces and arguments is missing double quotes " around the path.'));
-            }
+        resolvedCompilerPath = resolvedCompilerPath.trim();
+
+        const settings = new CppSettings(rootUri);
+        const compilerPathAndArgs = util.extractCompilerPathAndArgs(!!settings.legacyCompilerArgsBehavior, resolvedCompilerPath, undefined, rootUri?.fsPath);
+        const compilerLowerCase: string = compilerPathAndArgs.compilerName.toLowerCase();
+        const isCl: boolean = compilerLowerCase === "cl" || compilerLowerCase === "cl.exe";
+        const telemetry: { [key: string]: number } = {};
 
+        // Don't error cl.exe paths because it could be for an older preview build.
+        if (!isCl) {
             // Get compiler path without arguments before checking if it exists
-            resolvedCompilerPath = compilerPathAndArgs.compilerPath ?? undefined;
-            if (resolvedCompilerPath) {
+            if (compilerPathAndArgs.compilerPath) {
+                resolvedCompilerPath = compilerPathAndArgs.compilerPath;
                 let pathExists: boolean = true;
                 const existsWithExeAdded: (path: string) => boolean = (path: string) => isWindows && !path.startsWith("/") && fs.existsSync(path + ".exe");
                 if (!fs.existsSync(resolvedCompilerPath)) {
                     if (existsWithExeAdded(resolvedCompilerPath)) {
                         resolvedCompilerPath += ".exe";
-                    } else if (!this.rootUri) {
-                        pathExists = false;
                     } else {
-                        // Check again for a relative path.
-                        const relativePath: string = this.rootUri.fsPath + path.sep + resolvedCompilerPath;
-                        if (!fs.existsSync(relativePath)) {
-                            if (existsWithExeAdded(resolvedCompilerPath)) {
-                                resolvedCompilerPath += ".exe";
+                        const pathLocation = which.sync(resolvedCompilerPath, { nothrow: true });
+                        if (pathLocation) {
+                            resolvedCompilerPath = pathLocation;
+                            compilerPathAndArgs.compilerPath = pathLocation;
+                        } else if (rootUri) {
+                            // Check again for a relative path.
+                            const relativePath: string = rootUri.fsPath + path.sep + resolvedCompilerPath;
+                            if (!fs.existsSync(relativePath)) {
+                                if (existsWithExeAdded(relativePath)) {
+                                    resolvedCompilerPath = relativePath + ".exe";
+                                } else {
+                                    pathExists = false;
+                                }
                             } else {
-                                pathExists = false;
+                                resolvedCompilerPath = relativePath;
                             }
-                        } else {
-                            resolvedCompilerPath = relativePath;
                         }
                     }
                 }
 
+                const compilerPathErrors: string[] = [];
+
                 if (!pathExists) {
                     const message: string = localize('cannot.find', "Cannot find: {0}", resolvedCompilerPath);
                     compilerPathErrors.push(message);
-                } else if (compilerPathAndArgs.compilerPath === "") {
-                    const message: string = localize("cannot.resolve.compiler.path", "Invalid input, cannot resolve compiler path");
-                    compilerPathErrors.push(message);
+                    telemetry.PathNonExistent = 1;
                 } else if (!util.checkExecutableWithoutExtensionExistsSync(resolvedCompilerPath)) {
                     const message: string = localize("path.is.not.a.file", "Path is not a file: {0}", resolvedCompilerPath);
                     compilerPathErrors.push(message);
+                    telemetry.PathNotAFile = 1;
                 }
 
                 if (compilerPathErrors.length > 0) {
-                    errors.compilerPath = compilerPathErrors.join('\n');
+                    compilerPathAndArgs.error = compilerPathErrors.join('\n');
                 }
             }
         }
+        compilerPathAndArgs.telemetry = telemetry;
+        return compilerPathAndArgs;
+    }
+
+    private getErrorsForConfigUI(configIndex: number): ConfigurationErrors {
+        const errors: ConfigurationErrors = {};
+        if (!this.configurationJson) {
+            return errors;
+        }
+        const isWindows: boolean = os.platform() === 'win32';
+        const config: Configuration = this.configurationJson.configurations[configIndex];
+
+        // Check if config name is unique.
+        errors.name = this.isConfigNameUnique(config.name);
+        let resolvedCompilerPath: string | undefined | null;
+        // Validate compilerPath
+        if (!resolvedCompilerPath) {
+            resolvedCompilerPath = this.resolvePath(config.compilerPath, false, false);
+        }
+        const compilerPathAndArgs: util.CompilerPathAndArgs = CppProperties.validateCompilerPath(resolvedCompilerPath, this.rootUri);
+        errors.compilerPath = compilerPathAndArgs.error;
 
         // Validate paths (directories)
         errors.includePath = this.validatePath(config.includePath, { globPaths: true });
@@ -1928,7 +1932,6 @@ export class CppProperties {
 
         // Check for path-related squiggles.
         const paths: string[] = [];
-        let compilerPath: string | undefined;
         for (const pathArray of [currentConfiguration.browse ? currentConfiguration.browse.path : undefined, currentConfiguration.includePath, currentConfiguration.macFrameworkPath]) {
             if (pathArray) {
                 for (const curPath of pathArray) {
@@ -1950,13 +1953,6 @@ export class CppProperties {
             paths.push(`${file}`);
         });
 
-        if (currentConfiguration.compilerPath) {
-            // Unlike other cases, compilerPath may not start or end with " due to trimming of whitespace and the possibility of compiler args.
-            compilerPath = currentConfiguration.compilerPath;
-        }
-
-        compilerPath = this.resolvePath(compilerPath).trim();
-
         // Get the start/end for properties that are file-only.
         const forcedIncludeStart: number = curText.search(/\s*\"forcedInclude\"\s*:\s*\[/);
         const forcedeIncludeEnd: number = forcedIncludeStart === -1 ? -1 : curText.indexOf("]", forcedIncludeStart);
@@ -1973,46 +1969,20 @@ export class CppProperties {
         const processedPaths: Set<string> = new Set<string>();
 
         // Validate compiler paths
-        let compilerPathNeedsQuotes: boolean = false;
-        let compilerMessage: string | undefined;
-        const compilerPathAndArgs: util.CompilerPathAndArgs = util.extractCompilerPathAndArgs(!!settings.legacyCompilerArgsBehavior, compilerPath);
-        const compilerLowerCase: string = compilerPathAndArgs.compilerName.toLowerCase();
-        const isClCompiler: boolean = compilerLowerCase === "cl" || compilerLowerCase === "cl.exe";
-        // Don't squiggle for invalid cl and cl.exe paths.
-        if (compilerPathAndArgs.compilerPath && !isClCompiler) {
-            // Squiggle when the compiler's path has spaces without quotes but args are used.
-            compilerPathNeedsQuotes = (compilerPathAndArgs.compilerArgsFromCommandLineInPath && compilerPathAndArgs.compilerArgsFromCommandLineInPath.length > 0)
-                && !compilerPath.startsWith('"')
-                && compilerPathAndArgs.compilerPath.includes(" ");
-            compilerPath = compilerPathAndArgs.compilerPath;
-            // Don't squiggle if compiler path is resolving with environment path.
-            if (compilerPathNeedsQuotes || (compilerPath && !which.sync(compilerPath, { nothrow: true }))) {
-                if (compilerPathNeedsQuotes) {
-                    compilerMessage = localize("path.with.spaces", 'Compiler path with spaces and arguments is missing double quotes " around the path.');
-                    newSquiggleMetrics.CompilerPathMissingQuotes++;
-                } else if (!util.checkExecutableWithoutExtensionExistsSync(compilerPath)) {
-                    compilerMessage = localize("path.is.not.a.file", "Path is not a file: {0}", compilerPath);
-                    newSquiggleMetrics.PathNotAFile++;
-                }
-            }
-        }
-        let compilerPathExists: boolean = true;
-        if (this.rootUri && !isClCompiler) {
-            const checkPathExists: any = util.checkPathExistsSync(compilerPath, this.rootUri.fsPath + path.sep, isWindows, true);
-            compilerPathExists = checkPathExists.pathExists;
-            compilerPath = checkPathExists.path;
-        }
-        if (!compilerPathExists) {
-            compilerMessage = localize('cannot.find', "Cannot find: {0}", compilerPath);
-            newSquiggleMetrics.PathNonExistent++;
-        }
-        if (compilerMessage) {
+        const resolvedCompilerPath = this.resolvePath(currentConfiguration.compilerPath, false, false);
+        const compilerPathAndArgs: util.CompilerPathAndArgs = CppProperties.validateCompilerPath(resolvedCompilerPath, this.rootUri);
+        if (compilerPathAndArgs.error) {
             const diagnostic: vscode.Diagnostic = new vscode.Diagnostic(
-                new vscode.Range(document.positionAt(curTextStartOffset + compilerPathValueStart),
-                    document.positionAt(curTextStartOffset + compilerPathEnd)),
-                compilerMessage, vscode.DiagnosticSeverity.Warning);
+                new vscode.Range(document.positionAt(curTextStartOffset + compilerPathValueStart), document.positionAt(curTextStartOffset + compilerPathEnd)),
+                compilerPathAndArgs.error,
+                vscode.DiagnosticSeverity.Warning);
             diagnostics.push(diagnostic);
         }
+        if (compilerPathAndArgs.telemetry) {
+            for (const o of Object.keys(compilerPathAndArgs.telemetry)) {
+                newSquiggleMetrics[o] = compilerPathAndArgs.telemetry[o];
+            }
+        }
 
         // validate .config path
         let dotConfigPath: string | undefined;

Extension/src/LanguageServer/cppBuildTaskProvider.ts

@@ -98,7 +98,7 @@ export class CppBuildTaskProvider implements TaskProvider {
 
         // Get user compiler path.
         const userCompilerPathAndArgs: util.CompilerPathAndArgs | undefined = await activeClient.getCurrentCompilerPathAndArgs();
-        let userCompilerPath: string | undefined | null;
+        let userCompilerPath: string | undefined;
         if (userCompilerPathAndArgs) {
             userCompilerPath = userCompilerPathAndArgs.compilerPath;
             if (userCompilerPath && userCompilerPathAndArgs.compilerName) {