eslint-plugin-total-functions icon indicating copy to clipboard operation
eslint-plugin-total-functions copied to clipboard

Published 20 hours ago • verified publisher icon danielnixon

[no-unsafe-readonly-mutable-assignment] takes a long time/hangs in some corner cases

Open willheslam opened this issue 4 years ago • 7 comments

Fantastic project, really made a lot of otherwise slightly scary TS much more bulletproof. :)

I tried updating a medium-sized project to a version of this plugin post https://github.com/danielnixon/eslint-plugin-total-functions/issues/128 and it started to hang, then after several minutes a completely different eslint rule would report errors - specifically, strict boolean expressions.

After realising a lot of the functions in unsafe-assignment-rule.ts were being called sometimes hundreds of thousands of times, I tried implementing some general optimisations: https://github.com/danielnixon/eslint-plugin-total-functions/compare/master...willheslam:unsafe-assignment-optimisations

mainly by hoisting closures, lazily computing values, and using conditional blocks to return early and avoid extra work - mainly replacing beautiful functional code with clunkier, imperative and mutative code where it might help speed things up.

It didn't help! Eventually I found the problem and extracted into a smallish reproducible case using styled-components: export default styled as ThemedStyledInterface<Theme>

https://github.com/willheslam/eslint-plugin-total-functions-hang/blob/master/src/problem.ts

This causes the TSAsExpression case in unsafe-assignment-rule.ts to recur inside isUnsafeAssignment a huge number of times, until it eventually hangs on line 68: https://github.com/danielnixon/eslint-plugin-total-functions/blob/master/src/rules/unsafe-assignment-rule.ts#L68

