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

Control flow analysis of aliased conditions is not able to narrow object properties #46412

Closed
5 tasks done
DavidZidar opened this issue Oct 18, 2021 · 7 comments
Closed
5 tasks done
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@DavidZidar
Copy link

Suggestion

πŸ” Search Terms

  • Control flow
  • Aliased conditions
  • Type narrowing

βœ… 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 4.4 added "Control Flow Analysis of Aliased Conditions and Discriminants" which is a great feature, but unfortunately it seems unable to narrow the types on object properties which limits the benefit.

πŸ“ƒ Motivating Example

Take null checks for instance, the following code results in a compiler warning in the AliasedControlFlow-method. The RegularControlFlow method works fine even though it does the exakt same comparison.

interface ITest {
    prop?: string;
}

function AliasedControlFlow(test: ITest) {
    const hasProp = test.prop != null;

    // Object is possibly 'undefined'.
    return hasProp ? test.prop.big() : "";
}

function RegularControlFlow(test: ITest) {
    return test.prop != null ? test.prop.big() : "";
}

πŸ’» Use Cases

This would really help when converting code that is currently not using strictNullChecks as null checks may already be aliased in existing code.

@ahejlsberg
Copy link
Member

The issue here is that prop is not a readonly property. From #44730, the PR that implemented the feature:

Narrowing through indirect references occurs only when the conditional expression or discriminant property access is declared in a const variable declaration with no type annotation, and the reference being narrowed is a const variable, a readonly property, or a parameter for which there are no assignments in the function body.

@ahejlsberg ahejlsberg added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Oct 18, 2021
@DavidZidar
Copy link
Author

@ahejlsberg Thank you for the response! In this case readonly can not be used as it is an object.

'readonly' type modifier is only permitted on array and tuple literal types.

I'm not sure I understand the last part of the paragraph, there are no assignments in the function body except for the alias.

Is this a use case that is intended to be fixed in the future or is the limitation too much work to resolve?

@DavidZidar
Copy link
Author

The language keyword readonly did not work but the Readonly<T> mapped type does work, it may take care of some of the use cases as long as all properties on the type can be readonly.

@MartinJohns
Copy link
Contributor

The property must be marked readonly:

interface ITest {
    readonly prop?: string;
}

@DavidZidar
Copy link
Author

@MartinJohns Right, I understand, that's what Readonly<T> does when used on the function argument.

@ahejlsberg
Copy link
Member

Is this a use case that is intended to be fixed in the future or is the limitation too much work to resolve?

It's basically a cost/benefit trade-off in control flow analysis. In order to support mutable properties we'd have to check that there are no assignments to a property between the declaration of the aliased condition that references the property and the check of that aliased condition. It's possible to do so (anything is possible), but it is non-trivial and it's not clear the added complexity and potential performance cost is worth it.

@DavidZidar
Copy link
Author

@ahejlsberg I understand, that sounds reasonable. There could be a lot more code between the alias and the usage.

Do I close this issue now or leave it open for further discussion?

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

4 participants