feat: Add dynamic type #5212
feat: Add dynamic type #5212
Conversation
0cd6888 to
3280fa1
Compare
7c799aa to
a1c883b
Compare
d982098 to
4f608db
Compare
semantic/monotype.go
Outdated
| @@ -906,6 +913,21 @@ func NewDictType(keyType, valueType MonoType) MonoType { | |||
| return mt | |||
| } | |||
|
|
|||
| // NewDynamicType will construct a new Dynamic MonoType | |||
| func NewDynamicType() MonoType { | |||
| builder := flatbuffers.NewBuilder(32) // FIXME: how big? | |||
There was a problem hiding this comment.
The size of this is still an open question. I'm not that familiar with flatbuffers, but it does seem to me like it should be possible to mark this as 0-sized so long as whatever fields are associated with the node size themselves.
I'm thinking in terms of the BaseNode fields we see like the location of the node, etc.
6545b5c to
e618539
Compare
| if let MonoType::Dynamic(_) = &t { | ||
| self.typ = t.clone(); | ||
| return Ok(()); | ||
| } | ||
|
|
There was a problem hiding this comment.
@onelson This and the accompanying change to Expression::Index.infer() were my solution to get the new type inference tests to pass. This seems... hacky though. Open to suggestions for other solutions.
There was a problem hiding this comment.
I think that looks fair. I'm no expert though. My reading of this says that when r is dynamic, assign the type of dynamic to expression accessing the property (which is what we want).
The fact you're cloning t to do this is okay, I think? Otherwise you could look to assign a new instance of the type (but I'm not sure why that'd matter). There's a flatbuffer pointer inside which I'm not sure how to construct by hand 😭
Someone else with more experience in this are should probably check this out. Maybe Jonathan has input.
Edit: As an aside, in the vectorize code, we frequently wrap the originally inferred type of things by cloning the existing monotype and passing it into a wrapper type.
The difference between this situation and the vectorize code is in this case we're talking about a whole new value instead of replacing existing expressions to change their typing.
There was a problem hiding this comment.
I'd probably want to get @Marwes to have some eyes on this. Type inference is a strange area of code and doesn't really work in ways that are procedural at all times so this condition may not be valid.
I, unfortunately, don't have any input on this.
There was a problem hiding this comment.
Doing more targeted checks like this seems better to me generally, but I could maybe see issues depending on the order that type inference runs. That is, this requires that we know that the type is dynamic when we reach this point in the code, we can't discover that it is dynamic later and then pass this check. I don't know if that is actually a problem though
There was a problem hiding this comment.
Did we ever figure this out? It seems to me like it's possible this condition might not work correctly if we haven't yet determined that the original object was dynamic.
Just to make sure, if it turns out this fails, we can just turn dynamic off and this section of code will just not matter, correct?
There was a problem hiding this comment.
I think the deal is all the fns that will construct Dynamic items will be in experimental, but iirc there's no reasonable way to feature flag this stuff.
It does follow that if user's don't construct dynamic items, this match arm won't matter.
There was a problem hiding this comment.
Building on what Owen said, nothing currently uses the dynamic type. The functions that will use it will come from the other issues in the epic. I think the patches for those issues will be easier to test.
There was a problem hiding this comment.
@jsternberg Curious about your thoughts on this. I don't want to merge this if you have serious doubts. However, this is currently blocking other work, so if there's a specific change I can make to get this PR across the line, I'm happy to make it.
There was a problem hiding this comment.
I just need @Marwes to give feedback on the type inference stuff. The type inference code is solid and I don't want it to be hacky so it needs to be solid and backed by the algorithm.
The compiler code is presently incredibly hacky and is already set for a refactor. The way dynamic would be implemented in a newer compiler would just be to have it built in from the ground up as an inherent part of the reflected value which is already how the vm proposal proposes things. So I'm just not really concerned with changes to the compiler to accommodate dynamic.
| if let MonoType::Dynamic(_) = &t { | ||
| self.typ = t.clone(); | ||
| return Ok(()); | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd probably want to get @Marwes to have some eyes on this. Type inference is a strange area of code and doesn't really work in ways that are procedural at all times so this condition may not be valid.
I, unfortunately, don't have any input on this.
|
@Marwes could you weigh in on the questions Jonathan raised above? |
e311659 to
177dfba
Compare
177dfba to
af90651
Compare
There was a problem hiding this comment.
I share some of the hesitation re: inference but I hope we will still have the room to make adjustments as we implement the other bits on top of this new type.
My feeling is it'll be much easier to smoke out problems re: inference once we can use Dynamic in acceptance tests.
There was a problem hiding this comment.
I think my own personal concerns are fine and this can be merged. I'd still defer to @Marwes's review. If he approves, I think we're good.
Rebased and modified version of #4762.
Closes #5140
Added some tests to make sure that retrieving a dynamic value via index or member access still returns a dynamic value. Calling
Dynamic()on any non-dynamic value produces anUnexpectedKindpanic (similar to the other methods in the value interface). Similarly, attempting to callInt(),UInt(),Float(), etc. on a dynamic type also produces anUnexpectedKindpanic. This is to enforce the rule that dynamic values must be explicitly cast to the appropriate type in order to be used as such.