const isSignatureAssignable = (
   ...
): boolean => {
  const returnTypeIsAssignable = checker.isTypeAssignableTo(
    sourceSignature.getReturnType(),
    destinationSignature.getReturnType()
  );

It either hangs infinitely and eslint gives up, or eventually returns - not quite sure, but I'm guessing the former due to the weird interaction with another eslint rule reporting errors in another part of the codebase.

I'm not sure why it hangs, but several other checks across the codebase were also reasonably slow (a few hundred ms), so I came up with a generalised workaround: convert most of the functions into generators (with several yield falses scattered around for good measure), allowing the rules to exit with a message if it's taking too long to process a given node:

commit: https://github.com/willheslam/eslint-plugin-total-functions/commit/c757158b5c99f35172c42b86c556495356bf0e7a

work around repo

Applying this workaround to the reproducible repo gives:

/eslint-plugin-total-functions-hang/src/problem.ts
  10:16  warning  Took too long to process - consider disabling for this line  total-functions/no-unsafe-readonly-mutable-assignment

✖ 1 problem (0 errors, 1 warning)

This is pretty heavy handed and I don't expect this to be merged as-is, but I'm wondering what you think:

is this just a bug that can be fixed, or a consequence of a pretty complicated library like styled-components behaving pathologically with unsafe-assignment-rule's recursive nature?

If it's the latter, is the generator approach a reasonable workaround, and if so, could it potentially be applied more sparingly, e.g. only in isUnsafeAssignment?

Thanks, and have a great weekend!

willheslam avatar Nov 20 '20 04:11 willheslam

Your workaround is very clever. I'm not against merging it.

is this just a bug that can be fixed

I hope so 🤔

or a consequence of a pretty complicated library like styled-components behaving pathologically with unsafe-assignment-rule's recursive nature

I hope not...

I'm not familiar with styled-components but I'll take a look at your reproducer (thanks for that) and see what I can see.

Another idea is introducing a user configurable whitelist of types to ignore. You could then add the styled-components type(s) to that list in your eslint config file.

danielnixon avatar Nov 20 '20 21:11 danielnixon

I've had a quick look at this but I can't immediately see why the recursion isn't terminating. I suspect (without any real evidence) a bug in my seenTypes / nextSeenTypes logic that exists to bail out when we've encountered a recursive type.

danielnixon avatar Nov 20 '20 23:11 danielnixon

Adding this to isUnsafeAssignment gives a clue as to what's going on:

console.log(checker.typeToString(rawDestinationType));
console.log(checker.typeToString(rawSourceType));
console.log("seenTypes", seenTypes.length);

ThemedStyledInterface<Theme>
ThemedStyledInterface<DefaultTheme>
seenTypes 0
ThemedStyledFunction<"a", Theme, {}, never>
ThemedStyledFunction<"a", any, {}, never>
seenTypes 3
<U, NewA extends Partial<Pick<DetailedHTMLProps<AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement>, "color" | ... 262 more ... | "referrerPolicy"> & { ...; } & U> & { ...; } = {}>(attrs: Attrs<...>) => ThemedStyledFunction<...>
<U, NewA extends Partial<Pick<DetailedHTMLProps<AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement>, "color" | ... 262 more ... | "referrerPolicy"> & { ...; } & U> & { ...; } = {}>(attrs: Attrs<...>) => ThemedStyledFunction<...>
seenTypes 6
<Props extends {} = {}>(config: StyledConfig<Pick<DetailedHTMLProps<AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement>, "color" | ... 262 more ... | "referrerPolicy"> & { ...; } & Props>) => ThemedStyledFunction<...>
<Props extends {} = {}>(config: StyledConfig<Pick<DetailedHTMLProps<AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement>, "color" | ... 262 more ... | "referrerPolicy"> & { ...; } & Props>) => ThemedStyledFunction<...>
seenTypes 6
StyledComponent<"a", Theme, {}, never>
StyledComponent<"a", any, {}, never>
seenTypes 6
{ <WithC extends AnyStyledComponent>(component: WithC): StyledComponent<StyledComponentInnerComponent<WithC>, Theme, {} & StyledComponentInnerOtherProps<WithC>, StyledComponentInnerAttrs<...>>; <WithC extends "symbol" | ... 175 more ... | FunctionComponent<...>>(component: WithC): StyledComponent<...>; }
{ <WithC extends AnyStyledComponent>(component: WithC): StyledComponent<StyledComponentInnerComponent<WithC>, any, {} & StyledComponentInnerOtherProps<WithC>, StyledComponentInnerAttrs<...>>; <WithC extends "symbol" | ... 175 more ... | FunctionComponent<...>>(component: WithC): StyledComponent<...>; }
seenTypes 9
StyledComponent<StyledComponentInnerComponent<WithC>, Theme, {} & StyledComponentInnerOtherProps<WithC>, StyledComponentInnerAttrs<...>>
StyledComponent<StyledComponentInnerComponent<WithC>, any, {} & StyledComponentInnerOtherProps<WithC>, StyledComponentInnerAttrs<...>>
seenTypes 12
{ <WithC extends AnyStyledComponent>(component: WithC): StyledComponent<StyledComponentInnerComponent<WithC>, Theme, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | StyledComponentInnerAttrs<...>>; <WithC extends "symbol" | ... 175 more ... | Functio...
{ <WithC extends AnyStyledComponent>(component: WithC): StyledComponent<StyledComponentInnerComponent<WithC>, any, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | StyledComponentInnerAttrs<...>>; <WithC extends "symbol" | ... 175 more ... | FunctionC...
seenTypes 15
StyledComponent<StyledComponentInnerComponent<WithC>, Theme, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | StyledComponentInnerAttrs<...>>
StyledComponent<StyledComponentInnerComponent<WithC>, any, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | StyledComponentInnerAttrs<...>>
seenTypes 18
{ <WithC extends AnyStyledComponent>(component: WithC): StyledComponent<StyledComponentInnerComponent<WithC>, Theme, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 1 more ... | StyledComponentInnerAttrs<...>...
{ <WithC extends AnyStyledComponent>(component: WithC): StyledComponent<StyledComponentInnerComponent<WithC>, any, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 1 more ... | StyledComponentInnerAttrs<...>>;...
seenTypes 21
StyledComponent<StyledComponentInnerComponent<WithC>, Theme, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 1 more ... | StyledComponentInnerAttrs<...>>
StyledComponent<StyledComponentInnerComponent<WithC>, any, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 1 more ... | StyledComponentInnerAttrs<...>>
seenTypes 24
{ <WithC extends AnyStyledComponent>(component: WithC): StyledComponent<StyledComponentInnerComponent<WithC>, Theme, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 2 mor...
{ <WithC extends AnyStyledComponent>(component: WithC): StyledComponent<StyledComponentInnerComponent<WithC>, any, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 2 more ...
seenTypes 27
StyledComponent<StyledComponentInnerComponent<WithC>, Theme, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 2 more ... | StyledComponentInnerAttrs<...>>
StyledComponent<StyledComponentInnerComponent<WithC>, any, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 2 more ... | StyledComponentInnerAttrs<...>>
seenTypes 30
{ <WithC extends AnyStyledComponent>(component: WithC): StyledComponent<StyledComponentInnerComponent<WithC>, Theme, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...>, Styl...
{ <WithC extends AnyStyledComponent>(component: WithC): StyledComponent<StyledComponentInnerComponent<WithC>, any, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...>, Styled...
seenTypes 33
StyledComponent<StyledComponentInnerComponent<WithC>, Theme, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 3 more ... | StyledComp...
StyledComponent<StyledComponentInnerComponent<WithC>, any, {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...> & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 3 more ... | StyledCompon...
seenTypes 36
{ <WithC extends AnyStyledComponent>(component: WithC): StyledComponent<StyledComponentInnerComponent<WithC>, Theme, {} & StyledComponentInnerOtherProps<WithC> & ... 4 more ... & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 4 more ... | StyledComponentInnerAttrs<...>>; <WithC extends "sy...
{ <WithC extends AnyStyledComponent>(component: WithC): StyledComponent<StyledComponentInnerComponent<WithC>, any, {} & StyledComponentInnerOtherProps<WithC> & ... 4 more ... & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 4 more ... | StyledComponentInnerAttrs<...>>; <WithC extends "symb...
seenTypes 39
StyledComponent<StyledComponentInnerComponent<WithC>, Theme, {} & StyledComponentInnerOtherProps<WithC> & ... 4 more ... & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 4 more ... | StyledComponentInnerAttrs<...>>
StyledComponent<StyledComponentInnerComponent<WithC>, any, {} & StyledComponentInnerOtherProps<WithC> & ... 4 more ... & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 4 more ... | StyledComponentInnerAttrs<...>>
seenTypes 42
{ <WithC extends AnyStyledComponent>(component: WithC): StyledComponent<StyledComponentInnerComponent<WithC>, Theme, {} & StyledComponentInnerOtherProps<WithC> & ... 5 more ... & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 5 more ... | StyledComponentInnerAttrs<...>>; <WithC extends "sy...
{ <WithC extends AnyStyledComponent>(component: WithC): StyledComponent<StyledComponentInnerComponent<WithC>, any, {} & StyledComponentInnerOtherProps<WithC> & ... 5 more ... & StyledComponentInnerOtherProps<...>, StyledComponentInnerAttrs<...> | ... 5 more ... | StyledComponentInnerAttrs<...>>; <WithC extends "symb...

danielnixon avatar Nov 21 '20 07:11 danielnixon

So we're getting an explosion where {} becomes {} & StyledComponentInnerOtherProps<WithC> becomes {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<WithC> becomes {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<WithC> becomes ...

I believe we're bouncing around like this:

  1. interface ThemedStyledFunction of some other props O
  2. method withConfig that takes some other other props O'
  3. return type of withConfig is ThemedStyledFunction of this new other other props O'
  4. goto 1

Spitballing our options:

  1. Normalise types (remove duplicate intersection components like StyledComponentInnerOtherProps) before comparing to the list of previously seen types. This would terminate the recursion (if it's possible).
  2. When comparing types to those previously seen, use mutual truth of checker.isTypeAssignableTo instead of ===. By "mutual" I mean a isTypeAssignableTo b and b isTypeAssignableTo a. My hope here is that this would render {} & StyledComponentInnerOtherProps<WithC> and {} & StyledComponentInnerOtherProps<WithC> & StyledComponentInnerOtherProps<WithC> equal and thus we'd spot the type in the previously seens types and therefore the recursion would terminate. (Assuming this even works) I don't know what the side effects would be but my spidey sense is tingling given how informal co/contra-variance are in TypeScript.
  3. Terminate the recursion after some arbitrary largish depth (or after a certain amount of time like your suggestion).
  4. When comparing interfaces like ThemedStyledFunction (those that have a method that returns the same type with a different type argument), don't recurse into the method's return type. I'm worried that a solution like this is overly specific and wouldn't catch some other issue that is a cousin to this one.
  5. User configurable whitelist. In this case we'd add ThemedStyledFunction and/or the other styled-components types to it.

Maybe the best move here is option 3 plus one of the others.

danielnixon avatar Nov 21 '20 07:11 danielnixon

but several other checks across the codebase were also reasonably slow (a few hundred ms)

Thanks for flagging this. I reckon we consider slowness separately. I've got some ideas on this front:

  • User configurable whitelist
  • https://github.com/danielnixon/eslint-plugin-total-functions/issues/194

Contributions welcome, as always ;)

danielnixon avatar Nov 22 '20 01:11 danielnixon

@willheslam I've thought of an issue with the a isTypeAssignableTo b and b isTypeAssignableTo a approach.

readonly is ignored for the purposes of assignability checking, which kinda defeats our purposes here. We would have to check a isTypeAssignableTo b and b isTypeAssignableTo a AND a and b have the same readonlyness.

And then when checking for readonlyness we'd fall straight back into:

  1. interface ThemedStyledFunction of some other props O
  2. method withConfig that takes some other other props O'
  3. return type of withConfig is ThemedStyledFunction of this new other other props O'
  4. goto 1

danielnixon avatar Nov 23 '20 23:11 danielnixon

This seems very related: https://devblogs.microsoft.com/typescript/announcing-typescript-4-6/#improved-depth-checks

danielnixon avatar Mar 09 '22 09:03 danielnixon

I've boiled down a reproducer from that styled-components example:

        interface Foo<A extends object> {
          <C extends Foo<any>>(b: Foo<C> & { b: Foo<C> }): Foo<A & C>;
        }

        declare const a: Foo<{ a: string }>;

        const b = a as Foo<{ b: string }>;

danielnixon avatar Mar 12 '23 06:03 danielnixon

I've published a "fix".

In the latest release it takes ~20 seconds to lint that styled-components example on my machine, but that's better than stack overflow / never completing.

I'll keep poking at this to improve linting performance but I'm prepared to call this one solved.

danielnixon avatar Mar 12 '23 06:03 danielnixon

I don't feel so bad about stopping that recursion arbitrarily after watching Anders explain that it's what they do: https://www.youtube.com/watch?v=wpgKd-rwnMw&t=1714s

danielnixon avatar Mar 13 '23 11:03 danielnixon

Yeah, I agree with you - ultimately even if the type system has been designed to halt in an acceptably small O notation-ish relationship to code base size, there will always be some code base too large or a CPU too slow to give acceptable results in an IDE or linting pass!

willheslam avatar Mar 13 '23 11:03 willheslam

There is more work to do: https://github.com/danielnixon/eslint-plugin-total-functions/issues/732

danielnixon avatar Mar 13 '23 22:03 danielnixon