Skip to content

Commit 6a8155d

Browse files
bobbrowbobbrow
committed
Add back an error for missing quotes when the compiler parsed does not exist
1 parent 8cbc066 commit 6a8155d

File tree

3 files changed

+68
-58
lines changed

3 files changed

+68
-58
lines changed

‎Extension/src/LanguageServer/configurations.ts‎

Lines changed: 40 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1614,51 +1614,54 @@ export class CppProperties {
16141614
const telemetry: { [key: string]: number } = {};
16151615

16161616
// Don't error cl.exe paths because it could be for an older preview build.
1617-
if (!isCl) {
1618-
// Get compiler path without arguments before checking if it exists
1619-
if (compilerPathAndArgs.compilerPath) {
1620-
resolvedCompilerPath = compilerPathAndArgs.compilerPath;
1621-
let pathExists: boolean = true;
1622-
const existsWithExeAdded: (path: string) => boolean = (path: string) => isWindows && !path.startsWith("/") && fs.existsSync(path + ".exe");
1623-
if (!fs.existsSync(resolvedCompilerPath)) {
1624-
if (existsWithExeAdded(resolvedCompilerPath)) {
1625-
resolvedCompilerPath += ".exe";
1626-
} else {
1627-
const pathLocation = which.sync(resolvedCompilerPath, { nothrow: true });
1628-
if (pathLocation) {
1629-
resolvedCompilerPath = pathLocation;
1630-
compilerPathAndArgs.compilerPath = pathLocation;
1631-
} else if (rootUri) {
1632-
// Check again for a relative path.
1633-
const relativePath: string = rootUri.fsPath + path.sep + resolvedCompilerPath;
1634-
if (!fs.existsSync(relativePath)) {
1635-
if (existsWithExeAdded(relativePath)) {
1636-
resolvedCompilerPath = relativePath + ".exe";
1637-
} else {
1638-
pathExists = false;
1639-
}
1617+
if (!isCl && compilerPathAndArgs.compilerPath) {
1618+
const compilerPathMayNeedQuotes: boolean = !resolvedCompilerPath.startsWith('"') && resolvedCompilerPath.includes(" ") && compilerPathAndArgs.compilerArgsFromCommandLineInPath.length > 0;
1619+
let pathExists: boolean = true;
1620+
const existsWithExeAdded: (path: string) => boolean = (path: string) => isWindows && !path.startsWith("/") && fs.existsSync(path + ".exe");
1621+
1622+
resolvedCompilerPath = compilerPathAndArgs.compilerPath;
1623+
if (!fs.existsSync(resolvedCompilerPath)) {
1624+
if (existsWithExeAdded(resolvedCompilerPath)) {
1625+
resolvedCompilerPath += ".exe";
1626+
} else {
1627+
const pathLocation = which.sync(resolvedCompilerPath, { nothrow: true });
1628+
if (pathLocation) {
1629+
resolvedCompilerPath = pathLocation;
1630+
compilerPathAndArgs.compilerPath = pathLocation;
1631+
} else if (rootUri) {
1632+
// Check again for a relative path.
1633+
const relativePath: string = rootUri.fsPath + path.sep + resolvedCompilerPath;
1634+
if (!fs.existsSync(relativePath)) {
1635+
if (existsWithExeAdded(relativePath)) {
1636+
resolvedCompilerPath = relativePath + ".exe";
16401637
} else {
1641-
resolvedCompilerPath = relativePath;
1638+
pathExists = false;
16421639
}
1640+
} else {
1641+
resolvedCompilerPath = relativePath;
16431642
}
16441643
}
16451644
}
1645+
}
16461646

1647-
const compilerPathErrors: string[] = [];
1647+
const compilerPathErrors: string[] = [];
1648+
if (compilerPathMayNeedQuotes && !pathExists) {
1649+
compilerPathErrors.push(localize("path.with.spaces", 'Compiler path with spaces and arguments is missing double quotes " around the path.'));
1650+
telemetry.CompilerPathMissingQuotes = 1;
1651+
}
16481652

1649-
if (!pathExists) {
1650-
const message: string = localize('cannot.find', "Cannot find: {0}", resolvedCompilerPath);
1651-
compilerPathErrors.push(message);
1652-
telemetry.PathNonExistent = 1;
1653-
} else if (!util.checkExecutableWithoutExtensionExistsSync(resolvedCompilerPath)) {
1654-
const message: string = localize("path.is.not.a.file", "Path is not a file: {0}", resolvedCompilerPath);
1655-
compilerPathErrors.push(message);
1656-
telemetry.PathNotAFile = 1;
1657-
}
1653+
if (!pathExists) {
1654+
const message: string = localize('cannot.find', "Cannot find: {0}", resolvedCompilerPath);
1655+
compilerPathErrors.push(message);
1656+
telemetry.PathNonExistent = 1;
1657+
} else if (!util.checkExecutableWithoutExtensionExistsSync(resolvedCompilerPath)) {
1658+
const message: string = localize("path.is.not.a.file", "Path is not a file: {0}", resolvedCompilerPath);
1659+
compilerPathErrors.push(message);
1660+
telemetry.PathNotAFile = 1;
1661+
}
16581662

1659-
if (compilerPathErrors.length > 0) {
1660-
compilerPathAndArgs.error = compilerPathErrors.join('\n');
1661-
}
1663+
if (compilerPathErrors.length > 0) {
1664+
compilerPathAndArgs.error = compilerPathErrors.join('\n');
16621665
}
16631666
}
16641667
compilerPathAndArgs.telemetry = telemetry;

‎Extension/src/common.ts‎

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1108,6 +1108,16 @@ export function extractCompilerPathAndArgs(useLegacyBehavior: boolean, inputComp
11081108
let compilerPath: string | undefined = inputCompilerPath;
11091109
let compilerName: string = "";
11101110
let compilerArgsFromCommandLineInPath: string[] = [];
1111+
const trimLegacyQuotes = (compilerPath?: string): string | undefined => {
1112+
if (compilerPath && useLegacyBehavior) {
1113+
// Try to trim quotes from compiler path.
1114+
const tempCompilerPath: string[] = extractArgs(compilerPath);
1115+
if (tempCompilerPath.length > 0) {
1116+
return tempCompilerPath[0];
1117+
}
1118+
}
1119+
return compilerPath;
1120+
};
11111121
if (compilerPath) {
11121122
compilerPath = compilerPath.trim();
11131123
if (isCl(compilerPath) || checkExecutableWithoutExtensionExistsSync(compilerPath)) {
@@ -1122,31 +1132,15 @@ export function extractCompilerPathAndArgs(useLegacyBehavior: boolean, inputComp
11221132
// Otherwise, a path with a leading quote would not be valid.
11231133
compilerArgsFromCommandLineInPath = useLegacyBehavior ? legacyExtractArgs(compilerPath) : extractArgs(compilerPath);
11241134
if (compilerArgsFromCommandLineInPath.length > 0) {
1125-
compilerPath = compilerArgsFromCommandLineInPath.shift();
1126-
if (compilerPath) {
1127-
if (useLegacyBehavior) {
1128-
// Try to trim quotes from compiler path.
1129-
const tempCompilerPath: string[] = extractArgs(compilerPath);
1130-
if (tempCompilerPath?.length > 0) {
1131-
compilerPath = tempCompilerPath[0];
1132-
}
1133-
}
1134-
compilerName = path.basename(compilerPath);
1135-
}
1135+
compilerPath = trimLegacyQuotes(compilerArgsFromCommandLineInPath.shift());
1136+
compilerName = path.basename(compilerPath ?? '');
11361137
}
11371138
} else {
11381139
if (compilerPath.includes(' ')) {
1139-
// There is no leading quote, so we'll treat is as a command line.
1140+
// There is no leading quote, so we'll treat it as a command line.
11401141
const potentialArgs: string[] = useLegacyBehavior ? legacyExtractArgs(compilerPath) : extractArgs(compilerPath);
1141-
let potentialCompilerPath: string | undefined = potentialArgs.shift();
1142-
if (useLegacyBehavior && potentialCompilerPath) {
1143-
const tempCompilerPath: string[] = extractArgs(potentialCompilerPath);
1144-
if (tempCompilerPath?.length > 0) {
1145-
potentialCompilerPath = tempCompilerPath[0];
1146-
}
1147-
}
1142+
compilerPath = trimLegacyQuotes(potentialArgs.shift());
11481143
compilerArgsFromCommandLineInPath = potentialArgs;
1149-
compilerPath = potentialCompilerPath;
11501144
}
11511145
compilerName = path.basename(compilerPath ?? '');
11521146
}

‎Extension/test/scenarios/SingleRootProject/tests/compilerPath.test.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,8 @@ describe('validateCompilerPath', () => {
172172
const argsTests: [string, Uri, string, string[]][] = [
173173
['cl.exe /std:c++20 /O2', assetsFolder, 'cl.exe', ['/std:c++20', '/O2']], // issue with /Fo"test.obj" argument
174174
[`"${path.join(assetsFolderFsPath, 'b i n', 'clang++')}" -std=c++20 -O2`, assetsFolder, 'clang++', ['-std=c++20', '-O2']],
175-
[`${path.join('bin', 'gcc')} -std=c++20 -Wall`, assetsFolder, 'gcc', ['-std=c++20', '-Wall']]
175+
[`${path.join('bin', 'gcc')} -std=c++20 -Wall`, assetsFolder, 'gcc', ['-std=c++20', '-Wall']],
176+
['clang -O2 -Wall', assetsFolder, 'clang', ['-O2', '-Wall']]
176177
];
177178
it('Verify various compilerPath strings with args', () => {
178179
let index = -1;
@@ -220,4 +221,16 @@ describe('validateCompilerPath', () => {
220221
equal(result.telemetry?.PathNonExistent, 1, 'Should have telemetry for relative paths');
221222
equal(result.telemetry?.PathNotAFile, undefined, 'Should not have telemetry for invalid paths');
222223
});
224+
225+
it('Verify errors with unknown compiler not in Path with args', async () => {
226+
const result = CppProperties.validateCompilerPath('icc -O2', assetsFolder);
227+
equal(result.compilerName, 'icc', 'compilerName should be found');
228+
deepEqual(result.allCompilerArgs, ['-O2'], 'args should match');
229+
ok(result.error?.includes('Cannot find'), 'Should have an error for unknown compiler');
230+
ok(result.error?.includes('missing double quotes'), 'Should have an error for missing double quotes');
231+
equal(result.telemetry?.PathNonExistent, 1, 'Should have telemetry for relative paths');
232+
equal(result.telemetry?.PathNotAFile, undefined, 'Should not have telemetry for invalid paths');
233+
equal(result.telemetry?.CompilerPathMissingQuotes, 1, 'Should have telemetry for missing quotes');
234+
});
235+
223236
});

0 commit comments

Comments
 (0)