Type guard doesn't narrow out union member, for only some purposes

[wbt]

Bug Report

🔎 Search Terms

type guard const narrow narrowing union

🕗 Version & Regression Information

• The comment "no TS error" became true between 4.2.3 and 4.3.5. That fix may be instructive, and possibly broadenable to work when used as a type parameter.
• The errors became a bit more sensible between versions 4.5.5 and 4.6.4.
• Changes related to the Lowercase example of tying input and output types together are not reported here because the comment still holds.
• Nightly version at time of test: v5.0.0-dev.20230201

⏯ Playground Link

Playground link with relevant code

💻 Code

type G7Abbreviation = 'CA' | 'FR' | 'DE' | 'IT' | 'JP' | 'UK' | 'US' | 'EU';
type G8Abbreviation = G7Abbreviation | 'RU';
declare function takesG7WithParam<A extends G7Abbreviation>(arg0: A) : void;
declare function takesG7WithoutParam(arg0: G7Abbreviation) : void;
const takesG8 = function<
    TN extends G8Abbreviation //Constrains other params/return type in inspiring example; type param required
>(abbreviation: TN) : Lowercase<TN> {
    const abbreviationNarrowed = abbreviation; //type: G8Abbreviation at broadest
    if(abbreviationNarrowed === 'RU') {
        console.log('Legacy case handling, not passed on to next function.');
    } else {
        //This type should be Exclude<TN, 'RU'>, at most Exclude<G8Abbreviation, 'RU'>,
        //which in this case happens to be at its broadest G7Abbreviation
        const abbreviationNarrowedAgain = abbreviationNarrowed;
        //Error 2344: Type '"RU"' is not assignable to type 'G7Abbreviation'.
        takesG7WithParam<typeof abbreviationNarrowed>(abbreviationNarrowed);
        //Error 2345: Same 1st line as above error but without explanation lines:
        takesG7WithParam<typeof abbreviationNarrowedAgain>(abbreviationNarrowedAgain);
        takesG7WithParam<typeof abbreviation>(abbreviation); // same as above line
        //Next two lines should work the same, but first has error 2345:
        //'G8Abbreviation' is not assignable to 'G7Abbreviation'.
        takesG7WithoutParam(abbreviationNarrowedAgain);
        takesG7WithoutParam(abbreviationNarrowed); //no TS error, but type params are sometimes required!
        takesG7WithoutParam(abbreviation); //error because parameters aren't narrowed, not the Issue here
    }
    return abbreviation.toLowerCase() as Lowercase<TN>;
}

Note that a call to takesG7WithParam specifying the type parameter as the full union G7Abbreviation with the parameter abbreviationNarrowed works, but sometimes the type parameter should be typed more narrowly (and maybe there could be an issue on whether the parameter is assignable to that, but typeof with the same variable name should help.)

This demonstration doesn’t appear to be affected by known issues with conditional types, nor does it deal with function parameters or ArrayBuffer/Uint8Array types.

🙁 Actual behavior

Const types are not narrowed as expected, except that abbreviationNarrowed is correctly narrowed for the purpose of assigning type to a value parameter (its typeof result is not narrowed for the purpose of use as a type parameter though).

🙂 Expected behavior

