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

Array prototype extensions fail circularly with ...args: T[], but not args: T[] #58068

Open
Gershy opened this issue Apr 3, 2024 · 6 comments
Labels
Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Milestone

Comments

@Gershy
Copy link

Gershy commented Apr 3, 2024

🔎 Search Terms

Typescript array prototype extension spread rest variadic parameter circular failure TS2310 version 5.4.3

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about circular compilation failures

I've added two playground links; the first works, the second fails. The only difference between the snippets is receiving an array T[] as a spread argument vs a single argument.

⏯ Playground Link

Working: https://www.typescriptlang.org/play?#code/PTAEBUAsEsGdQO4HsBOBrWAuAUAYyQHawAuoBApggLLnGRIAm8AvKAN7ahegCGDDAHnAA+ABR04mCAG0AugBpQANx4AbAK7ksM2QEop4dqAmwAdAAd1sSKNN2VGrboDcoFLXUoCytZtjSABllXAF9sEOdsYgBPc3JQADlKAEEUFB5omjpGFmNY8iQAMzJKLPomSOgCYnIUQp5ceNT06KFhUHIADxqCJkSUtIyynPYI7GwAeQAjACtyXGJTBnJCqvIABRQkOJQY0WaMiy3iJBi4xQByPgYLxTYOgnUAW1qeKdVyKXrVWHJFBBQ0GIbw+UmIKE0igcmikFGotHKZmuoBCLnG+CIpCe0WaUhIgIIAHM5KBWHJItjmqZrqJpKArlNcLd6ctCsyLoSYBdQHpInhCLAkB9TKokITRPdKWkUS4gA

Failing: https://www.typescriptlang.org/play?#code/PTAEBUAsEsGdQGYENoBtYC4BQBjA9gHawAuoBApgO4Cy5xkeAJvALygDeWo3oSjjAHnAA+ABT04GCAG0AugBpQAOhUA3JKgCu5TDNkBKKeA6gJsJQAdNsSKJVL1WnfoDcoAE51N7gqEfbYaQAGWTcAXywwlyxiAE8LclAAOSoAQXd3JFjaeiZWU3jyPAQyKhyGZmjoAmJyd2QcRPTM2KFhUHIAD1qCZmS0jKzyvI4orCwAeQAjACtyHGIlRnIEavIABXc8BPc40Wasyy3iPDiExQByPkYLxXYOgk0AWzqkKdRyKWR0ckVKd2gxDeHykxHc2kU-k+pRodAq5muoDCrnG+CIpCesWaUhIAIIAHM5KA2HJopjmkprqIrlMcLdQBdlgh6Rd8TALijcIRYHgPkpUHh8aJ7uSMkjXEA

💻 Code

Failing:

const newMethods = {
    add<T>(this: T[], ...values: T[]): T { this.push(...values); return values[0]; }
};
type NewArrayMethods = typeof newMethods;
interface Array<T> extends NewArrayMethods {};

Object.defineProperty(Array.prototype, 'add', { enumerable: false, writable: true, value: newMethods.add });

const myArr: string[] = [];
myArr.add('abc', 'def', 'ghi');

console.log({ myArr });

Working (only difference is ...values: T[] is now values: T[] - without ...):

const newMethods = {
    add<T>(this: T[], values: T[]): T { this.push(...values); return values[0]; }
};
type NewArrayMethods = typeof newMethods;
interface Array<T> extends NewArrayMethods {};

Object.defineProperty(Array.prototype, 'add', { enumerable: false, writable: true, value: newMethods.add });

const myArr: string[] = [];
myArr.add([ 'abc', 'def', 'ghi' ]);

console.log({ myArr });

🙁 Actual behavior

Example using ...values: T[] fails to compile!

🙂 Expected behavior

Both examples should compile successfully.

Additional information about the issue

No response

@jcalz
Copy link
Contributor

jcalz commented Apr 4, 2024

It's not a crash. (I guess the template should explain what this means since so many people seem to choose this option erroneously.)

I'm not sure why this is supposed to be a bug; circularity warnings can be annoying but that doesn't rise to the level of a bug in the language. Yes, you can avoid warnings by changing your code. Note that the error message mentions that the problem is partially due to the lack of a type annotation on newMethods. So if I wanted to get your code working, I'd annotate like

const newMethods: {
    add<T>(this: T[], ...values: T[]): T;
} = {
    add(...values) { this.push(...values); return values[0]; }
};

Playground link

@fatcerberus
Copy link

fatcerberus commented Apr 4, 2024

I'm honestly confused why the code is written this way (interface Array<T> extends ...) instead of just augmenting Array directly. It's really weird to do declaration merging via extends, imo.

@Gershy
Copy link
Author

Gershy commented Apr 4, 2024

It's not a crash. (I guess the template should explain what this means since so many people seem to choose this option erroneously.)

I'm not sure why this is supposed to be a bug; circularity warnings can be annoying but that doesn't rise to the level of a bug in the language. Yes, you can avoid warnings by changing your code. Note that the error message mentions that the problem is partially due to the lack of a type annotation on newMethods. So if I wanted to get your code working, I'd annotate like

const newMethods: {
    add<T>(this: T[], ...values: T[]): T;
} = {
    add(...values) { this.push(...values); return values[0]; }
};

Playground link

I created an issue as I feel the two examples should compile the same way, whether it's successful or unsuccessful. I can't see any reason why circular typing would invalidate one example but not the other - that is, I don't see why the spread operator would have any connection to circular type definitions.

Explicitly declaring the typing of newMethods introduces repetition and reduces maintainability - and everything works, without explicit typing, in the other example - why shouldn't it be able to work here? It feels like an inconsistency in typescript, and hence a bug (as opposed to feature request).

@jcalz
Copy link
Contributor

jcalz commented Apr 4, 2024

#49263 (comment)

@Gershy
Copy link
Author

Gershy commented Apr 4, 2024

I'm honestly confused why the code is written this way (interface Array<T> extends ...) instead of just augmenting Array directly. It's really weird to do declaration merging via extends, imo.

I think this way could allow for more maintainability if extending a prototype with many properties; extends automatically applies them without needing to manually enumerate the property names

@RyanCavanaugh RyanCavanaugh added the Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases label Apr 4, 2024
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Apr 4, 2024
@RyanCavanaugh
Copy link
Member

If there's some super straightforward fix here we can take it, but I wouldn't consider this a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Projects
None yet
Development

No branches or pull requests

4 participants