Skip to content

RegExpExecArray should have explicit always-defined properties #50520

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
DanielRosenwasser opened this issue Aug 29, 2022 · 6 comments · Fixed by #50713
Closed

RegExpExecArray should have explicit always-defined properties #50520

DanielRosenwasser opened this issue Aug 29, 2022 · 6 comments · Fixed by #50713
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". Help Wanted You can do this
Milestone

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Aug 29, 2022

[L]ooking a little closely at the ECMAScript spec, I do think that there is a mismatch between what we have and what gets created in the runtime. RegExpBuiltinExec says that the following are created unconditionally:

  • an index property
  • an input property
  • a property at index 0
  • a groups property that is always set, but may be undefined

So RegExpExecArray should look something kind of like

// src/lib/es5.d.ts

interface RegExpExecArray extends Array<string> {
    // always present
    index: number;

    // always present
    input: string;

    // always present - helpful for '--noUncheckedIndexedAccess'
    0: string;
}
// src/lib/es2018.regexp.d.ts

interface RegExpExecArray extends Array<string> {
    // always present - helpful for '--exactOptionalPropertyTypes'
    groups: { [key: string]: string } | undefined;
}

Derived from content posted by @DanielRosenwasser in #50211 (comment)


In additionl to the above, it seems like RegExpMatchArray/RegExpExecArray are sometimes both returned from functions that undergo the same internal code paths. So we should probably ensure that one is compatible with the other, and have a test that ensures as much.

@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript labels Aug 29, 2022
@DanielRosenwasser
Copy link
Member Author

Relevantish because @DetachHead sent #49682

@DanielRosenwasser DanielRosenwasser changed the title RegExpMatchArray/RegExpExecArray likely should be identical, have explicit always-defined properties RegExpExecArray likely should be identical, have explicit always-defined properties Aug 29, 2022
@DanielRosenwasser DanielRosenwasser changed the title RegExpExecArray likely should be identical, have explicit always-defined properties RegExpExecArray should have explicit always-defined properties Aug 29, 2022
@DanielRosenwasser
Copy link
Member Author

Because people will come back to this issue, here's an example of where RegExpExecArray and RegExpMatchArray really should differ.

> "blahfoo".match(/(bar)?/g)
< (8) ['', '', '', '', '', '', '', '']

> /(bar)?/g.exec("blahfoo")
< (2) ['', undefined, index: 0, input: 'blahfoo', groups: undefined]

@TomasHubelbauer
Copy link

TomasHubelbauer commented Aug 30, 2022

In your code sample I believe the last line should be this:

< (2) ['', { undefined, index: 0, input: 'blahfoo', groups: undefined }]

Edit: This is wrong :-)

@fatcerberus
Copy link

@TomasHubelbauer No, he was right:
image

@DanielRosenwasser DanielRosenwasser added Help Wanted You can do this Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". labels Aug 30, 2022
@DanielRosenwasser DanielRosenwasser added this to the Backlog milestone Aug 30, 2022
@graphemecluster
Copy link
Contributor

Actually there shouldn’t be anything like RegExpMatchArray. Because the spec states that String.prototype.match either returns either what RegExp.prototype.exec returns, or a string array with at least a single item, we should change the return type of .match to [string, ...string[]] | RegExpExecArray | null. And, please consider reverting #49682 since it breaks a lot of existing codes (#50633).

@DetachHead
Copy link
Contributor

i think the solution is to add the 0: string to RegExpExecArray as well instead of reverting #49682. i'm happy to make a PR for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants