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

Prevent error 2488 Type 'RegExpMatchArray | null' must have a '[Symbol.iterator]()' method that returns an iterator. when match is guaranteed #47808

Closed
5 tasks done
tredondo opened this issue Feb 9, 2022 · 9 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@tredondo
Copy link

tredondo commented Feb 9, 2022

Suggestion

πŸ” Search Terms

Type 'RegExpMatchArray | null' must have a 'Symbol.iterator' method that returns an iterator.
string.match
null guard

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

TypeScript could infer that in the branch of an if (string.match(RE)) { ... }, string.match(RE) won't be null.

πŸ“ƒ Motivating Example

When assigning the results of string.match() to several variables via array destructuring, we must first ensure that .match() didn't return null. If the regexp is constant and not performance-sensitive, this can be also done as follows:

const RE = /\d+/;
const string = 'abc123xyz';

if (string.match(RE)) {
    const [digits] = string.match(RE);  // TS(2488), but string.match(RE) is guaranteed non-null here
    console.log(digits);
}

Currently, tsc doesn't detect that string.match(RE) won't be null in the if branch.

πŸ’» Use Cases

Trying to assign the result of a string.match to several variables, using array destructuring.

A workaround is to create an intermediary variable for the match results:

const RE = /\d+/;
const string = 'abc123xyz';

const matchResults = string.match(RE);
if (matchResults) {
    const [digits] = matchResults;
    console.log(digits);
}

Another workaround is to use try/catch around the match().

@AviVahl
Copy link

AviVahl commented Feb 9, 2022

When you're 100% certain it cannot be null, you can use .match(...)! to tell typescript that. Note: if you do that when it might actually be null, you'll get runtime error thrown for that destructuring.

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-0.html#non-null-assertion-operator

@fatcerberus
Copy link

string.match(RE) is just another method call to the compiler - it just knows that this is a method that takes a regexp and returns RegExpMatchArray | null; it has no way to know it will return the same value both times.

The correct way is to assign the result the result to a temporary and check that, as you did in your workaround. I personally would never write the first snippet, as calling a method twice that you know is going to return the same thing both times just creates extra work for the computer unnecessarily.

@tredondo
Copy link
Author

tredondo commented Feb 9, 2022

I personally would never write the first snippet, as calling a method twice that you know is going to return the same thing both times just creates extra work for the computer unnecessarily.

Agree with that, but the resulting code is less readable in if-else-if statements if temporary variables are created:

const res1 = string.match(re1);
if (res1) {
    const [g1] = res1;
    // ...
} else {
    const res2 = string.match(re2);
    if (res2) {
        const [g2] = res2;
        // ...
    }
}

Vs. matching again:

if (string.match(re1)) {
    const [g1] = string.match(re1)!;
    // ...
} else if (string.match(re2)) {
    const [g1] = string.match(re2)!;
    // ...
}

A temporary matchResult can still be used to avoid calling .match() again,

let matchResult;
if (matchResult = string.match(re1)) {
    const [g1] = matchResult;
    // ...
} else if (matchResult = string.match(re2)) {
    const [g1] = matchResult;
    // ...
}

but that leads to a linting error about using the assignment as a comparison.

@Retsam
Copy link

Retsam commented Feb 9, 2022

@tredondo How are you suggesting this be solved, exactly?

Are you suggesting that TS should type-guard when you repeat function calls with the same arguments? If so, this isn't true in the general case:

function myMatch() {
    if(Math.random() > .5) {
        return [1,2,3]
    }
    return null;
}
if(myMatch()) {
   // If TS assumes `myMatch()` returns `[1,2,3]` then this compiles... but it's obviously not safe
    const [one, two, three] = myMatch()
}

Are you suggesting that TS should have special case-logic for treating string.match as an exception, hard-coded into the compiler? Theoretically possible, but I doubt it's going to happen - I don't think the team is likely interested in hard-coded exceptions to the normal type system rules for specific functions.

The only way I could see this being solved without either hard-coded exceptions (or big losses of type-safety) would be to add entirely new concepts to the language (e.g. "pure" functions that always return the same output for their inputs, identified at the type level).


So, yeah, I think you're best off looking for a solution in your own code. If you think the if(matchResult = string.match(re1)) is significantly better than the nested-if version, then sure, I say disable the linter rule and go for it. ("The linter was made for man, not man for the linter")

@tredondo
Copy link
Author

tredondo commented Feb 9, 2022

TS should have special case-logic for treating string.match as an exception, hard-coded into the compiler?

That's what I was thinking, if it's not an awkward solution.

@Retsam
Copy link

Retsam commented Feb 9, 2022

TS should have special case-logic for treating string.match as an exception, hard-coded into the compiler?

That's what I was thinking, if it's not an awkward solution.

Yeah, I think that'd be an awkward solution, I don't think the TS team wants to add hard-coded compiler exceptions for specific APIs. I'm pretty sure there aren't any existing examples of compiler-baked exceptions for build-in APIs (even though there's quite a few places where they theoretically could).

Opening the door to these sort of hard-coded exceptions would potentially add a lot of complexity, and I think it overall makes the language more confusing if some special functions don't "play by the same rules" as the rest of the language.

I think there'd have to be a significant benefit, and I don't think "it avoids an extra variable assignment / violating a linter rule" or "I think this is slightly cleaner with less indentation" meets that bar.

@MartinJohns
Copy link
Contributor

MartinJohns commented Feb 9, 2022

const [digits] = string.match(RE) || [];
if (digits) {
    console.log(digits);
}

Straight forward, no duplicate computations, type-safe (if noUncheckedIndexedAccess is enabled).

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Feb 9, 2022
@RyanCavanaugh
Copy link
Member

We don't hardcode specific method behavior into the compiler; this makes it impossible for users to write functions that behave the same way as built-ins, which is a really frustrating state to be in.

A possible avenue would be #7770 - two identical pure function calls could be treated as "the same" for purposes of narrowing, but even that isn't really sufficient here, since RegExps themselves are not immutable. You would have to somehow know that match doesn't look at lastIndex.

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

7 participants