Enable ban-types lint rule #19586
Enable ban-types lint rule #19586
Conversation
ghost
mentioned this pull request
ghost
src/harness/parallel/worker.ts
Outdated
| @@ -2,7 +2,9 @@ namespace Harness.Parallel.Worker { | |||
| let errors: ErrorInfo[] = []; | |||
| let passing = 0; | |||
|
|
|||
| type Executor = {name: string, callback: Function, kind: "suite" | "test"} | never; | |||
| type MochaCallback = (this: Mocha.ITestCallbackContext, done: MochaDone) => void; | |||
There was a problem hiding this comment.
Technically, this should be an ISuiteCallbackContext for "suite" and only this context for "test". Also gives you your answer for the todo below: this callback type (as written, not corrected) would be correct there, I believe.
src/harness/parallel/worker.ts
Outdated
| (before as any) = (cb: Function) => beforeFunc = cb; | ||
| let afterFunc: Function; | ||
| (after as any) = (cb: Function) => afterFunc = cb; | ||
| let beforeFunc: () => void; |
There was a problem hiding this comment.
You sure you don't want to write type Callable = () => void somewhere and use it everywhere just to reduce the verbosity around places like here? Callback types have a lot of sigils and are visually noisy because of it.
src/harness/unittests/session.ts
Outdated
| @@ -54,7 +54,7 @@ namespace ts.server { | |||
| } | |||
|
|
|||
| // Disable sourcemap support for the duration of the test, as sourcemapping the errors generated during this test is slow and not something we care to test | |||
| let oldPrepare: Function; | |||
| let oldPrepare: {}; | |||
There was a problem hiding this comment.
While technically valid because of the any casting; this feels unfortunate.
| @@ -4455,7 +4455,7 @@ namespace ts.projectSystem { | |||
|
|
|||
| describe("cancellationToken", () => { | |||
| // Disable sourcemap support for the duration of the test, as sourcemapping the errors generated during this test is slow and not something we care to test | |||
| let oldPrepare: Function; | |||
| let oldPrepare: {}; | |||
tslint.json
Outdated
| "ban-types": { | ||
| "options": [ | ||
| ["Object", "Avoid using the `Object` type. Did you mean `object`?"], | ||
| ["Function", "Avoid using the `Function` type. Prefer a specific function type, like `() => void`."], |
There was a problem hiding this comment.
You can change this to refer to ts.AnyFunction now.
|
@mhegazy Could you review? |
src/compiler/core.ts
Outdated
| * Safer version of `Function` which should not be called. | ||
| * Every function should be assignable to this, but this should not be assignable to every function. | ||
| */ | ||
| export type AnyFunction = (...args: never[]) => {} | null | undefined | void; |
There was a problem hiding this comment.
why not use something more specific in each location, e.g. in an input position, either (...args: any[]): T if you care about input (e.g. memoize) or (...args: any[]): void if you do not care about the output (e.g. assert)?
There was a problem hiding this comment.
I think this is more specific as it can't be called (with arguments at least) and the return value can't be used without casting. If we use ...args: any[] we can pass in a function taking a number but then call it on a string and there will be no warning at compile-time.
There was a problem hiding this comment.
most of these are not called, e.g. getFunctionName, assert,, fail, etc.. and we never call them with null or void.. and the ones that accept undefined are usually marked with optional parameters..
for these it seems more correct to jut have (...args: any[])=>void or (...args: never[]) => void if you want to be strict.
Functionis unsafe as any function is assignable to it and it can be called with arbitrary arguments.There are a few places where we just need something to be a
Functionfor the purpose of having a stack trace but we don't ever actually call it.Disabled running this rule on
Symbolsince we use that to mean something else.