Pass sequence types downstream #2996
Conversation
There was a problem hiding this comment.
This is separate from any validity checking, which will be implemented in separate PRs.
Nope, checking has to be in this PR. Or more specifically, verifying types and generating error messages has to be done in this PR (the actual UI can be done later). As I point out in my code comments, the lack of checks just makes the entire type propagation incorrect, so this isn't an optional thing.
| } | ||
| type = newType; | ||
| } | ||
| if (assignedType.sequence) { | ||
| inputLengths.set(id, assignedType.sequence); |
There was a problem hiding this comment.
This is wrong. You just set the input seq type regardless of whether it's actually assignable. What if a node requires a seq of length 10, and it's assigned a seq of length 20?
However, that's not all. Fixing assignability aside, there's also a problem with assigning the seq type per input. That's wrong as well. E.g. if you have a collector with 2 iterated input mapping to the same iterator input info, then assigning any one of them narrows the type of the other.
As an example, imagine a Merge Sprite sheet node that takes an image input and an index input (to give users absolute control over the order of images). The Image and Index inputs would both need to be of the same length (so they would use the same iter input info internally).
And there's a third problem related to other seq properties. If we add more properties to the Sequence type (and I plan on doing that in the future), then it will be hard to define the type value of the IterInput<x> variable(s).
Using the modified Merge Sprite sheet node from above: Assuming that the Image input is assigned Sequence { length: 10, foo: true } and the Index input is assigned Sequnce { length: 10 | 20, foo: false }, what should the type of IterInput0 be? It can't be Sequence { length: 10 | 20, foo: true | false }, because lengths must be narrowed, not widened. But it also can't be Sequence { length: 10, foo: never }, because that would evaluate the never type, which implies that an assignability error, even though this assignment should be fine.
To summarize: this change brings 3 problems:
- No assignability check.
- No type narrowing for related iterated inputs.
- No clear way to define the type of the iterator.
| const edgeSource = getSourceType(n.id, id); | ||
| if (edgeSource) { | ||
| if (edgeSource.output && edgeSource.sequence) { |
There was a problem hiding this comment.
I'm not convinced that this is correct. Please explain why you made this the new condition, specifically, why you check both.
| if (definition.schema.kind === 'regularNode' && inputLengths.size > 0) { | ||
| // This only works because we assume sequences from a single node are all the same size. | ||
| // In the future, this might not be the case. | ||
| outputLengths.set(item.output.id, [...inputLengths.values()][0]); | ||
| } |
There was a problem hiding this comment.
The whole point of your recent iterator changes was to allow connecting 2 different iterators of a single node if they have the same length. You can't just assume they have the same length, in the system that is supposed to verify this very fact.
|
I'm not capable of doing this correctly |
Allows sequence types to get passed down to regularNodes that inherit a sequence. This is separate from any validity checking, which will be implemented in separate PRs.