Tslint: Analysis: Determine which tslint-microsoft-contrib rules should be moved to tslint

[HamletDRC]

I'd like to know which of the tslint-microsoft-contrib rules you'd like me to merge back into the tslint product. We can discuss the rules here and then I can later create an issue and pull request for each of the rules we want to port.

NOTE: Answer has been edited to incorporate the feedback in comments.

Rules to Merge to TS Lint

| Row # | Rule Name | Description |
| :-- | :-- | :-- |
| 4 | max-func-body-length | Avoid long functions. The line count of a function body must not exceed the value configured within this rule's options.
You can setup a general max function body length applied for every function/method/arrow function e.g. [true, 30] or set different maximum length for every type e.g. [true, { "func-body-length": 10 , "arrow-body-length": 5, "method-body-length": 15, "ctor-body-length": 5 }]. To specify a function name whose parameters you can ignore for this rule, pass a regular expression as a string(this can be useful for Mocha users to ignore the describe() function) |
| 21 | no-for-in | Avoid use of for-in statements. They can be replaced by Object.keys |
| 22 | no-missing-visibility-modifiers | Class members (both fields and methods) should have visibility modifiers specified. THe Principle of Least Visibility guides us to prefer private methods and fields when possible. If a developer forgets to add a modifier then TypeScript assumes the element should be public, which is the wrong default choice. |
| 28 | no-unnecessary-bind | Do not bind 'this' as the context for a function literal or lambda expression. If you bind 'this' as the context to a function literal, then you should just use a lambda without the bind. If you bind 'this' as the context to a lambda, then you can remove the bind call because 'this' is already the context for lambdas. Works for Underscore methods as well. |
| 29 | no-with-statement | Do not use with statements. Assign the item to a new variable instead |

Rules to Discuss Further

