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

Added recursive check for nested tuple elements #43567

Closed
wants to merge 1 commit into from

Conversation

armanio123
Copy link
Member

Fixes #43516

Due to how the checker works, this algorithm will recursively check any nested tuples multiple times. I didn't found yet a way to reduce this unnecessary work. I'm open to ideas.

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Apr 7, 2021
@DanielRosenwasser
Copy link
Member

I think the spirit of the check was never really to dive deep into spreads. If you hit a spread, you probably want to just bail out from the check on that individual member, regardless of whether you have a tuple as the direct child. I'd check with others to see if that's the case.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

I was imagining this would happen during createNormalizedTupleType, which I think would let you avoid recursion, and would also ensure this works:

declare const point2d: [x: 1, y: 2];
declare const point3d: [...typeof point2d, z: 3]; // should be ok

But it may also be the case that this is harder than it sounds, and the correct answer for now is it’s not worth fixing. But I think worth trying is an approach like this:

  1. In checkTupleType instead of the recursive check, simply ignore any spreads.
  2. In createNormalizedTupleType, expandedDeclarations tracks whether any of the tuple elements have labels. When it’s an empty array, we don’t know yet. When it’s non-empty, at least one member had a label, so we assume that all do. When it’s undefined, at least one member lacked a label, so we assume that all do. In addElement, expandedDeclarations gets silently mutated to undefined in that latter case. If it ever goes from non-empty to undefined, or if it’s ever undefined and declaration is defined, we have an inconsistency that can be reported on currentNode (or on declaration, or on the last declaration that was defined).

@armanio123
Copy link
Member Author

I've been checking createNormalizedTupleType and looks like it will be a bit more complicated than that. Mainly because we need to add to elementTypes that one of the parameters is a NamedTupleMember (in your example that would be z: 3).

Currently we get that information for the nested tuple, but for the other parameter we just have a type number, so we are unable to detect if this is named or not.

On the other hand ignoring the spread is super easy to achieve.

I'm guessing the concerns on the recursion is due to performance. If you feel is worth it, instead of recursion I can just check one level deep and ignore any rests' if found after that.

Let me know what you think.

@andrewbranch
Copy link
Member

It looks like a small change will be necessary to getArrayOrTupleTargetType and getTupleTargetType to allow a holey array of namedMemberDeclarations. Currently, if any are elements are missing names, none are set. (This makes sense for the current rules, where named-ness is determined up front and an error is issued if any elements can’t be syntactically determined to have names. But with your change, that rule no longer makes sense.) Setting the namedMemberDeclarations even if some are missing should allow you to get the info you need in createNormalizedTupleType.

I'm guessing the concerns on the recursion is due to performance.

Not really, it’s that the example I gave in my last comment wouldn’t work. You can’t determine that ...typeof point2d has names by syntax alone. The recursive syntactic check would only work for types like [one: 1, ...[two: 2]], which there’s no reason to write. The only reason to spread one tuple type into another tuple type is if you’d already written the first one somewhere else and didn’t want to repeat it. So the feature isn’t worth doing at all unless it can reason about [one: 1, ...SomeOtherType].

@armanio123
Copy link
Member Author

As discussed this issue is invalid as the syntax is by design.

@armanio123 armanio123 closed this Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Spreading one named tuple in another throws an error
4 participants