-
Notifications
You must be signed in to change notification settings - Fork 889
[new-rule] no-restricted-globals #3824
Changes from 3 commits
eedabef
262b7ba
614353a
7074399
049688e
ea5c8de
78bf5eb
bc99384
c018aaf
0d4ede8
109dfa6
1bcd788
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
/** | ||
* @license | ||
* Copyright 2014 Palantir Technologies, Inc. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
import * as ts from "typescript"; | ||
|
||
import * as Lint from "../index"; | ||
|
||
export class Rule extends Lint.Rules.TypedRule { | ||
/* tslint:disable:object-literal-sort-keys */ | ||
public static metadata: Lint.IRuleMetadata = { | ||
ruleName: "no-restricted-globals", | ||
description: "Disallow specific global variables.", | ||
descriptionDetails: Lint.Utils.dedent` | ||
Disallowing usage of specific global variables can be useful if you want to allow | ||
a set of global variables by enabling an environment, but still want to disallow | ||
some of those. For instance, early Internet Explorer versions exposed the current | ||
DOM event as a global variable event, but using this variable has been considered | ||
as a bad practice for a long time. Restricting this will make sure this variable | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: just "considered a bad practice" |
||
isn’t used in browser code. | ||
`, | ||
optionsDescription: "This rule takes a list of strings, where each string is a global to be restricted.", | ||
options: { | ||
type: "list", | ||
items: {type: "string"}, | ||
}, | ||
optionExamples: [ | ||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
[ | ||
true, | ||
"name", | ||
"length", | ||
"event", | ||
], | ||
], | ||
type: "functionality", | ||
typescriptOnly: false, | ||
requiresTypeInfo: true, | ||
}; | ||
/* tslint:enable:object-literal-sort-keys */ | ||
|
||
public static FAILURE_STRING(name: string) { | ||
return `Unexpected global variable '${name}'. Use local parameter instead.`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or a local variable, right?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. thanks |
||
} | ||
|
||
public applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): Lint.RuleFailure[] { | ||
const bannedGlobals = new Set(this.ruleArguments as string[]); | ||
return this.applyWithFunction(sourceFile, walk, bannedGlobals, program.getTypeChecker()); | ||
} | ||
} | ||
|
||
function walk(ctx: Lint.WalkContext<Set<string>>, checker: ts.TypeChecker): void { | ||
return ts.forEachChild(ctx.sourceFile, function recur(node: ts.Node): void { | ||
switch (node.kind) { | ||
Zzzen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
case ts.SyntaxKind.TypeReference: | ||
// Ignore types. | ||
return; | ||
case ts.SyntaxKind.PropertyAccessExpression: | ||
// Ignore `y` in `x.y`, but recurse to `x`. | ||
return recur((node as ts.PropertyAccessExpression).expression); | ||
case ts.SyntaxKind.Identifier: | ||
return checkIdentifier(node as ts.Identifier); | ||
default: | ||
Zzzen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return ts.forEachChild(node, recur); | ||
} | ||
}); | ||
|
||
function checkIdentifier(node: ts.Identifier): void { | ||
if (!ctx.options.has(node.text)) { | ||
return; | ||
} | ||
|
||
const symbol = checker.getSymbolAtLocation(node); | ||
const declarations = symbol === undefined ? undefined : symbol.declarations; | ||
if (declarations === undefined || declarations.length === 0) { | ||
return; | ||
} | ||
|
||
const declaredInLibDom = declarations.some((decl) => decl.getSourceFile().fileName.endsWith(".d.ts")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if you declare something in an Or what about a project that uses Maybe the rule should get the list of files TS is compiling with (can we do that?) along with its compiler options, and add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, custom definition files should be considered the same as official |
||
|
||
if (declaredInLibDom) { | ||
ctx.addFailureAtNode(node, Rule.FAILURE_STRING(node.text)); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
function foo(evt: Event) { | ||
let length: number = 1; | ||
console.log(length); | ||
|
||
Event.target; | ||
|
||
event.target; | ||
~~~~~ [Unexpected global variable 'event'. Use local parameter instead.] | ||
} | ||
|
||
console.log(length); | ||
~~~~~~ [Unexpected global variable 'length'. Use local parameter instead.] | ||
|
||
import { name } from "./foo"; | ||
console.log(name); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"compilerOptions": { | ||
"module": "commonjs", | ||
"lib": [ | ||
"dom" | ||
] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
{ | ||
"rules": { | ||
"no-restricted-globals": [ | ||
Zzzen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
true, | ||
"name", | ||
"length", | ||
"event" | ||
] | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: escape the "event" in "global variable event" with code to make it a little more clear.