• The assignment of one constant to another (here, to abbreviationNarrowedAgain) does not lead to a new error (e.g. the difference between the first two calls to takesG7WithoutParam).
• Both of the consts are narrowed to Exclude<G8Abbreviation, 'RU'> within the else block, for all purposes.
• None of the calls involving abbreviationNarrowed or abbreviationNarrowedAgain have errors.
• All of the errors should have the additional lines of detail shown on the first one, but only the version of the first error from TS 4.6+. (Worse errors in older versions is not raised as an issue here with the current version.)
• When the type of a value is narrowed for the purpose of use as a value parameter, its typeof is also narrowed for the purpose of use as a type parameter.
• The two declared functions should be equivalent, except that the one with a type parameter is more readily extensible to add other parameters which are connected to the first one through the generic type parameter.
• Also in the category of expected behavior, the last line shouldn't require a cast, but that's a separate issue (String case change methods should return intrinsic string manipulation types #44268) not core to the issue being raised here.
• Per comment by @MartinJohns below, the error in calls to takesG7WithoutParam should note that TN is the type that can't be assigned to G7Abbreviation rather than G8Abbreviation.

[MartinJohns]

//This type should be Exclude<G8Abbreviation, 'RU'>
//which in this case happens to equal G7Abbreviation

But that wouldn't be right. You're checking a variable typed TN, not a variable typed G8Abbreviation. If anything it should be Exclude<TN, "RU">. TN is a type argument that could be typed "RU" | "CA", so in the else branch it's type is "CA". It could also just be "RU", meaning the type in the else branch is never.

const abbreviationNarrowed = abbreviation; //type: G8Abbreviation

This comment is already a fundamentally wrong assumption. The type is TN, not G8Abbreviation.

[wbt]

OK, example comments updated.
Same general report; the expected type is additionally constrained by any further tightening of TN.
I tried to keep the example as generic as possible but TN might be narrower. I was also referencing the TS error message, which says the type it can't assign to G7Abbreviation is G8Abbreviation. I added that to the list of expected behaviors.

Also note that the type shown on hover over the parameter in the second call to takesG7WithoutParam is G7Abbreviation.

If in some particular call, TN is typed to just 'RU', static analysis should be able to determine that the else block is never entered, so it doesn't matter that the narrowed type would be never there. The same function could be reusable in multiple places, some of which use that value for TN and others which use a broader or different set.

[RyanCavanaugh]

The problem with using Exclude for negative narrowing comes with nesting - just keeping it in the higher-order form, Exclude<Exclude<A, B>, C> and Exclude<Exclude<A, C>, B> are equivalent in evaluation but likely couldn't be identified as such.

The only right way to do this is to intersect with a negated type, so marking as duplicate of #4196.

The comment "no TS error" became true between 4.2.3 and 4.3.5. That fix may be instructive, and possibly broadenable to work when used as a type parameter.

Not really. The logic here kicks in when TS is relating a type parameter to a concrete union target type, which isn't the case in the other examples.

[wbt]

The type works fine when evaluating for the purposes of assignment to a value parameter; why can't that same logic be applied when determining the type for a type parameter?

[wbt]

The problem with using Exclude for negative narrowing comes with nesting - just keeping it in the higher-order form, Exclude<Exclude<A, B>, C> and Exclude<Exclude<A, C>, B> are equivalent in evaluation but likely couldn't be identified as such.

I would expect those nested Excludes to collapse down to Exclude<A, B | C> or Exclude<A, C | B> respectively, and those two should be identifiable as equivalent.

[wbt]

Also, even if negated types is added, that won't necessarily fix this, and I don't think fixing this requires negated types (just Exclude).
If this issue gets autoclosed as a duplicate, there will be no revisiting it even after negated types gets added (if it ever does) and any new reports of the same issue will properly be closed as a duplicate of this closed one.

[RyanCavanaugh]

I would expect those nested Excludes to collapse down to Exclude...

@wbt these discussions are not productive. I'm saying that this requires a feature, and you're saying that it doesn't, all we need to do is -->implement all the work of that feature instead<--. That's what makes this not-a-bug: it's a feature's worth of complexity.

[wbt]

And since being able to analyze nested Excludes correctly when they're being used as a type parameter (while TS already correctly handles the analysis when determining assignability of a value parameter; no complexity or work to fix that) is apparently a feature's worth of work, we should just not do that and instead close this Issue report as a duplicate of another issue that isn't required for fixing this one and if implemented, still won't fix this one?

To me, that doesn't track.

I suggested a way that nested Excludes could be handled in a way that would address your concern that equivalent expressions "likely couldn't be identified as such" - I apologize for having interpreted your previous requests for possible solutions to hard problems as sincere. I see I was wrong there and the suggestion was unwelcome.

Fixing this issue as reported doesn't even require getting nested Excludes right, just automatically applying a single Exclude. Getting nested Excludes right, if TS doesn't already, might be a separate issue.

[RyanCavanaugh]

Assignability (a yes/no question) is different from narrowing (producing a new type to accurately represent some subset of another type), and is a much simpler problem.

Look, I'm very willing to take these things as bug fixes when they're actually bug fixes. If my estimation is that something is not a bug fix -- is actually much more complex and is actually entwined into a larger set of feature work -- that's the call I have to make.

And it's entirely possible I'm wrong on some of these! But if you're not sending PRs to implement these things you think are actually much simpler than I claim they are, then we'll never really know which is which.

So anyway if you want to send a PR to fix this, please really do go right ahead. I am not opposed to this working the way we both agree it ideally should. I am saying it is not as simple as you're implying it is, and is a feature-level work item to get right, and that the feature that would make this work is negated types. If we had negated types, it's extremely likely that PR would include negative narrowing via intersection; it would simplify a lot of other codepaths.

[wbt]

The error being reported here is a wrong answer to an assignability question. I'm glad to hear that simplifies things.

In the line takesG7WithoutParam(abbreviationNarrowedAgain); there is a false negative answer to the question of assignability as a value parameter, which results from one const being assigned to another.

In at least the first two calls to takesG7WithParam there is a false negative answer to the question of assignability as a type parameter, i.e. is it assignable to the constraints placed on the type parameter.

Look at the text of the error itself! While on that topic, the text of the error message gives more detail in the first instance than the others (and as @MartinJohns points out, isn't strictly accurate), which seems like unexpected behavior that could probably be fixed.

Adding negated types is a very complex feature with wide-reaching implications across the language and any type system based on it. Fixing that won't fix this and with this closed as a duplicate, it'll never get fixed (future reports properly closed as duplicate of this one) and those assignability questions will always give the wrong answer.

Even if this weren't an assignability-question issue, and even if fixing it is a "feature's worth" of work, I don't agree that that means it should be closed. There are plenty of other kept-open issues (including the one about negated types!) that require a feature's worth of work.

[wbt]

The problem with using Exclude for negative narrowing comes with nesting - just keeping it in the higher-order form, Exclude<Exclude<A, B>, C> and Exclude<Exclude<A, C>, B> are equivalent in evaluation but likely couldn't be identified as such.

It looks like TS does identify the equivalence between these correctly, whether for use in value or type parameters, so that should not be a reason why the assignability questions here can't be answered correctly. Even if that were an error, it would (a) be a separate issue, and (b) not actually block fixing cases like those reported above.

[RyanCavanaugh]

The error being reported here is a wrong answer to an assignability question.

Sure, but typechecking is necessarily an incomplete operation, and the default answer needs to be "no" in cases where feature work to correctly answer "yes" hasn't been done yet. Which just brings us full circle again.

Back to this example, taking a narrowed expression type involving a generic and then performing a single yes-or-no "assignable to this union" means TS can use the simplified (i.e. non-generic) form of that expression and still achieve correct results. This logic doesn't generalize to the entire type itself - you can't take a T extends string | number that's been narrowed to string and just treat it as string when the correct type is T, because this will absolutely give incorrect results.

The fact remains that the bare mechanics of the type system today can't yet represent what needs to happen in this example. Negated types are the correct solution to that problem. A while back, someone identified that we could achieve better completeness in places in expression space with a clever (but isolated) solution, so we did that, and now you're using that solution as an argument for why the type system should, again, have a feature it doesn't yet have. In absence of that clever isolated solution, all three calls to takesG7WithoutParam would be an error, and the need of negated types to properly handle this code would probably be more apparent.

Or maybe I'm wrong about all of that and a simple PR that fixes this example exists. It's happened before. Again, if you want to put up that PR, or find someone to write it for you, I offer my 100% support. But I'm not going to put this in the bucket that says "Straightforward defects" when I really don't think that's the case.

Re: open/closed/etc: I'm just going to draw a line in the sand here. You've complained about open/closed/other metadata on issues a lot, and I've specifically explained the how-and-why of what we do with issues here on several occasions. Any schema capable of handling 35,000 issues is necessarily going to involve choices that at least one person doesn't agree with, but that doesn't give that one person unlimited license to keep bringing it up. Your choices here are to stop raising this as a point of contention, have your issues locked on first response, or be blocked from the repo. These are the only options I can provide in the interest of moderating the issue tracker and keeping conversations productive, so I'll leave you with the choice in how we proceed.

[typescript-bot]

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

[wbt]

Re: open/closed/etc: I'm just going to draw a line in the sand here. You've complained about open/closed/other metadata on issues a lot,

I wish there were some meta place to have a discussion on that directly without polluting some specific issue about unexpected behavior in TypeScript code. However, since none of the unexpected behaviors raised above are going to be addressed through this Issue, keeping the response here seems reasonable.

The way you've claimed to use open/closed/etc. seems to be very different from most other open- and closed-source projects. Generally speaking, an issue marked Closed is one where there is a decision made that no part of the issue will ever be addressed. For example in this Issue, there are multiple unexpected behaviors including assignment from one const to another triggering an error, as well as correctness and specificity of error messages, and the closure indicates nothing in the issue report was useful or will ever lead to making TypeScript even a little bit better. The message communicated with this closure is that nothing of what’s unexpected here is feasible to solve, or maybe that behavior is actually supposed to be considered expected.

Distilling issues and authoring issue reports, especially in this repository, is a highly effortful process and having so many of them closed soon after filing sends a pretty clear message (whether it's what you intend or not) that those contributions and all of the effort behind them will make no difference toward the goal they were intended to support (i.e. making TypeScript better). Because active effort goes into each closure decision, such contribution attempts are obviously taking effort away from the goal the work was intended to support, making things worse, and such contributions are actively unwelcome.

In open-source, most contributors start out by filing bug reports. If those are well-received, the new contributor might get adventurous enough to make some simple PRs (and if they are not, there's usually no point in investing the time and effort needed to learn the code-base or build/test process as contributions affecting code are less likely to go over well). Often, those are on issue reports someone else filed, especially if tagged “good first issue;” these would not likely have been fixed without the initial report. If simple PRs are well received, the new contributor might then invest the code-base understanding effort needed for more complex PRs, later progressing into more active maintenance tasks and sometimes winding up with contributions at the level of a core contributor. There is already plenty of natural attrition at each step, but open-source projects have much higher chances of long-term success when they are more welcoming of contributions at those lower early levels instead of shutting them out with unwelcoming signals. Seeing TypeScript as an otherwise very promising project be this unwelcoming to contributors on the outer rings of that journey pains me not just as a recipient of that but also as an observer and as someone who hopes the best for TypeScript's long-term success. This is an unnecessary self-inflicted wound on the project. Again, this strong unwelcoming message may not be the one you're intending to send, but it is the one you're sending, especially with messages like the above that there cannot even be discussion on the topics to propose merits of alternative outcomes. It signals that the open-source norm of meritocracy doesn’t apply to this project: how can the best ideas win when they can’t even be raised, depending on the identity of the speaker? The extent of unwelcoming practices here seem to contradict the MS Open Source Code of Conduct but that might just be one more set of norms that don’t actually apply in this project.

The comment above explicitly unwelcoming a comment proposing a solution to what had been falsely identified as an unsolved hard problem blocking resolution to this issue further discourages any steps toward proposal of solutions.

There are some cases where issue closure is fully appropriate. First, if an issue report actually duplicates another such that fixing the original will be necessary & sufficient to fix the duplicate, it should be closed as a duplicate, and issues tagged “duplicate” should then receive no further attention at all. Second, if an issue report describes something that isn't actually an issue (example here), that should be closed. If an Issue report aims to advance a non-goal that should be closed declined / out of scope. For issue reports where a fix would actually require a general solution to the halting problem or equivalent, I agree with your earlier post that closure makes sense there, though in all cases where a corresponding closure reason has been given, I think that's a gross exaggeration of what would actually be required to solve the issue. The proposal below would still help a bit in those situations.

Issue reports that are closed as a duplicate of nothing are the most frustrating, because they strongly send the “report is unwelcome” WONTFIX message noted above while ensuring no future review due to the “duplicate” tag, and simultaneously communicate the message that good-faith efforts to make things better will be met with responses that are not only unwelcoming but also disingenuous. This is further compounded by blaming a reader for a “misread” which is somehow inherently “aggressive” as if not using words for what they commonly mean couldn’t possibly be at all contributing to any miscommunication. You have claimed that this practice actually just means that the issue seems hard to fix, implying that it really should be obvious to a reader that “Closed/Duplicate” really means “Seems too hard to fix” and why would we want to discard the useful information from that initial assessment by leaving such issues open instead of burying them with a closure and inapplicable reason tag?

You could have a label “seems too hard to fix” that would communicate what you claim to mean much more effectively. It would be not thrilling but far more understandable and thus less unwelcoming to see, and draw more PRs from those who disagree. It could even coexist with “Bug” on the same issue where applicable. The conventional meaning of the “design limitation” label is much tighter, implying that someone has done the analysis and figured out an issue is not actually possible to fix because of a design decision in TS, and attempts to fix it will be rejected because of conflict with an intentional design choice. “Seems” is a valuable component of that label if you want to communicate (per your last post above) that the initial analysis might be wrong and fixes would be more likely applauded than rejected. The “too” helps distinguish it from the expected case, that still-outstanding bugs are hard to fix and avoids communicating the message that the team just refuses to take on any difficult bugs at all.

If fixing one issue is blocked awaiting a fix to another, an open status with a “Blocked” label makes much more sense, as it will lead to occasional future review to see if it is still blocked. A lot of the issues closed as duplicates of negated types that actually require negated types but will still require additional work even after that is implemented should probably be in this status, for example. The project has a “Blocked” label already, but isn’t using it much.

This project has discarded a surprising number of open-source norms. In addition to those identified above about the claimed meaning of closure reasons and meritocracy in discussions, other typical open-source norms aren’t used here such as semver (though the primary recommended distribution channel assumes that’s followed here in automatic upgrade/downgrade decisions), and including in documentation a description of how things actually work. On the latter point, this project’s expectation is that instead of qualifying oversimplified descriptions in documentation, developers are supposed to keep up-to-date on reading through a quickly-growing issue list with tens of thousands of issues, including closed issues, hundreds of which have 3-digit comment counts. Even still then, the description of intended functionality in PRs still cannot be relied upon to provide appropriate expectations of what the code should do (i.e. a violation of the PR description isn’t enough to label something as a bug). How many other conventions are disregarded or opposite in this project’s code-base or contribution pipeline? In the documentation, you have introductions like “TS for {JS, Java/C#, Functional} Programmers”; maybe “open source” should be added to that list with a concise clear description of all the ways this project diverges from common practices used elsewhere so that folks can get that in one place instead of annoying core maintainers by folks having to learn it slowly over time.

It sounds here like you’d rather silence or ban an account who’s been contributing in good faith, along with any other accounts who might dare to lobby for fixing an issue they’re reporting, perhaps on suspicion of shared/related control. Your threats above will discourage future attempts at contributions to make TS better, but I really would encourage some reflection on the divergences between this and other open-source projects, the messages being received by would-be supporters of the project through these issue closures and contributor silencings, and whether good-faith reports noting fixable issues really should be tossed out so frequently.

I understand that it’s not easy to maintain a large, complex, and popular project with such a quickly-growing issue list, and I know issue triage isn’t fun, but I think there’s a lot of room for improvement in how this is currently handled and the messages that such quick closures or inaccurate tags send to issue reporters who are also putting forth nontrivial efforts to try to make things better. Thanks for your work.