Skip to content

Use full isReadonlySymbol check rather than declaration flags #48064

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

Merged
merged 3 commits into from
Feb 28, 2022

Conversation

weswigham
Copy link
Member

When comparing property readonly-ness for subtype checks. This fixes comparing a mapped type readonly prop (which gets a Readonly check flag) with a declared readonly prop (which has a Readonly modifier flag, but no check flag).

That alone fixes #47940 with the caveat that the type guard should look for a readonly field rather than a mutable one (since narrowing is by subtype and not assignability, and ArrayLike has a readonly length property).

In addition, I also limit the new subtype check to only the strictSubtypeRelation, as used by subtype reduction, so as to not change behavior in control flow, and thus fixes #47940 completely.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Feb 28, 2022
@jakebailey
Copy link
Member

Testing on DT, this does fix the lodash issue, though I'm not sure if the test on the PR is comparable (it's the same as the has test in the original issue).

with the caveat that the type guard should look for a readonly field rather than a mutable one (since narrowing is by subtype and not assignability, and ArrayLike has a readonly length property).

Would you suggest I un-draft (and retitle) this: DefinitelyTyped/DefinitelyTyped#58979

@weswigham
Copy link
Member Author

Nah, DefinitelyTyped/DefinitelyTyped#58979 shouldn't be needed with this with the change in this PR that limits the check to strict subtype only.

@jakebailey
Copy link
Member

Thanks, I'll close it.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, although I'm not 100% sure how the updated comment should explain it.

@@ -19747,9 +19747,10 @@ namespace ts {
// This ensures the subtype relationship is ordered, and preventing declaration order
// from deciding which type "wins" in union subtype reduction.
// They're still assignable to one another, since `readonly` doesn't affect assignability.
// This is only applied during the strictSubtypeRelation for compatability's sake in control flow
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be better to include which operations will use this, something like:

Suggested change
// This is only applied during the strictSubtypeRelation for compatability's sake in control flow
// This is only applied during the strictSubtypeRelation -- currently used in subtype reduction

@weswigham weswigham merged commit 7191875 into microsoft:main Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#47069 breaks narrowing in @types/has and @types/lodash
4 participants