Narrowing the type inside forEach doesn't follow the if check from the outside

[jcubic]

🔎 Search Terms

type narrowing

🕗 Version & Regression Information

• I was unable to test this on prior versions because I was testing in the playground

⏯ Playground Link

https://www.typescriptlang.org/play?ssl=17&ssc=31&pln=17&pc=38#code/MYewdgzgLgBApgWwA5QJ4H0BOBXANnGAXhgAoIALOOKALhgGEBlZtfRy6gGhmFwEMIEdGD4I4daJgCWYAOYBKIgD4YAbwCwAKBg6e4aDBkATOAA8iMClSgA6YIIBKeOBBv45UcgG4tuyx1sZCDhMKCd8EgADGwASVV4BIRExAF81GBTI7mMzeR9tXUxqbEwwf2s7R2cIAG0c0wBdfJT8rVBIWGCodHsICzIAuiYWVDYA7gTBYVFxSyhpOW5oUdnGVjhFQhUNAp12g2D8YCgQTAtouMmkmcz8v3xYHHwLGps3q2pKiHCXBpsAM2MJCeBC2al8fkKxVKMBBhg6fDAwDgIH+DGYaxWPxgADIcbDnDZDnBjqcACpmWCEamWOBHE6YO66FJ5CE6KRokgAQhBih2kMKzgsiBQGBBA2sE34U2SGyZOhSbMMnLhQSgiORqPRI3wPz5Sr8+1g5mIIK5RPW8shAHkAEYAKxJtjgYHmUhcZHW8gBpwAonxgOQSCQakhMCAkNwAG58XDYOANTbbA0Cgn4C0rInUAAK4aQITQJDDEejsfjrN2kJZVr8dsdxxsLrdHuW+G9-z9AaDIeLkZgMbjCaT4MrqZgpizUFzEYLqCLedLg4rY+rSsVmhaQA

💻 Code

const empty_rule = (sheet: CSSStyleSheet, class_name: string) => {
    const index = sheet.cssRules.length;
    sheet.insertRule(`.${class_name} { }`, index);
    return sheet.cssRules[index];
};

const set_css = (sheet: CSSStyleSheet, class_name: string, style: Style) => {
    const selector = `.${class_name}`;
    let rule = [...sheet.cssRules].find(rule => {
        return rule instanceof CSSStyleRule && rule.selectorText === selector;
    });
    if (!rule) {
        rule = empty_rule(sheet, class_name);
    }
    if (rule instanceof CSSStyleRule) {
        const x = rule.style;
        Object.entries(style).forEach(([prop, value]) => {
            // this throw an error
            rule.style.setProperty(prop, value);
        });
        Object.entries(style).forEach(([prop, value]) => {
            // this works
            x.setProperty(prop, value);
        });
    }
};

🙁 Actual behavior

You can't use a variable that was type narrowed inside forEach. You got an error:

Property 'style' does not exist on type 'CSSRule'.

🙂 Expected behavior

I expect rule.style to work the same inside map.

Additional information about the issue

It looks like inside forEach all typechecks are ignored. Another issue is that the rule can be undefined inside forEach but outside it's ok.

[jcalz]

Another one for the #9998 pile.

[fatcerberus]

Yep, and there’s even a whole issue template explaining not to post this exact issue and everything (“types not correct with or in callback”)…

[jcubic]

I was looking into FAQ but I didn't find an issue like this. Where does it say to not post those types of issues?

[jcalz]

It’s like the sixth button when you open a new issue… but if I were filing a bug report and didn’t already know about it I doubt I’d notice it. Ideally such a thing would be mentioned inside the bug report template itself, but 🤷‍♂️.

[jcubic]

I would put it into the FAQ since the issue says to check the FAQ first before creating an issue.

[fatcerberus]

The explanation in question:
https://github.com/microsoft/TypeScript/issues/new?assignees=&labels=Duplicate&projects=&template=types-not-correct-in-with-callback.md&title=

I’m honestly surprised it isn’t listed in the “Bugs That Aren’t Bugs” section of the FAQ, there’s an issue like this posted at least once a week. People hit this all the time.

Also relevant is #11498 which discusses a way this might be fixed for certain cases (like this one).

[jcubic]

The reason why I and probably everyone else didn't see the question is because people read from top to bottom until they found a proper issue type. They don't read the rest of the issues just because they may have important information, who does that? I would definitely add this to the FAQ instead of adding a fake issue.

I would also get rid of those fake issues that answer the problem and put everything into FAQ. That is supposed to Be a place to read common bugs that are not bugs.

[jcubic]

I just read the fake issue template and I don't really understand. How:

Object.entries(style).forEach(([prop, value]) => {

});

is the same as setTimeout which can get executed after delay and something else can modify the code. This code doesn't have any side effects. Those are built-in methods of JavaScript. the function is obviously a callback like in the issue, but shouldn't this work differently than async functions?

[jcalz]

The type system cannot express that a callback is going to be called immediately, so it can’t tell the difference between those two examples. Please see #11498

[fatcerberus]

The reason why I and probably everyone else didn't see the question is because people read from top to bottom until they found a proper issue type. They don't read the rest of the issues just because they may have important information, who does that?

I don't think you're expected to read all the templates (indeed, who does that?), just to recognize that "types not correct in/with callback¹" covers your issue and then, when you click it, it gives you more context. Interestingly, people do pick that template sometimes... but for completely unrelated issues that then get erroneously tagged as duplicates 😅

I agree this should be in the FAQ though and I'm surprised it isn't.

Footnotes

1. I suspect a large part of the problem is that people don't intuitively consider the function passed to, e.g., .forEach, as being a "callback", as that term carries a lot of baggage relating to asynchronous operation. An equivalent FAQ entry would have to be very carefully worded or people will likely skim past it, too. ↩

[jcubic]

If there was an entry like that in the FAQ I would never create an issue. For me, the callback is always a function passed as an argument to another function.

[MartinJohns]

They don't read the rest of the issues just because they may have important information, who does that?

🤚

[RyanCavanaugh]

The reason why I and probably everyone else didn't see the question is because people read from top to bottom until they found a proper issue type

"First match wins" overload resolution remains a somewhat problematic design aspect of both TypeScript and TypeScript users, apparently 🙂

[jcalz]

That also explains why I only think there's a "Website" button when I talk about that page in the abstract.