-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
no-unsafe-call: ban calling Function #9108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
This would probably be better as a part of |
I think I'm ok with that, too. After a little experimentation i think that's safe as far as passing the narrowed function, too, since declare const looselyTypedFunction: Function;
// TS error, not assignable.
const strictlyTypedFunction: () => void = looselyTypedFunction;
// ditto for passing as a parameter to functions
declare function callsWithAConcreteCallSignature(f: () => void): void;
// TS error
callsWithAConcreteCallSignature(looselyTypedFunction) So I think |
I guess the downside is just that I could see wanting |
💡🧠 completely different direction: what if we instead made a rule like
|
The problem with doing that is that it requires type info to handle the typeof usage.
|
So is our conclusion to build this as a part of |
My hot take would be to just put both the existing behavior of |
I don't think there's a case where you want to be unsafe in every case. What kind of codebase would say "yes I always want to allow unsafely calling I think no option is fine - people can use a disable comment to opt-out on a case-by-case basis. |
100% agree.
However, I don't think this conclusion follows. It does make sense to want to allow calling |
I don't see how |
I'd agree. I don't see why you'd ban one without the other. Both are very clear and present vectors for unsafety and they are really the same thing. |
I agree that there is zero difference in safety. But, there is a difference in ergonomics and developer intention. Banning using |
My thinking would be this: most people are going to just enable the rule with the defaults and do no more - we have learned that the majority of users don't want to read docs and granularly configure things and instead want sensible defaults. So adding an option would likely be unused as we'd default it to true. Most users wouldn't read the docs and decide they want the rule on but with just the Function check only. I definitely understand your reasoning - but I think that it's just not going to be a usecase people want (and thus unnecessary branching for us). I'm not saying we never add an option! If people ask for it then we should. But I think shipping without it is completely okay. |
Ah, gotcha. I fully agree with this! 🙂 |
Is there any way to narrow I have an unknown value I used to do: if (typeof schema !== 'object' || schema === null || !('parse' in schema) || typeof schema.parse !== 'function') throw new Error(`Expected a Zod schema`);
try {
schema.parse(myData);
} This narrows Maybe the docs for the |
@controversial Interesting question. JS doesn't really provide any way to know something is safely callable at runtime, so I'm not sure of a "safe" way to narrow Consider class Foo {};
typeof Foo; // returns 'function'
Foo.length; // returns 0
// so I should be safe to call this with 0 args, right?
Foo(); // Uncaught TypeError: Class constructor Foo cannot be invoked without 'new' With that in mind, the following will pass your checks, but still cause a runtime exception const schema = { parse: Foo }; I think your best bet is to use eslint-disable comments and/or type assertions and surround stuff in try/catch 🤷 |
I think this'd be a good note for the docs, yeah. If anybody wants to file an issue and/or send a PR, that's a 👍 from me! |
Before You File a Proposal Please Confirm You Have Done The Following...
My proposal is suitable for this project
Description
Unintuitively, TS narrows
typeof x === 'function'
to the unsafeFunction
type if x is not already constrained. However, it is safe to use on a constrained type. We should report on unconstrained callables.Fail Cases
Pass Cases
Additional Info
it's probably fine/good to allow
typeof x === 'function
as long asx
isn't called. I'm not sure exactly where the line should be drawn between reporting and not reporting. It seems like it should be reported if we passx
to a function that might expect a specific call signature. Maybe a heuristic could betypeof x === 'function
is ok as long asx
isn't referenced in thetrue
branch of the conditional?The text was updated successfully, but these errors were encountered: