Skip to content

Commit 1798e8f

Browse files
author
Andy
authored
Merge pull request #15131 from Microsoft/boolean-trivia-spacing
boolean-trivia lint rule: Enforce space between comment and argument
2 parents 34e3f5f + ed5eca2 commit 1798e8f

23 files changed

+143
-124
lines changed

‎Gulpfile.ts

+9-9
Original file line numberDiff line numberDiff line change
@@ -384,7 +384,7 @@ function prependCopyright(outputCopyright: boolean = !useDebugMode) {
384384
}
385385

386386
gulp.task(builtLocalCompiler, /*help*/ false, [servicesFile], () => {
387-
const localCompilerProject = tsc.createProject("src/compiler/tsconfig.json", getCompilerSettings({}, /*useBuiltCompiler*/true));
387+
const localCompilerProject = tsc.createProject("src/compiler/tsconfig.json", getCompilerSettings({}, /*useBuiltCompiler*/ true));
388388
return localCompilerProject.src()
389389
.pipe(newer(builtLocalCompiler))
390390
.pipe(sourcemaps.init())
@@ -395,14 +395,14 @@ gulp.task(builtLocalCompiler, /*help*/ false, [servicesFile], () => {
395395
});
396396

397397
gulp.task(servicesFile, /*help*/ false, ["lib", "generate-diagnostics"], () => {
398-
const servicesProject = tsc.createProject("src/services/tsconfig.json", getCompilerSettings({ removeComments: false }, /*useBuiltCompiler*/false));
398+
const servicesProject = tsc.createProject("src/services/tsconfig.json", getCompilerSettings({ removeComments: false }, /*useBuiltCompiler*/ false));
399399
const {js, dts} = servicesProject.src()
400400
.pipe(newer(servicesFile))
401401
.pipe(sourcemaps.init())
402402
.pipe(servicesProject());
403403
const completedJs = js.pipe(prependCopyright())
404404
.pipe(sourcemaps.write("."));
405-
const completedDts = dts.pipe(prependCopyright(/*outputCopyright*/true))
405+
const completedDts = dts.pipe(prependCopyright(/*outputCopyright*/ true))
406406
.pipe(insert.transform((contents, file) => {
407407
file.path = standaloneDefinitionsFile;
408408
return contents.replace(/^(\s*)(export )?const enum (\S+) {(\s*)$/gm, "$1$2enum $3 {$4");
@@ -429,7 +429,7 @@ gulp.task(servicesFile, /*help*/ false, ["lib", "generate-diagnostics"], () => {
429429
// cancellationToken.js
430430
const cancellationTokenJs = path.join(builtLocalDirectory, "cancellationToken.js");
431431
gulp.task(cancellationTokenJs, /*help*/ false, [servicesFile], () => {
432-
const cancellationTokenProject = tsc.createProject("src/server/cancellationToken/tsconfig.json", getCompilerSettings({}, /*useBuiltCompiler*/true));
432+
const cancellationTokenProject = tsc.createProject("src/server/cancellationToken/tsconfig.json", getCompilerSettings({}, /*useBuiltCompiler*/ true));
433433
return cancellationTokenProject.src()
434434
.pipe(newer(cancellationTokenJs))
435435
.pipe(sourcemaps.init())
@@ -442,7 +442,7 @@ gulp.task(cancellationTokenJs, /*help*/ false, [servicesFile], () => {
442442
// typingsInstallerFile.js
443443
const typingsInstallerJs = path.join(builtLocalDirectory, "typingsInstaller.js");
444444
gulp.task(typingsInstallerJs, /*help*/ false, [servicesFile], () => {
445-
const cancellationTokenProject = tsc.createProject("src/server/typingsInstaller/tsconfig.json", getCompilerSettings({}, /*useBuiltCompiler*/true));
445+
const cancellationTokenProject = tsc.createProject("src/server/typingsInstaller/tsconfig.json", getCompilerSettings({}, /*useBuiltCompiler*/ true));
446446
return cancellationTokenProject.src()
447447
.pipe(newer(typingsInstallerJs))
448448
.pipe(sourcemaps.init())
@@ -455,7 +455,7 @@ gulp.task(typingsInstallerJs, /*help*/ false, [servicesFile], () => {
455455
const serverFile = path.join(builtLocalDirectory, "tsserver.js");
456456

457457
gulp.task(serverFile, /*help*/ false, [servicesFile, typingsInstallerJs, cancellationTokenJs], () => {
458-
const serverProject = tsc.createProject("src/server/tsconfig.json", getCompilerSettings({}, /*useBuiltCompiler*/true));
458+
const serverProject = tsc.createProject("src/server/tsconfig.json", getCompilerSettings({}, /*useBuiltCompiler*/ true));
459459
return serverProject.src()
460460
.pipe(newer(serverFile))
461461
.pipe(sourcemaps.init())
@@ -479,7 +479,7 @@ gulp.task(tsserverLibraryFile, /*help*/ false, [servicesFile], (done) => {
479479
js.pipe(prependCopyright())
480480
.pipe(sourcemaps.write("."))
481481
.pipe(gulp.dest("src/server")),
482-
dts.pipe(prependCopyright(/*outputCopyright*/true))
482+
dts.pipe(prependCopyright(/*outputCopyright*/ true))
483483
.pipe(insert.transform((content) => {
484484
return content + "\r\nexport = ts;\r\nexport as namespace ts;";
485485
}))
@@ -551,7 +551,7 @@ gulp.task("LKG", "Makes a new LKG out of the built js files", ["clean", "dontUse
551551
// Task to build the tests infrastructure using the built compiler
552552
const run = path.join(builtLocalDirectory, "run.js");
553553
gulp.task(run, /*help*/ false, [servicesFile], () => {
554-
const testProject = tsc.createProject("src/harness/tsconfig.json", getCompilerSettings({}, /*useBuiltCompiler*/true));
554+
const testProject = tsc.createProject("src/harness/tsconfig.json", getCompilerSettings({}, /*useBuiltCompiler*/ true));
555555
return testProject.src()
556556
.pipe(newer(run))
557557
.pipe(sourcemaps.init())
@@ -757,7 +757,7 @@ gulp.task("browserify", "Runs browserify on run.js to produce a file suitable fo
757757
browserify(intoStream(file.contents), { debug: true })
758758
.bundle((err, res) => {
759759
// assumes file.contents is a Buffer
760-
const maps = JSON.parse(convertMap.fromSource(res.toString(), /*largeSource*/true).toJSON());
760+
const maps = JSON.parse(convertMap.fromSource(res.toString(), /*largeSource*/ true).toJSON());
761761
delete maps.sourceRoot;
762762
maps.sources = maps.sources.map(s => path.resolve(s === "_stream_0.js" ? "built/local/_stream_0.js" : s));
763763
// Strip browserify's inline comments away (could probably just let sorcery do this, but then we couldn't fix the paths)

‎scripts/tslint/booleanTriviaRule.ts

+70-51
Original file line numberDiff line numberDiff line change
@@ -8,69 +8,88 @@ export class Rule extends Lint.Rules.AbstractRule {
88
}
99

1010
function walk(ctx: Lint.WalkContext<void>): void {
11-
ts.forEachChild(ctx.sourceFile, function recur(node: ts.Node): void {
11+
const { sourceFile } = ctx;
12+
ts.forEachChild(sourceFile, function recur(node: ts.Node): void {
1213
if (node.kind === ts.SyntaxKind.CallExpression) {
1314
checkCall(node as ts.CallExpression);
1415
}
1516
ts.forEachChild(node, recur);
1617
});
1718

1819
function checkCall(node: ts.CallExpression): void {
19-
for (const arg of node.arguments) {
20-
switch (arg.kind) {
21-
case ts.SyntaxKind.TrueKeyword:
22-
case ts.SyntaxKind.FalseKeyword:
23-
case ts.SyntaxKind.NullKeyword:
24-
break;
25-
case ts.SyntaxKind.Identifier:
26-
if ((arg as ts.Identifier).originalKeywordKind !== ts.SyntaxKind.UndefinedKeyword) {
27-
continue;
28-
}
29-
break;
30-
default:
31-
continue;
20+
if (!shouldIgnoreCalledExpression(node.expression)) {
21+
for (const arg of node.arguments) {
22+
checkArg(arg);
3223
}
24+
}
25+
}
3326

34-
if (node.expression.kind === ts.SyntaxKind.PropertyAccessExpression) {
35-
const methodName = (node.expression as ts.PropertyAccessExpression).name.text;
36-
// Skip certain method names whose parameter names are not informative
37-
if (methodName.indexOf("set") === 0) {
38-
continue;
39-
}
40-
switch (methodName) {
41-
case "apply":
42-
case "assert":
43-
case "call":
44-
case "equal":
45-
case "fail":
46-
case "isTrue":
47-
case "output":
48-
case "stringify":
49-
continue;
50-
}
27+
/** Skip certain function/method names whose parameter names are not informative. */
28+
function shouldIgnoreCalledExpression(expression: ts.Expression): boolean {
29+
if (expression.kind === ts.SyntaxKind.PropertyAccessExpression) {
30+
const methodName = (expression as ts.PropertyAccessExpression).name.text;
31+
if (methodName.indexOf("set") === 0) {
32+
return true;
5133
}
52-
else if (node.expression.kind === ts.SyntaxKind.Identifier) {
53-
const functionName = (node.expression as ts.Identifier).text;
54-
// Skip certain function names whose parameter names are not informative
55-
if (functionName.indexOf("set") === 0) {
56-
continue;
57-
}
58-
switch (functionName) {
59-
case "assert":
60-
case "contains":
61-
case "createAnonymousType":
62-
case "createImportSpecifier":
63-
case "createProperty":
64-
case "createSignature":
65-
case "resolveName":
66-
continue;
67-
}
34+
switch (methodName) {
35+
case "apply":
36+
case "assert":
37+
case "call":
38+
case "equal":
39+
case "fail":
40+
case "isTrue":
41+
case "output":
42+
case "stringify":
43+
return true;
6844
}
69-
70-
const ranges = ts.getLeadingCommentRanges(arg.getFullText(), 0);
71-
if (!(ranges && ranges.length === 1 && ranges[0].kind === ts.SyntaxKind.MultiLineCommentTrivia)) {
72-
ctx.addFailureAtNode(arg, 'Tag boolean argument with parameter name');
45+
}
46+
else if (expression.kind === ts.SyntaxKind.Identifier) {
47+
const functionName = (expression as ts.Identifier).text;
48+
if (functionName.indexOf("set") === 0) {
49+
return true;
7350
}
51+
switch (functionName) {
52+
case "assert":
53+
case "contains":
54+
case "createAnonymousType":
55+
case "createImportSpecifier":
56+
case "createProperty":
57+
case "createSignature":
58+
case "resolveName":
59+
return true;
60+
}
61+
}
62+
return false;
63+
}
64+
65+
function checkArg(arg: ts.Expression): void {
66+
if (!isTrivia(arg)) {
67+
return;
68+
}
69+
70+
let ranges = ts.getTrailingCommentRanges(sourceFile.text, arg.pos) || ts.getLeadingCommentRanges(sourceFile.text, arg.pos);
71+
if (ranges === undefined || ranges.length !== 1 || ranges[0].kind !== ts.SyntaxKind.MultiLineCommentTrivia) {
72+
ctx.addFailureAtNode(arg, "Tag boolean argument with parameter name");
73+
return;
74+
}
75+
76+
const range = ranges[0];
77+
const argStart = arg.getStart(sourceFile);
78+
if (range.end + 1 !== argStart && sourceFile.text.slice(range.end, argStart).indexOf("\n") === -1) {
79+
ctx.addFailureAtNode(arg, "There should be 1 space between an argument and its comment.");
80+
}
81+
}
82+
83+
function isTrivia(arg: ts.Expression): boolean {
84+
switch (arg.kind) {
85+
case ts.SyntaxKind.TrueKeyword:
86+
case ts.SyntaxKind.FalseKeyword:
87+
case ts.SyntaxKind.NullKeyword:
88+
return true;
89+
case ts.SyntaxKind.Identifier:
90+
return (arg as ts.Identifier).originalKeywordKind === ts.SyntaxKind.UndefinedKeyword;
91+
default:
92+
return false;
7493
}
7594
}
7695
}

‎src/compiler/binder.ts

+3-3
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,7 @@ namespace ts {
453453
return local;
454454
}
455455
else {
456-
return declareSymbol(container.locals, /*parent*/undefined, node, symbolFlags, symbolExcludes);
456+
return declareSymbol(container.locals, /*parent*/ undefined, node, symbolFlags, symbolExcludes);
457457
}
458458
}
459459
}
@@ -2333,7 +2333,7 @@ namespace ts {
23332333

23342334
function bindThisPropertyAssignment(node: BinaryExpression) {
23352335
Debug.assert(isInJavaScriptFile(node));
2336-
const container = getThisContainer(node, /*includeArrowFunctions*/false);
2336+
const container = getThisContainer(node, /*includeArrowFunctions*/ false);
23372337
switch (container.kind) {
23382338
case SyntaxKind.FunctionDeclaration:
23392339
case SyntaxKind.FunctionExpression:
@@ -2423,7 +2423,7 @@ namespace ts {
24232423
function bindCallExpression(node: CallExpression) {
24242424
// We're only inspecting call expressions to detect CommonJS modules, so we can skip
24252425
// this check if we've already seen the module indicator
2426-
if (!file.commonJsModuleIndicator && isRequireCall(node, /*checkArgumentIsStringLiteral*/false)) {
2426+
if (!file.commonJsModuleIndicator && isRequireCall(node, /*checkArgumentIsStringLiteral*/ false)) {
24272427
setCommonJsModuleIndicator(node);
24282428
}
24292429
}

‎src/compiler/checker.ts

+7-7
Original file line numberDiff line numberDiff line change
@@ -2620,7 +2620,7 @@ namespace ts {
26202620
propertyName,
26212621
optionalToken,
26222622
propertyTypeNode,
2623-
/*initializer*/undefined));
2623+
/*initializer*/ undefined));
26242624
}
26252625
}
26262626
return typeElements.length ? typeElements : undefined;
@@ -2754,7 +2754,7 @@ namespace ts {
27542754

27552755
/** @param endOfChain Set to false for recursive calls; non-recursive calls should always output something. */
27562756
function getSymbolChain(symbol: Symbol, meaning: SymbolFlags, endOfChain: boolean): Symbol[] | undefined {
2757-
let accessibleSymbolChain = getAccessibleSymbolChain(symbol, context.enclosingDeclaration, meaning, /*useOnlyExternalAliasing*/false);
2757+
let accessibleSymbolChain = getAccessibleSymbolChain(symbol, context.enclosingDeclaration, meaning, /*useOnlyExternalAliasing*/ false);
27582758
let parentSymbol: Symbol;
27592759

27602760
if (!accessibleSymbolChain ||
@@ -13386,7 +13386,7 @@ namespace ts {
1338613386
/// non-intrinsic elements' attributes type is the element instance type)
1338713387
function getJsxElementPropertiesName() {
1338813388
// JSX
13389-
const jsxNamespace = getGlobalSymbol(JsxNames.JSX, SymbolFlags.Namespace, /*diagnosticMessage*/undefined);
13389+
const jsxNamespace = getGlobalSymbol(JsxNames.JSX, SymbolFlags.Namespace, /*diagnosticMessage*/ undefined);
1339013390
// JSX.ElementAttributesProperty [symbol]
1339113391
const attribsPropTypeSym = jsxNamespace && getSymbol(jsxNamespace.exports, JsxNames.ElementAttributesPropertyNameContainer, SymbolFlags.Type);
1339213392
// JSX.ElementAttributesProperty [type]
@@ -15615,7 +15615,7 @@ namespace ts {
1561515615
}
1561615616

1561715617
function isCommonJsRequire(node: Node) {
15618-
if (!isRequireCall(node, /*checkArgumentIsStringLiteral*/true)) {
15618+
if (!isRequireCall(node, /*checkArgumentIsStringLiteral*/ true)) {
1561915619
return false;
1562015620
}
1562115621
// Make sure require is not a local function
@@ -17071,7 +17071,7 @@ namespace ts {
1707117071
function getTypeOfExpression(node: Expression, cache?: boolean) {
1707217072
// Optimize for the common case of a call to a function with a single non-generic call
1707317073
// signature where we can just fetch the return type without checking the arguments.
17074-
if (node.kind === SyntaxKind.CallExpression && (<CallExpression>node).expression.kind !== SyntaxKind.SuperKeyword && !isRequireCall(node, /*checkArgumentIsStringLiteral*/true)) {
17074+
if (node.kind === SyntaxKind.CallExpression && (<CallExpression>node).expression.kind !== SyntaxKind.SuperKeyword && !isRequireCall(node, /*checkArgumentIsStringLiteral*/ true)) {
1707517075
const funcType = checkNonNullExpression((<CallExpression>node).expression);
1707617076
const signature = getSingleCallSignature(funcType);
1707717077
if (signature && !signature.typeParameters) {
@@ -21859,7 +21859,7 @@ namespace ts {
2185921859
}
2186021860
else if (isTypeReferenceIdentifier(<EntityName>entityName)) {
2186121861
const meaning = (entityName.parent.kind === SyntaxKind.TypeReference || entityName.parent.kind === SyntaxKind.JSDocTypeReference) ? SymbolFlags.Type : SymbolFlags.Namespace;
21862-
return resolveEntityName(<EntityName>entityName, meaning, /*ignoreErrors*/ false, /*dontResolveAlias*/true);
21862+
return resolveEntityName(<EntityName>entityName, meaning, /*ignoreErrors*/ false, /*dontResolveAlias*/ true);
2186321863
}
2186421864
else if (entityName.parent.kind === SyntaxKind.JsxAttribute) {
2186521865
return getJsxAttributePropertySymbol(<JsxAttribute>entityName.parent);
@@ -22682,7 +22682,7 @@ namespace ts {
2268222682
? SymbolFlags.Value | SymbolFlags.ExportValue
2268322683
: SymbolFlags.Type | SymbolFlags.Namespace;
2268422684

22685-
const symbol = resolveEntityName(node, meaning, /*ignoreErrors*/true);
22685+
const symbol = resolveEntityName(node, meaning, /*ignoreErrors*/ true);
2268622686
return symbol && symbol !== unknownSymbol ? getTypeReferenceDirectivesForSymbol(symbol, meaning) : undefined;
2268722687
}
2268822688

‎src/compiler/commandLineParser.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -1170,7 +1170,7 @@ namespace ts {
11701170
const relativeDifference = convertToRelativePath(extendedDirname, basePath, getCanonicalFileName);
11711171
const updatePath: (path: string) => string = path => isRootedDiskPath(path) ? path : combinePaths(relativeDifference, path);
11721172
// Merge configs (copy the resolution stack so it is never reused between branches in potential diamond-problem scenarios)
1173-
const result = parseJsonConfigFileContent(extendedResult.config, host, extendedDirname, /*existingOptions*/undefined, getBaseFileName(extendedConfigPath), resolutionStack.concat([resolvedPath]));
1173+
const result = parseJsonConfigFileContent(extendedResult.config, host, extendedDirname, /*existingOptions*/ undefined, getBaseFileName(extendedConfigPath), resolutionStack.concat([resolvedPath]));
11741174
errors.push(...result.errors);
11751175
const [include, exclude, files] = map(["include", "exclude", "files"], key => {
11761176
if (!json[key] && extendedResult.config[key]) {

0 commit comments

Comments
 (0)