| Row # | Rule Name | Description |
| :-- | :-- | :-- |
| 5 | missing-jsdoc | All files must have a top level JSDoc comment. A JSDoc comment starts with /** (not one more or one less asterisk) and a JSDoc at the 'top-level' appears without leading spaces. Trailing spaces are acceptable but not recommended. |
| 9 | no-banned-terms | Do not use banned terms: caller, callee, eval, arguments. These terms refer to functions or properties that should not be used, so it is best practice to simply avoid them. |
| 11 | no-delete-expression | Do not delete expressions. Only properties should be deleted |
| 15 | no-empty-interfaces | Do not use empty interfaces. They are compile-time only artifacts and they serve no useful purpose |
| 17 | no-function-constructor-with-string-args | Do not use the version of the Function constructor that accepts a string argument to define the body of the function |
| 18 | no-function-expression | Do not use function expressions; use arrow functions (lambdas) instead. In general, lambdas are simpler to use and avoid the confusion about what the 'this' references points to. Function expressions that contain a 'this' reference are allowed and will not create a failure. |
| 20 | no-increment-decrement | Avoid use of increment and decrement operators particularly as part of complicated expressions |
| 23 | no-multiline-string | Do not declare multiline strings |
| 25 | no-octal-literal | Do not use octal literals or escaped octal sequences |
| 30 | prefer-array-literal | Use array literal syntax when declaring or instantiating array types. For example, prefer the Javascript from of string[] to the TypeScript form Array. Prefer '[]' to 'new Array()'. Prefer '[4, 5]' to 'new Array(4,5)'. Prefer '[undefined, undefined]' to 'new Array(4)'. |
| 31 | promise-must-complete | When a Promise instance is created, then either the reject() or resolve() parameter must be called on it within all code branches in the scope. For more examples see the feature request. |

Rules that Remain in Contrib

| Row # | Rule Name | Description |
| :-- | :-- | :-- |
| 1 | chai-vague-errors | Avoid Chai assertions that result in vague errors. For example, asserting expect(something).to.be.true will result in the failure message "Expected true received false". This is a vague error message that does not reveal the underlying problem. It is especially vague in TypeScript because stack trace line numbers often do not match the source code. A better pattern to follow is the xUnit Patterns Assertion Message pattern. The previous code sample could be better written as expect(something).to.equal(true, 'expected something to have occurred'); |
| 2 | export-name | The name of the exported module must match the filename of the source file. This is case-sensitive but ignores file extension. Since version 1.0, this rule takes a list of regular expressions as a parameter. Any export name matching that regular expression will be ignored. For example, to allow an exported name like myChartOptions, then configure the rule like this: "export-name": [true, "myChartOptionsg"] |
| 3 | jquery-deferred-must-complete | When a JQuery Deferred instance is created, then either reject() or resolve() must be called on it within all code branches in the scope. For more examples see the feature request. |
| 6 | missing-optional-annotation | A parameter that follows one or more parameters marked as optional is not itself marked optional |
| 7 | mocha-avoid-only | Do not invoke Mocha's describe.only or it.only functions. These functions are useful ways to run a single unit test or a single test case during your build, but please be careful to not push these methods calls to your version control repositiory because it will turn off any of the other tests. |
| 8 | no-backbone-get-set-outside-model | Avoid using model.get('x') and model.set('x', value) Backbone accessors outside of the owning model. This breaks type safety and you should define getters and setters for your attributes instead. |
| 10 | no-cookies | Do not use cookies |
| 12 | no-disable-auto-sanitization | Do not disable auto-sanitization of HTML because this opens up your page to an XSS attack. Specifically, do not use the execUnsafeLocalFunction or setInnerHTMLUnsafe functions. |
| 13 | no-document-write | Do not use document.write |
| 14 | no-duplicate-parameter-names | Do not write functions or methods with duplicate parameter names |
| 16 | no-exec-script | Do not use the execScript functions |
| 19 | no-http-string | Do not use strings that start with 'http:'. URL strings should start with 'https:'. Http strings can be a security problem and indicator that your software may suffer from cookie-stealing attacks. Since version 1.0, this rule takes a list of regular expressions as a parameter. Any string matching that regular expression will be ignored. For example, to allow http connections to example.com and examples.com, configure your rule like this: "no-http-string": [true, "http://www.example.com/?.", "http://www.examples.com/?."] |
| 24 | no-multiple-var-decl | Do not use comma separated variable declarations |
| 26 | no-reserved-keywords | Do not use reserved keywords as names of local variables, fields, functions, or other identifiers. |
| 27 | no-unnecessary-semicolons | Remove unnecessary semicolons |
| 32 | use-named-parameter | Do not reference the arguments object by numerical index; instead, use a named parameter. This rule is similar to JSLint's Use a named parameter rule. |

[jkillian]

Thanks for posting this @HamletDRC! I probably won't get to make a response until next week, but it's on my todo list

[jkillian]

I think one guiding principle here will be that TSLint won't include library specific rules, which would rules things like 1 out. Here's a quick list of what I'm thinking for each rule:

1) no; library specific
2) no; not convinced this fits well with ES6-style code we want to encourage
3) no; library specific
4) yes;
5) unsure; should all classes have JS-doc? why only files?
6) no; enforced by compiler already
7) no; library specific
8) no; library specific
9) unsure; some overlap with current TSLint rules
10) no; platform specific
11) unsure
12) no; platform specific
13) no; platform specific
14) no; enforced by compiler
15) unsure;
16) no; platform specific
17) unsure; seems useful but does anyone use new Function() in the first place?
18) unsure;
19) no; platform specific
20) unsure; Does this ban ++ and -- all together? Or only when used in a complex expression?
21) yes;
22) yes;
23) unsure; not sure on the rationale, what this allows/disallows
24) yes;
25) unsure;
26) no; already part of TSLint variable-name rule
27) no; already part of TSLint semicolon rule
28) yes;
29) yes;
30) unsure; like it in general, but new Array(arrayLength) should be valid I think
31) unsure
32) no; using arguments at all in TS should be discouraged I'd say

A lot of these sound like good rules, just not suitable as ones built-in to TSLint, but perfect for a package of custom rules as they are in now.

@adidahiya What do you think about the "unsure"/"yes" rules?

[jkillian]

Relevant to rule 24, looks like there's now a PR for it: https://github.com/palantir/tslint/pull/1191

[HamletDRC]

So we are left with these rules needing a decision:

• 5 - missing-jsdoc - Every 'thing' must be documented at least a little. Whether it is a module or class or file with a few things in it.
• 9 - no-banned-terms - The is specific to the Microsoft SDL and you probably don't want it.
• 11 - no-delete-expression - see https://jslinterrors.com/only-properties-should-be-deleted
• 15 - no-empty-interfaces - What is the point of an empty interface? What can yo udo with it?
• 17 - no-function-constructor-with-string-args - This rule has many false positives because the TypeChecker in TS Lint does not give you correct information and often throws errors. I would leave this out until the "Lint whole program" issue is fixed.
• 18 - no-function-expression - need your input
• 20 - no-increment-decrement - yes it bans ++ and -- altogether. It is a rather popular rule actually.
• 23 - no-multiline-string - this disallows all multi-line strings. The rationale is that it is an overrused and overvalued feature and brings many issues with carriage returns and whitespace.
• 25 - no-octal-literal - This is a MS SDL recommendation.
• 30 - prefer-array-literal - yes this even bans new Array(4) which does have purpose but is rarely used.
• 31 - promise-must-complete - need your input

Also, 27 (no-unnecessary-semicolons) is not covered by the tslint semi-colon rule as far as I can tell.

[JeremyCarlsten]

What exactly would you mean by "stay in tslint-microsoft-contrib" are you saying that we should run two different linters on our codebase, or replace tslint all together?

If not then I would say that we(you) should include no-unnecessary-semicolons.

I'm working on converting an existing project to TS and I'm doing so on a file-by-file basis.
That means I'm running my TS compiled code through the existing jshint process that thankfully caught the unnecessary semicolons on my new ts class.
But... I was then perplexed as to why tslint didn't do this.

[jkillian]

tslint-microsoft-contrib is a package of extra rules that can be run by TSLint to check your TS code. This issue is just about moving rules from the external tslint-microsoft-contrib package to the core TSLint package

[andy-ms]

no-empty-interfaces: Great minds think alike. #1889

[JoshuaKGoldberg]

If it helps, no-self-this is now no-this-assignment in TSLint itself.

https://github.com/palantir/tslint/issues/2861

[JoshuaKGoldberg]

no-function-constructor-with-string-args can now done as an optionally typed rule: without type info it only flags string literals. It should probably also include setInterval and setTimeout, with a config to add other built-in functions that allow strings as functions.

Here are a few more rules to propose to be added:

1. no-empty-line-after-opening-brace
2. no-regex-spaces
3. no-single-line-block-comment
4. no-typeof-undefined
5. no-useless-files
6. non-literal-require

Also, all the React-based rules should probably be triaged/moved to tslint-react.

[ajafff]

Has anyone on this thread thought about the licensing stuff?
You need everyone who changed the rule to agree with the relicensing as Apache 2.0 and the patent grant to Palantir and the TSLint project that is required by Apache 2.0.
Otherwise TSLint needs to explicitly note which parts of the project are licensed as MIT.

[JoshuaKGoldberg]

@ajafff I'm hazy on the exact legal practices here, but would it be enough if I took responsibility for the rules and submitted newly-written-from-scratch equivalents for all of them? It should be fairly quick.

[ajafff]

@JoshuaKGoldberg I guess that could work. Though it depends on how much the new implementation differs from the original.

If it was Apache 2.0 you were required to keep the copyright notice, even if you have completely rewritten all code that originally had the Apache 2.0 license. MIT doesn't seem to require this, it just requires the notice in "copies or substantial portions of the Software".

[JoshuaKGoldberg]

Good news! I reviewed this use case with the legal representatives on the Microsoft side, and there are no legal blockers on our end. I'll send some pull requests. ❤

In more detail: for Microsoft devs who've already been cleared to contribute to TSLint, because the tslint-microsoft-contrib code was already released on open source _and_ is not relevant to business&IP-related patents, we're cleared to redistribute and/or contribute it under another organization's project.

[Rycochet]

Adding a comment for import-name here, which isn't listed.

Personally I'd rather see the internal variable-name get extended to enforce those rules against the import names (ie, default and as <name>). The import-name rules do have their own way of doing things that ends up matching the variable rules, but you need to ensure you've done them right, so maybe bringing it over, and adding another option of "import-name": [true, "variable-name"] might be worth doing to automatically map the rules across?

[JoshuaKGoldberg]

Per #4534, this is no longer relevant. https://typescript-eslint.io will receive rule donations instead. 🚀