-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Rule suggestion: use of never
#2616
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 could definitely be added to our |
I found this when looking for something similar myself and want to try my hand at implementing it. I just want to confirm what I'm thinking of implementing and get suggestions ahead of time. I think the rule could be very simple and error on the use of Maybe if we want to get fancy the rule could have an option to allow never return values if that's something a lot of people might want. But that could also be a follow up if the need is there. For a name, this seems like it'd be similar to |
Instead of creating a new rule entirely, IMO it'd be much better to add handling for The rules are already setup to prevent unsafe operations on an |
I played around with prototyping it last night and I think the functionality I have is a bit different. It identifies any variable which is of type In this example below: const obj = "abc";
if (typeof obj === "undefined") {
const test = obj; // Both test and obj on this line are errors as they have the never type
console.log(test); // test is an error here as it has the never type
}
function neverReturn (): never { // This is fine as it's a return type and not a variable type
throw new Error("abc");
}
let num: never; // num is an error as it explicitly has the never type This seems to solve the same problem as identifying operations on Let me know if that makes sense and if not, which rule you were thinking of extending to add this functionality? (And what functionality specifically.) |
Your first case is a Your third case isn't handled - as declaring a return of type
|
I see what you mean. When I was reading the description and messages for a rule like Would you suggest rewording those existing rules to be more generic and adding this functionality to each of those? It probably wouldn't need to be an option to enable/disable since |
Definitely - the docs for the rules currently only mention It might be good to add an option to let people opt into it, but I don't think it's a requirement. I think just catching more cases is something everything will want. |
Okay sounds like a plan. I'll put out a PR hopefully today with the updated |
Assigning to function unimplemented(variant: never) {
throw Error(`Unimplemented variant: ${variant}`)
}
type Color = "red" | "green" // | "blue"
function handleColor(color: Color) {
if (color === "red") {
console.log("the color is red")
}
else if (color === "green") {
console.log("the color is green")
}
else {
unimplemented(color) // will show error if you add "blue"
}
}
If it's used properly then |
I think it's better to open an issue to TypeScript, since this invalid case should be caught by compiler (it's about type checking), not a linter. Edit: The reason why we should report to TypeScript is when there's a variable whose type is Also, assigning |
It's literally the opposite of safe. const x = 1 as never;
const y: string = x;
y.bold();
TS does ban obviously never satisfiable conditions. const x = typeof 'a';
type T = typeof x; // "string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "object" | "function"` Which means that Feel free to raise an issue with TS regarding |
Just as @phaux said, it's unsafe that you cast Assigning function unimplemented(): never {
throw new Error()
}
function doStuff(): number {
return unimplemented()
} Bottom type in Rust: fn main() {
do_stuff();
}
fn do_stuff() -> i32 {
unimplemented!()
} Bottom type in Haskell: ( main = putStrLn doStuff
doStuff :: String
doStuff = undefined |
But that's the point. What's the difference between Assigning By your logic - it's completely safe to use
In general assigning Taking your example - the correct code is to not return at all: function unimplemented(): never {
throw new Error()
}
function doStuff(): number {
unimplemented()
} Which represents safe code, because you're not assigning the Taking another example of switching on a union type: function test(x: 'a' | 'b'): number {
switch (x) {
case 'a': return 1;
case 'b': return 2;
default:
return x;
}
} This code is inherently unsafe, but it typechecks fine, because This should not happen and is a bug. function throwNever(value: never): never {
throw new Error(`Unexpected value ${value}`);
}
function test(x: 'a' | 'b'): number {
switch (x) {
case 'a': return 1;
case 'b': return 2;
default:
throwNever(x);
}
} Why? Because you're assigning a |
The only valid ways in typescript to get function a(): never {
throw new Error()
} function a (): never {
while (true) {}
} And either of them will never return anything (thus 'never') So any code after them didn't really matter, your program can't go there because it is already dead. A function that returns never is technically works like a |
That's incorrect - you've forgotten about the type narrowing case that I mentioned in my comment. function test(x: 'a' | 'b'): number {
switch (x) {
case 'a': return 1;
case 'b': return 2;
default:
x // typeof === never
}
} Which is the case that's discussed in the OP. |
Then you need a variable that typed as 'never' but actually have value to go there. (the value you passed as 'x') However you can't. The actual unsafe code is |
Again, as I mentioned in my comment. If you have front-end code which talks to a backend API, and you do not push the two in sync (which is a very common deployment strategy), then it's very easy to introduce new values on the backend without having the front-end code handle it. Just because TS says it's Types are not infallible, which is why it is not safe to assign |
I think the |
to clarify: I am suggesting that this code should be flagged: function doesSomething(arg: string) {
return arg.toUpperCase();
}
function test(x: 'a') {
switch (x) {
case 'a': return;
default: doesSomething(x);
}
} Because assigning I am not suggesting that this code should be flagged: function assertNever(arg: never) {
throw new Error(`never: ${arg}`);
}
function test(x: 'a') {
switch (x) {
case 'a': return;
default: assertNever(x);
}
} That's assigning a |
Gotcha - I would agree with you that it's generally undesirable to hit that situation. You're basically writing dead code at that point. I do still feel a bit split because even within our own codebase we've written stuff like doStuff({
someNonOptionalProperty: undefined!, // TODO
}); which is a bit less heavy-handed than |
Yeah I totally get that. In the "real world" sometimes you do need to do weird, potentially unsafe stuff to get across the line (either due to complex types, bugs in TS, stuff that needs to be done in time crunch, etc). These rules (specifically our Lint rules are better to err on the noisy side and catch all the cases (including intentional ones) rather than being quiet and accidentally letting bugs slip through to production. |
About that unreachable condition, I though A While typescript think it is still reachable after that point (I remembered that it is cause by a design decision or so that separate the control flow analysis and typing?) Probably this issue: I probably understand what are you talk about, but I think it is at most a warning instead (it is dead code) of a error as long as you validated server response before use it. Typescript assume input type is correct and never generate runtime guard for you due to their design philosophy. |
I think that code should be flagged: function assertNever(arg: never) {
throw new Error(`never: ${arg}`); // <- unsafe use of never right here
}
function test(x: 'a') {
switch (x) {
case 'a': return;
default: assertNever(x);
}
} based on the way that code is, if that function executes, the value is clearly not function assertNever(arg: never) {
throw new Error(`never: ${arg as unknown}`);
} I think the only safe ways to use a variable that is typed or narrowed to be
|
more like function assertNever(arg: never) { // <- Error: rule 1, non construable never parameter, a function that has such type can't be normally actually called.
throw new Error(`never: ${arg}`); // <- Error: rule 2, use of a never value is nonsense, do you really mean to do it?
} And also function test(x: 'a') {
switch (x) {
case 'a': return;
default: assertNever(x); // <- Error: rule 2, assertNever will not be called because a never value is not construable
}
} Because an actual Probably the secondary rule can be silent by a And still, this is not an error, at most a warning. |
I must object with the idea that function useTheValue(a: { foo: number }) {
return a.foo;
}
export function testFunc(a: string | number) {
if (typeof a === 'boolean') {
// If `a === true`, then `useTheValue` will be called with it, causing undefined behavior
// But really, calling `testFunc` with `true` is already undefined behavior:
// Calling `a.length` would already cause errors
return useTheValue(a);
}
return 0;
} This class of bug can also happen without having any function useTheValue(a: {foo: number}) {
return a.foo;
}
export function testFunc(a: {foo: number} | undefined) {
if (a) {
type A = typeof a; // {foo: number}
// if `a === true`, then the function will be called in this branch, causing undefined behavior
return useTheValue(a);
}
} The fact that When this rule is broken, however, then instancing I think that a rule that forbids the cast |
I'm sorta thinking this is an XY problem. In export function testFunc(a: string | number) {
if (typeof a === 'boolean') { This type check should already give a warning because the expression export function testFunc(a: {foo: number} | undefined) {
if (a) { Maybe there should be a lint rule like |
I don't see a single example here that is actually a problem, or something is missing. A function test(x: 'a' | 'b'): number {
switch (x) {
case 'a': return 1;
case 'b': return 2;
default:
x // typeof === never
}
} Will fail with Even if you write In case of the initial example, you won't be able to pass argument Maybe you talk about running typescript outside of |
That is objectively false. Example: function doSomethingWithString(arg: string) {
return arg.charAt(0);
}
function foo(arg: string) {
// big OOPs - I accidentally used !== instead of ===
if (typeof arg !== 'string') {
return doSomethingWithString(arg);
}
return '';
} This code typechecks completely fine, yet at runtime it will crash. Another example that happens a lot in the real world where values can come from outside JS (i.e. from an API) - it's possible to have the boundary types wrong. const result = await fetchFromApi('/api');
switch (result.type) {
case ResultType.One:
// do something
break;
// ...
default:
doSomethingWithNever(result);
} This code, much like the previous code, will typecheck fine because according to the types we would have handled all of the cases. But say that we add a new enum value to the backend and push that to production before we update the front-end... well now our values at runtime don't match the types we wrote them against - and we'll start operating on |
The code doesn't break at runtime because you never invoke it and when you invoke it you get type error, for example: function doSomethingWithString(arg: string) {
return arg.charAt(0)
}
function foo(arg: string) {
// big OOPs - I accidentally used !== instead of ===
if (typeof arg !== "string") {
return doSomethingWithString(arg)
}
return ""
}
foo("ss")
foo(200) // Argument of type 'number' is not assignable to parameter of type 'string'.ts(2345) Unless you turn off ts strict maybe invoke the plain JS wrongly, but with this argument the whole TS idea is turned upside down. Now you suddenly have to rethink all TS and start testing things that you shouldn't test because TS enforces the types.
Well this is because you've casted (as) the API response instead of actually parsing it. Which circles us back to "TS can't help you if you make assertions without enforcing it". |
Demonstration, what all the examples here are doing in some way (namespace Wrong) and where TS will never give you any runtime safety, and an example (namespace Correct) how it should properly be done: As you can see same type of problem without You won't find a general rule to lint whether and assertion is correct and safe or not, by nature. And all issues that derive from it will transitively inherit this property. |
// module.js
export function unsafe(): any {
return 1;
}
// local.ts
import {unsafe} from 'module';
foo(unsafe()); // TS check fine, looks fine as well, but crashes
It's all fine and dandy to say "you should check every single field that your API sends you to ensure that the value exactly matches the types you've defined", but the reality is that is simply not efficient or even good programming in a lot of cases, definitely not at scale. There's a tradeoff to be had and you usually need to add some degree of trust. A lot of webapps can send hundreds of API requests in minutes - that could be megabytes of data that you would need to do deep runtime-level assertions on to ensure it's completely correct. This would be horribly slow and just bad for everyone involved given that 99.9% of the time the checks will pass. As a real-world example - at Meta they use Relay to generate strict types for their API responses based on the GraphQL queries defined in code and the GraphQL types defined for the server. So how could it be possible that things are out-of-sync? Version pinning. The client's JS version gets pinned to a specific release for a period of time (a few days or so). However the server doesn't get pinned in the same way - it's always on the latest code. So there are some cases where you might make a change on the server that adds an enum value, and have that enum value be sent and processed by old JS code! The same thing happens with mobile apps - mobile apps often aren't running on the latest code and instead need to be updated via an app store. Out of date app leads to out of date code. The point I'm getting at is that yes, in a completely idealistic, perfect world - types would exactly match the runtime values and Shit happens - so helping people do the right thing by informing them of potentially unsafe code is a much better course of action than doing nothing. Case-in-point, all of our |
I'm aware libraries often explicitly define But the nature of those two problems is different. The The later problem isn't really detectable, as it has to be assumed everywhere. You have to assume there is no typing as it can always be an assertion. You basically have to write pure JS code always assuming types wrong. How do you want to lint that? It is the same with the The To me it appears such logical checks make more sense, instead of going with never. |
Something where this would be very useful is to actually find bugs in your types. During some major refactor I somehow changed
|
Repro
The following code is clearly bad, but compiles just fine, because the
typeof
condition narrows the type ofa
tonever
, allowing it to be passed to a function that clearly can't accept it. (This is distilled from a real example in a Microsoft codebase.)I apologize if it exists - but I can't find a
typescript-eslint
rule that will detect this.I think the rule I want is "disallow any use of a
never
-typed value except as an immediate operand toreturn
".The text was updated successfully, but these errors were encountered: