Skip to content
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

Enhancement: consistent-type-assertions Require satisfies with the assertion #7096

Closed
4 tasks done
ethanresnick opened this issue Jun 12, 2023 · 9 comments
Closed
4 tasks done
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on

Comments

@ethanresnick
Copy link

ethanresnick commented Jun 12, 2023

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/consistent-type-assertions/

Description

Casting a value to a different type (using as) is unsafe, but sometimes necessary. However, using satisfies to state assumptions about the value's type before the assertion, in as much detail as TS can validate, makes the cast much less dangerous as the code evolves.

As one example, suppose you have a variable x, which TS sees as a string | number, and you cast it to a string using x as string. Then, suppose the code that produces x is refactored so that x can now be a string | number | boolean. At that point, the cast is probably incorrect/unsafe, but TS won't issue any errors, and the refactor may well lead to a bug.

However, if the original cast were instead x satisfies string | number as string, then the refactor would lead to a type error, because string | number | boolean does not satisfy string | number.

Essentially, then, someVar satisfies T as T2 is a way to cast someVar from T to T2; it will fail if someVar isn't a T(!), compared to version without satisfies, which will succeed in cases where it shouldn't. Accordingly, the proposed check would require using satisfies on any expression that has a type assertion.

Of course, combining satisfies and as can also make upcasts safe, as was discussed in the issue proposing satisfies, whereas an upcast with as can accidentally be a downcast (esp. after a refactor).

This check could be a new option to the consistent-type-assertions rule, or it could be its own rule; the latter might allow introducing more customization options down the line — e.g., if we find that there are some assertions where the satisfies doesn't really add much, and lint failures would just add a lot of noise. Examples could include:

  • when the expression's type before asserting is/includes any, as casting that to something safer shouldn't require satisfies.
  • when the expression is a literal (as there's no risk of its type being/becoming unexpected)
  • when the type of the expression is part of the type assertion (e.g., const y = x as Exclude<typeof x, number>), as the user is already doing an assertion that's conservative/making minimal assumptions.
  • possibly when the expression's type before asserting is unknown, as x satisfies unknown as T might be a bit unneccessary.

As a separate rule, this could also have its own fixer, which would replace x as Y with x satisfies [insert type of x] as Y.

Fail

declare const value: string | number;

const message = `Hello ${value as string}`;

Pass

declare const value: string | number;

const message = `Hello ${value satisfies string | number as string}`;

Additional Info

No response

@ethanresnick ethanresnick added enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Jun 12, 2023
@bradzacher
Copy link
Member

but sometimes necessary

I think the cases where it is absolutely necessary are few and far between in modern TS.

What examples to you have of cases where it's "sometimes necessary"?

Then, suppose the code that produces x is refactored so that x can now be a string | number | boolean. At that point, the cast is probably incorrect/unsafe,

One could easily argue that the previous code was equally unsafe as it already relied upon a refining assertion to work (which is the reason as assertions are unsafe - they are bivariant and allow widening or refining assertions, whereas satisfies only allows widening).

And that the previous code could have been improved to be completely safe via .toString().

when the expression's type before asserting is/includes any,
... is a literal
... is unknown

In order to do any of this, the rule would require types - which the rule currently does not use. So these would be a non-starter as an addition to consitent-type-assertions


Where I'm at right now is that the style this enforces isn't really a whole lot of extra safety because there are still unsafe refining as assertions. Sure it does provide you with a TS-validated form of documentation - but these assumptions would really be better written as runtime checks to be truly safe.

The code is also super verbose and there's many cases where you probably can't properly satisfy the linter - for example if there are anonymous object types or really large anonymous unions - both of which are fairly common cases (which is the same reason many people don't like explicit function return types!).

I do see some benefits to this style to those that want it - however I think there's not going to be a whole lot of people who actively want this style due to the verbosity and difficulty naming types. From my experience at scale it's not uncommon for codebases to just allow as assertions because it's too hard to properly eliminate them as teams move fast to ship features.

For a general purpose plugin like ours I just don't see anywhere near enough of our userbase wanting to use this style. Which is why I don't think this belongs here.

That's not to say it doesn't deserve to exist, just better in a different (or entirely separate) plugin.

I'll leave this open to evaluate community engagement.

@bradzacher bradzacher added evaluating community engagement we're looking for community engagement on this issue to show that this problem is widely important and removed triage Waiting for maintainers to take a look labels Jun 12, 2023
@ethanresnick
Copy link
Author

ethanresnick commented Jun 13, 2023

@bradzacher Thanks for the quick reply — and for all your work maintaining typescript-eslint. It's been an invaluable project for all the code I've written recently, so I really appreciate it.

In terms of who this rule would be aimed at:

I agree with you that there are a lot of teams, especially larger teams with developers of varying skill levels, that just can't realistically forbid as assertions; that would slow them down too much, as some of the cases where they'd use a cast can be quite tricky to resolve properly, and the cast usually seems "safe enough". This rule wouldn't be for those teams.

Instead, I think this rule would be aimed at the teams that, today, are inclined to ban all assertions using assertionStyle: "never". These teams are opting-in to doing some extra work to convince the compiler that everything checks out, in exchange for more safety. However, even with that mindset, I don't think it's fair to say that an assertion is never the best option, and I'll show examples below where I think an assertion is the right fit (or even the only option).

So, this rule would enable teams in the "no assertion" mindset to get more safety in those rare cases where they do choose to use an assertion. It could even be combined with assertionStyle: "never"; consistent-type-assertions would then be disabled on the few lines where an assertion is needed, but this rule would still apply.

With that preface, below are some random examples where I think a type assertion makes the most sense. These are examples that only fairly sophisticated TS users would run into, with the possible exception of the Object.fromEntries example. That's to be expected, though: as you rightly point out, the need for assertions is quite rare in modern TS.

However, precisely because type assertions should be rare and discouraged, I don't think the verbosity of this style is a major problem, and is arguably even a virtue (if it nudges people on the margin towards something even safer). In the cases where an assertion is still the best option, though, then I think it's reasonable to say "we should still make the assertion as safe as possible", which is what motivated this style.

I'm reasonably confident that we could make a rule here that make teams in the 'no assertion' mindset strictly better off, and that would have a low false positive rate (by having carve outs for some of the cases mentioned in my OP, like not needing satisfies when you're asserting from any, etc).


The examples:

  1. We have a type NonEmptyArray<T> = [T, ...T]. However, we run into situations like:

    declare const x: NonEmptyArray<X>;
    declare const someFn: (it: X) => Y;
      
    x.map(someFn) // TS infers `Y[]` rather than NonEmptyArray<Y>

    I.e., the "non-emptiness" gets lost by TS during the map. In these cases, our code will do something like x.map(someFn) satisfies Y[] as NonEmptyArray<Y>. I suppose we could instead do x.map(someFn) as NonEmptyArray<ReturnType<typeof someFn>> (to protect against someFn being refactored), but that's not really shorter or less confusing, and won't always work right if someFn has overloads.

  2. The type definition for Object.fromEntries takes a parameter T for the type of the resulting object's values, but always returns an object typed as { [k: string]: T }. I.e., there's no way to define the resulting object's keys. However, we sometimes have cases where the entries are generated by mapping over some array of constants, as in:

    const x = Object.fromEntries(
      someConstants.map(k => [k, /* ...expression to make value of type Y... */] as const)
    );

    In these cases, we need x to have a more precise type than { [k: string]: Y }, so we do:

    const x = Object.fromEntries(
      ...
    ) satisfies Record<string, Y> as { [K in (typeof someConstants)[number]]: Y }

    At the place in our code where I found this, Y was a shared object type serving as a kind of "interface"/contract, so this check was making sure that the object literal produced in the map() call actually satisfied the interface. If we just had the as without the satisfies, I think we could accidentally omit a required property, especially if/when Y is updated.

  3. The safe upcast use case, mentioned in the issues that lead to satisfies being added, is one we have as well. In our case, we use an upcast to keep certain implementation details private. Specifically, we have "recipes", each of which is represented by an array of "step" objects. However, the particular steps in a recipe is an implementation detail. So, when we define a recipe, we do:

    type Recipe = ReadonlyDeep<NonEmptyArray<Step>>;
    
    export const recipes = {
      NEW_RECIPE_X: [
        { /*.. object for step 1... */},
        { /*.. object for step 2... */}, 
        /* etc */
      ] satisfies Recipe as Recipe
    } satisfies Record<string, Recipe>;

    With as Recipe instead of satisfies Recipe as Recipe, the type checking gets much less precise on the Step objects (esp. for literal strings inside them). With as const on each recipe's array of steps, we end up leaking the details of the recipe's steps.

  4. Typescript has certain (longstanding) limitations in how it reasons about union types — see, e.g., Distribute property type union members to create union of object types microsoft/TypeScript#36969. We have a lot of fairly complex union types and so often run into these issues. The only way to solve them is with a cast, but we want to make the cast as minimal as possible, and satisfies lets us do that. Specifically, for the example shown in that linked issue:

    type Test =
      | { b: [false, "a"] }
      | { b: [true, "b"] }
    
    declare const b2: boolean;
    const a: Test = { b: b2 ? [b2, "b"] : [b2, "a"] }

    The assignment to a will fail as written, because of the TS limitation. So, we instead do something like:

     const a = { b: b2 ? [b2, "b"] : [b2, "a"] } satisfies Pick<Test, keyof Test> as Test

    In that example, the satisfies adds a lot more safety than just asserting as Test directly. The Pick just forces TS to "collapse" the two union cases, in a way that enables the compiler to see that the assignment is safe. So, again, the pattern is "use satisfies to encode as much detail as the compiler can validate, to make sure that you're only casting the bits that you really have to".

@bradzacher
Copy link
Member

  1. We have a type NonEmptyArray = [T, ...T]. However, we run into situations [where] the "non-emptiness" gets lost by TS during the map.

You can augment the TS types to provide a definition for NonEmptyArray so TS understands this which would avoid the need for a cast at all.

You can do this by patching TS, by declaring an ambient global declaration in your workspace, or by using a "node modules lib def" to override the inbuilt TS types.

@bradzacher
Copy link
Member

  1. The type definition for Object.fromEntries takes a parameter T for the type of the resulting object's values, but always returns an object typed as { [k: string]: T }.

Similar to before - you can avoid the need for an assertion everywhere by re-typing Object.fromEntries.
This overload is a bit more difficult because it's a static method so you can't quite control which overload TS would pick - but instead you can use a local override with a better type for your usecase.

Again - no as there, yet correctly types the output. You can even make a simple module for this to make it easy to consume anywhere in the codebase.

The only case that you can't handle here is if you want to map from a tuple to a tuple for the entires because TS doesn't provide any way to map from a tuple to a tuple, and I don't believe it provides any tooling to "generate" tuple types of indeterminate lengths.
I doubt you're writing code like someConstants = [ dozens of constants ]; someConstants.map(..) as [['a', valuea], ['b', valueb], ...] because if you're writing the type out explicitly then you would just ditch the .map and do as const instead...

@bradzacher
Copy link
Member

  1. the safe upcast usecase

I'm not sure I understand why you specifically need an as there - satisfies will validate that the object can be assigned to Recipe - at that point AFAIK the as only really "renames" the anonymous object type to explicitly be Recipe - which may be desirable in some cases - but I'd assume most cases doesn't really matter.

You could also completely remove the need for the as with a slight change to the code and an extra assignment to use an explicit (safe) annotation..

Or you could similarly remove the need for both an as and a satisfies by extracting the keys into a separate type declaration.

Both options don't need an as

@bradzacher
Copy link
Member

  1. distribution of union types

This does look like a difficult one given you're butting up against a limitation of TS.
Though I would question why you're using non-discriminated object unions instead of discriminated once - as the latter is much better supported and much easier to handle.

But this is definitely a rare edge case as evidenced by the fact that there's just 2 reactions to the issue and it's been open for 3 years.

If your codebase is commonly hitting this usecase and requiring an as here then I'd probably be taking a step back and asking the question "why are we doing things so much differently to the rest of the TS community?" and consider ways to refactor away from the problem. But I don't know your codebase - so I obviously can't comment on the feasibility of such a change, so I can only speak speculatively.

@ethanresnick
Copy link
Author

ethanresnick commented Jun 13, 2023

@bradzacher For my first example, augmenting the built-in types as you suggest is a great solution. I will use that!

For my second example, I don't think the fromTupleEntries function you made is quite safe; the issue is that the array of entries at runtime can be missing entries for some of the constituents of the K union type. E.g.:

const arr: Array<'a' | 'b' | 'c'>  = ['a'];
const x = fromTupleEntries(arr.map((k) => [k, 'foo']));

In the above, TS will think that x.b and x.c exist, even though they don't! So, the cast in my original example was only safe because it's clear that the union type given by (typeof someConstants)[number] can't contain any strings except those actually in someConstants at runtime. But that's a determination that seems like it has to be made at each call site in the code (not in a separate function like fromTupleEntries.)

For the third example, the point of the as is to hide which steps are in each recipe; see here. Your first approach does achieve that, but I find the extra constant kinda inelegant; in a case where I needed to safe upcast only one item (as opposed to n recipes) I would definitely want to use satisfies X as X, rather than making a dummy variable. (Your second solution in that case doesn't enforce that there's a recipe defined for every RecipeNames.)

For the fourth example, all I can say is that these kind of "it's correct but TS can't prove it" cases do come up from time to time, and they create a class of cases where a type assertion is appropriate, no matter how strict a team wants to be. Also, here's a version of the fourth example to show, more realistically (and with better names) how that union type issue can manifest.

That said, I'm not sure exactly what you're getting at with your comments. Are you trying to argue that type assertions are never the best option, or just that they should be rare? (Or maybe you're just trying to improve my code, which I appreciate! 😁) If you think they should be rare, then I totally agree, but I still think this rule would improve them in the few places they are used.

@ethanresnick
Copy link
Author

Btw, I did just try to add the overload you suggested for example 1, but it didn't work, unfortunately 😢 The issue seems to have to do with TS' current limitations around unions of call signatures; see the error here, which goes away without the augmented definition.

@bradzacher
Copy link
Member

This issue has been open for 5 months now and it has not garnered any additional reactions or comments from other users.

We take this as signal that it's not a request that is important for the community and as such we are going to reject the proposal for this project.
If this is important to you consider using our tooling to publish a plugin for this rule!

Thanks!

@bradzacher bradzacher closed this as not planned Won't fix, can't repro, duplicate, stale Nov 13, 2023
@bradzacher bradzacher added wontfix This will not be worked on and removed evaluating community engagement we're looking for community engagement on this issue to show that this problem is widely important labels Nov 13, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants