Type deduction RFC#4
Conversation
thegedge
left a comment
thegedge
left a comment
There was a problem hiding this comment.
Looks good, @LhKipp . I only focused on the content that was present, but I'll think some more about things that may have been missed and come back and add additional comments (if I think of anything missing). I added a few nitpicks, and some more important comments (which I've highlighted with a
One interesting thing I found with this RFC was that you're basically describing a large surface area of the parser. One thing that we're kind of missing right now is a good description of our parser. It's making me think about whether or not we could somehow maintain such a description 🙂
Since @jonathandturner has been reviewing some of the code you've written, and has a lot more context on this space, I'm going to lean on his thumbs up as the final 👍 to take this to consensus with the team.
| (false, true) -> set_union_excluding_any(existing_deductions, set_intersection(existing_deductions, new_deductions)) | ||
| (false, false) -> set_intersection(existing_deductions, new_deductions)? | ||
| } | ||
| } |
There was a problem hiding this comment.
I wonder if this would be simpler to reason about if we expanded SyntaxShape::Any into the set of types it can represent, and then did checked_insert like above (just a set intersection) 🤔
There was a problem hiding this comment.
I was just wondering that too. A kind of SyntaxShape::OneOf(a, b, c) form.
There was a problem hiding this comment.
I wonder if this would be simpler to reason about if we expanded
SyntaxShape::Anyinto the set of types it can represent, and then didchecked_insertlike above (just a set intersection) thinking
After thinking this through, I am not sure whether it would simplify the implementation.
Looking closes at the pseudo code, it is obvious that:
(true, false) -> set_union_excluding_any(new_deductions, set_intersection(existing_deductions, new_deductions))is the same as:
(true, false) -> new_deductionsThe reason I have written it down like this is, that meta information (where was this deduction be made?) needs to get merged. If SyntaxShape::Any would be expanded into every type, handling the meta data correctly might be a pain in the concrete implementation.
I was just wondering that too. A kind of
SyntaxShape::OneOf(a, b, c)form.
OneOf should then probably contain a vector of SyntaxShape variants? From my point of view this is almost the same as a vector of SyntaxShape's. It may make the code a little more declarative, but at the added cost of having a different size for the SyntaxShape enum. I would like to stick with the current pseudo code.
|
@LhKipp I just want to add a big wow! and thank you! for this RFC. Lots of detail here. |
|
@thegedge Thanks for your feedback. The comments I haven't answered are all valid. I will update the rfc and the implementation hopefully some time soon. I am quite bussy the next 2.5 weeks. Please first expect major progress after 2.5 weeks :). |
|
Thanks, @LhKipp. No rush. Excited to see this come together, on whatever timeline works best for you 🙂 |
|
Thanks everyone. Your comments helped a lot to polish up this rfc. |
|
Thanks all. And thanks @LhKipp for putting this together. Accepting this RFC. |
4 participants
No description provided.