Skip to content

Bug: [prefer-reduce-type-parameter] unfixable reporting #3440

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

Closed
3 tasks done
JounQin opened this issue May 24, 2021 · 8 comments · Fixed by #10494
Closed
3 tasks done

Bug: [prefer-reduce-type-parameter] unfixable reporting #3440

JounQin opened this issue May 24, 2021 · 8 comments · Fixed by #10494
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@JounQin
Copy link
Contributor

JounQin commented May 24, 2021

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have read the FAQ and my problem is not listed.

Repro

{
  "rules": {
    "@typescript-eslint/<rule>": ["prefer-reduce-type-parameter"]
  }
}
// first case
export function normalizeParams<P>(params: P): P {
  return Object.entries(params).reduce((acc, [key, value]) => {
    value = typeof value === 'string' ? value.trim() : value;
    if (value) {
      acc[key as keyof P] = value;
    }
    return acc;
  }, {} as P);
}

// second case
export type Keys<T> = { [Key in keyof T]: Key };

export const toKeys = <T>(source: T): Keys<T> =>
  Object.keys(source).reduce((acc, k) => {
    const key = k as keyof T;
    acc[key] = key;
    return acc;
  }, {} as Keys<T>);

Expected Result

Ignore such cases

Actual Result

Error reporting

Additional Info

Versions

package version
@typescript-eslint/eslint-plugin 4.24.0
@typescript-eslint/parser 4.24.0
TypeScript 4.2.4
ESLint 7.27.9
node 12.22.1
@JounQin JounQin added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels May 24, 2021
@bradzacher
Copy link
Member

bradzacher commented May 24, 2021

Both of your examples are completely unsound due to the as though.
You're forcing typescript to treat P/T as an object, but they are unbounded generics - meaning they can be non-objects.

const str = normalizeParams('foo');
// typeof str === 'foo'
// but at runtime
// str === {0: 'f', 1: 'o', 2: 'o'}

const num = normalizeParams(1);
// typeof num === 1
// but at runtime
// num === {}
const str = toKeys('foo');
// typeof str === string
// but at runtime
// str === {0: "0", 1: "1", 2: "2"}

const num = toKeys(1);
// typeof num === number
// but at runtime
// num === {}

Doing an as assertion really is unsafe here.
So the rule is really doing its job here by enforcing you don't write broken code.

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look labels May 24, 2021
@JounQin
Copy link
Contributor Author

JounQin commented May 25, 2021

Then what about adding extends object after the generic?

export function normalizeParams<P extends object>(params: P): P {
  return Object.entries(params).reduce((acc, [key, value]) => {
    value = typeof value === 'string' ? value.trim() : value;
    if (value) {
      acc[key as keyof P] = value;
    }
    return acc;
  }, {} as P);
}

Or, do you have any recommendation to handle these cases?

@bradzacher
Copy link
Member

bradzacher commented May 28, 2021

It's hard to say exactly - TS does treat generics kind of weirds in conjunction with empty object types.
This also all boils down to the object type being super hard to use at all.

THere's no way to write this function without using an as assertion from what I can tell.

@bradzacher bradzacher added enhancement New feature or request and removed awaiting response Issues waiting for a reply from the OP or another party labels May 28, 2021
@JounQin
Copy link
Contributor Author

JounQin commented Sep 18, 2021

Actually I respectly disagree with

This also all boils down to the object type being super hard to use at all.

It is very useful when you want a generic which must be an Object, not primitive nor Array, there is no alternative, the suggestions Record or unknown are not useful here. Of course this is another topic/rule question.

@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Oct 25, 2021
@JoshuaKGoldberg
Copy link
Member

I actually just hit this in the typescript-eslint codebase 😄:

function parseOptions(context: Context): ParsedOptions {
const normalizedOptions = context.options
.map(opt => normalizeOption(opt))
.reduce((acc, val) => acc.concat(val), []);
return util.getEnumNames(Selectors).reduce((acc, k) => {
acc[k] = createValidator(k, context, normalizedOptions);
return acc;
}, {} as ParsedOptions);
}

Simplified in our playground:

type ParsedOptions = Record<'a' | 'b', number>;

function parseOptions(): ParsedOptions {
  const result = ['a', 'b'].reduce((acc, k) => {
    acc[k] = k;
    return acc;
  }, {} as ParsedOptions);

  return result;
}

@bradzacher @JounQin what was the fix you had in mind? Adding an option to disable as casts?

Note that we're blocked from actually detecting unfixable reporting on my favorite TypeScript issue: microsoft/TypeScript#9879. 😢

@bradzacher
Copy link
Member

bradzacher commented Jan 20, 2023

Potential fixes:

  1. don't error if you use both:
  const result = ['a', 'b'].reduce<ParsedOptions>((acc, k) => {
    acc[k] = k;
    return acc;
  }, {} as ParsedOptions);
  1. don't error if the value being asserted is specifically {} (maybe behind an option)

@bradzacher
Copy link
Member

The big problem you've got is this code is straight-up unsound though - it's not good code!

For example Josh your code has ParsedOptions typed Record<string, number> - yet your code does acc[k] = k - creating an object like Record<string, string>!!! TS doesn't complain about it at all thanks to the completely unsafe as.

There's also the problem of missing keys - your type specifies the object must have the keys 'a' | 'b', but if you were to modify your array to just be ['a'] then the code would typecheck fine and TS would accept result.b.toFixed() - but it would crash at runtime because b isn't ever defined!

.reduce is just a bad way to construct an object with known keys, sadly.
And there's not really a good option otherwise because like Object.fromEntries forcibly returns Record<string, T> - so you can't type the keys.

@JoshuaKGoldberg JoshuaKGoldberg added blocked by external API Blocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API and removed accepting prs Go ahead, send a pull request that resolves this issue labels Oct 11, 2023
@JoshuaKGoldberg
Copy link
Member

Now that v8 is released, per #7936, we can mark this issue as unblocked! 🚀

@JoshuaKGoldberg JoshuaKGoldberg changed the title [prefer-reduce-type-parameter] unfixable reporting Bug: [prefer-reduce-type-parameter] unfixable reporting Aug 1, 2024
@JoshuaKGoldberg JoshuaKGoldberg added bug Something isn't working accepting prs Go ahead, send a pull request that resolves this issue and removed enhancement New feature or request blocked by external API Blocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API labels Aug 1, 2024
@github-actions github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Dec 29, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
3 participants