Skip to content

never-returning branches should not be considered when determining whether a property is initialized in a constructor #56362

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
ssalbdivad opened this issue Nov 10, 2023 · 22 comments
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript

Comments

@ssalbdivad
Copy link

ssalbdivad commented Nov 10, 2023

πŸ”Ž Search Terms

constructor, initialization, initialized, 2564, never, throw, error

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried

⏯ Playground Link

https://tsplay.dev/w6Q9yW

πŸ’» Code

const throwParseError = (def: unknown): never => {
    throw new Error(`${def} is not a valid definition`)
}

class Foo {
    // Property 'bar' has no initializer and is not definitely assigned in the constructor.(2564)
    bar: string

    constructor(def: string | null) {
        if (def === null) {
            // this return pattern is useful for narrowing
            // if you remove `return` here, the initialization error goes away
            // but the assignment to bar is then an error
            return throwParseError(null)
        }
        this.bar = def
    }
}

πŸ™ Actual behavior

TypeScript reports that Property 'bar' has no initializer and is not definitely assigned in the constructor.(2564)

πŸ™‚ Expected behavior

TypeScript should identify that in every viable branch of the constructor, bar is assigned.

Additional information about the issue

No response

@MartinJohns
Copy link
Contributor

Make your function a never returning function and drop the return keyword and it works as you expect.

Your lack of annotation for throwParseError means it's not analyzed for control flow. TypeScript does not consider your return throw..() as a throwing operation, just as an early returning constructor, so the property remains uninitialized.

@ssalbdivad
Copy link
Author

ssalbdivad commented Nov 10, 2023

@MartinJohns Thanks, I was a bit hasty creating the repro here and have updated it to include never as an explicit return for the function.

However, this is is an issue I've run into for a long time (@Andarist finally convinced me to create it πŸ˜…) and if you don't return, you lose narrowing. Another option would be to narrow bar without having to return, but since TS doesn't do that in general I assume it would be a bigger problem for CFA.

@nmain
Copy link

nmain commented Nov 10, 2023

It seems to work as expected when @MartinJohns 's suggested changes are made:

const throwParseError: (_: unknown) => never = (def: unknown): never => {
    throw new Error(`${def} is not a valid definition`)
}

class Foo {
    // Property 'bar' has no initializer and is not definitely assigned in the constructor.(2564)
    bar: string

    constructor(def: string | null) {
        if (def === null) {
            // this return pattern is useful for narrowing
            // if you remove `return` here, the initialization error goes away
            // but the assignment to bar is then an error
            throwParseError(null)
        }
        this.bar = def
    }
}

https://www.typescriptlang.org/play?ts=5.4.0-dev.20231110#code/MYewdgzgLgBFAWAnEB3ACgQ0RApgUUWUQC4YAKAfVIFcwBrMVMAShgF4A+GMHANx0TtyAExwAzGvUYoWpHv0GcYAbwBQMDXCSpuOFDAJEyAAwAky0WIC+MAJYRuIWBhi8MAG1vCYl22FtQtuDGzKpWqqrA7hgQDgBiICAq6poA9KkwaMgADgJQAJ4wAOQARlhFMPAxjnb+gR62AF4CMBhg3vaOsL51OO6FMRC2AOY8HWBaODCgkFCI1MBQIIgAdGQATACsAGwALKGaMGUkMNCIfsMRhzNnC0uIZJakZxcwAD7c1O7urGqHh7YxCJxOw2GxPt9fil-v90lpOogcFBqIgJtkMFAoAIJp1qLgxF8YGJltwsMgUBdoTC0hlATB8iBqDBEQBbED8GDGRHI1HGSoCHAAGkmtQCtgajQxQQmAiIMGGIBwDgwKAw+Sp1JgcJK1FgCCmgxGYBZODAeqSxzsDn1EzaMFlyw11IQ5Mw2HwhGWZDAXx+To04U1CHsK0t4MsVPC4SAA

@MartinJohns
Copy link
Contributor

MartinJohns commented Nov 10, 2023

if you don't return, you lose narrowing

This is not the case when you use a never returning function and call it accordingly. Because of the return keyword it's not analyzed as an assertion function.

@ssalbdivad
Copy link
Author

@nmain Interesting that if you annotate throwParseError directly instead of its return, it works. Suppose it makes sense based on some other issues with the way CFA handles arrow functions.

However, in this case as in other cases (e.g. using an arrow function in an asserts predicate), there should be a way to treat it as equivalent to a top-level function in terms of its signature inference if the return is explicitly annotated.

@ssalbdivad
Copy link
Author

@MartinJohns TS should be able to identify this as a never-returning function:

const throwParseError = (def: unknown): never => {
    throw new Error(`${def} is not a valid definition`)
}

@MartinJohns
Copy link
Contributor

TS should be able to identify this as a never-returning function:

There is an open issue about this that I just don't have at hand right now. Will edit the comment when I found it.

@ssalbdivad
Copy link
Author

ssalbdivad commented Nov 10, 2023

Based on @nmain's workaround, it seems like this is likely a duplicate of the issue I logged for resolving the signature of an arrow function at this stage:

#56147

@MartinJohns this is probably what you mean

I will leave this open for now but fine with me if the team confirms it's the same underlying issue and wants to track it there.

@MartinJohns
Copy link
Contributor

I think I'm thinking of another issue that I can't seem to find on mobile right now, but that issue you linked works as well. :-D

@ssalbdivad
Copy link
Author

I think the main problem with these is that unless you know about this nuance of when TypeScript resolves arrow function signatures and how it can use them, you'd never think to explicitly annotate the variable as a workaround (in addition to the syntax being much clunkier).

Even having logged that other issue, it didn't occur to me until I saw @nmain's repro that it was related, especially since in this case there is no explicit asserts predicate.

@MartinJohns
Copy link
Contributor

I found this which is highly related to your last comment: #36067

I think it should be added to the FAQ.

@ssalbdivad
Copy link
Author

@MartinJohns Thank you, it's great to know that exists!

Documentation would be nice, but IMO in this case it should just be fixed. This is a lot less messy than most CFA scenarios in TS. Just check if a variable is an arrow function with an annotated return, and if so treat it like a normal function declaration in terms of signature resolution.

@RyanCavanaugh
Copy link
Member

Just check if a variable is an arrow function with an annotated return

We discussed this option and decided that the inconsistency of some objects needing an explicit type annotation and some not felt more like a bug than the existing behavior

@ssalbdivad
Copy link
Author

@RyanCavanaugh Can you give an example of these objects?

It seems like a top-level arrow function is such a common case, and it's clearly analogous to the annotated form like const throwParseError: (def: unknown) => never = .... Even syntactically it looks the same.

I also do feel like regularly there are cases where explicit returns are required (e.g. for recursive functions) and while there may be cases where TS ideally could infer them, I don't consider those bugs. I don't consider this exactly a bug either, but I do feel like it is hard to deny handling that case explicitly would result in an overall better experience for most TS devs.

@RyanCavanaugh
Copy link
Member

const obj = {
  blah: (): never => { throw null }
}

// Why not controlflowed? Seems like a bug!
obj.blah();

@Andarist
Copy link
Contributor

Andarist commented Nov 10, 2023

With both of those examples in mind we now have 2 situations that look like a bug whereas by recognizing arrows we’d have just one ;p

I see your overall point but since arrows are just so common (and some linters even enforce them)… it just feels to me that they could be recognized purely on grounds of pragmatism

@ssalbdivad
Copy link
Author

@RyanCavanaugh I disagree that it would seem like a bug for control flow not to work in that case. I think all of these cases basically fall onto a spectrum based on how common they are and how expensive they would be to detect.

The top-level arrow function case seems a) extremely common and b) very cheap to detect. It's clear that once you start getting into objects like this, the trade-offs are not going to be as good, so I don't think anyone could fault you for special casing top-level arrow functions only.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Nov 10, 2023

I'm generally a pragmatist too, but we also get a lot (a LOT) of feedback from people who just want the language to be "consistent" even at the expense of convenience. People will demand, for example, that the documentation spells out every single case that does and doesn't work, once you go beyond a simple rule.

@ssalbdivad
Copy link
Author

@RyanCavanaugh Anyone who would complain about this is trolling🧌

Just as never them and carry on building a language that is already statistically shown to be loved by developers.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Declined The issue was declined as something which matches the TypeScript vision labels Nov 10, 2023
@Andarist
Copy link
Contributor

If you define a new rule around this then it will behave consistently (according to the defined rule) 😜

@ssalbdivad
Copy link
Author

ssalbdivad commented Nov 10, 2023

@Andarist Right, this is how @ahejlsberg originally described the criteria for having an "explicit type" in #32695 (comment):

An entity is considered to have an explicit type when it is declared as a function, method, class or namespace, or as a variable, parameter or property with an explicit type annotation. (This particular rule exists so that control flow analysis of potential assertion calls doesn't circularly trigger further analysis.)

Even as stated, I think the word function is ambiguous here and could include arrow functions, but worst case scenario you just add "or arrow function with annotated parameter and return types."

This would not be ambiguous with regard to cases like the object @RyanCavanaugh mentioned, which would not match any of these criteria.

@typescript-bot
Copy link
Collaborator

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

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants