Type disambiguation rule 1 (TD1)#257
Conversation
When multiple arguments to a VDF have the same custom type, then they all have the same concrete type.
villagestevers
requested review from
dbentley-vsql,
malone-at-work and
tomas-villagesql
malone-at-work
left a comment
malone-at-work
left a comment
There was a problem hiding this comment.
We are doing more string casting and conversion than we need to.
| if (expected_type.id != VEF_TYPE_CUSTOM) { | ||
| continue; | ||
| assert(expected_type.custom_type != nullptr); | ||
| const std::string expected_qbn = make_qualified_base_name( |
There was a problem hiding this comment.
We should probably have make_qualified_base_name take string_views, because in multiple places we call it now, we create a std::string, just to pass in.
Also, we may just want to generate the "qbn" at registration time.
There was a problem hiding this comment.
I think I fixed this in the next change - at least the extension name. We can't do it for qbn because we need to change the second part IIRC.
I won't do it as part of this change if that's ok. I did not use string_view, but I can look at that too as a cleanup if that's ok.
| if (tc != nullptr && tc->is_unknown()) { | ||
| auto it = known_params.find(expected_qbn); | ||
| if (it != known_params.end()) { | ||
| LEX_STRING lex_type_name; |
There was a problem hiding this comment.
Why a LEX_STRING? Can we fix this to be a std::string_view?
And I am going to say again, maybe we should preprocess the information in the Victionary to be the type we need.
There was a problem hiding this comment.
Can you elaborate on the second point?
For the first, the ResolveTypeToContext was using LEX_STRING because it was originally only used from Parsetree code (and might still be), which points into a query string as a pointer and a length (LEX_STRING). This is in spirit the same as a std::string_view, but why would we change what we have over to that?
There was a problem hiding this comment.
First point: all this casting to LEX_STRING is needless noise. ResolveTypeToContext() casts it to other things internally, never using the LEX_STRING type. LEX_STRING may have made sense at some point, but now multiple lines of this function are used to do a conversion to something that isn't used as that type, making it harder to understand the code with zero upside. I am fine fixing it in a follow up PR.
The victionary clients shouldn't cache the ABI types - we should have it cache the types we need at runtime. So if we really needed this as a LEX_STRING, the victionary client should have a LEX_STRING that it returns. If we often use a fully qualified name, let's store that, instead of constructing it at every site it is used.
malone-at-work
left a comment
malone-at-work
left a comment
There was a problem hiding this comment.
Is there a reason we can't write a test for this yet?
| return true; | ||
| } | ||
| if (resolved_tc != nullptr) { | ||
| args[i]->set_type_context(resolved_tc); |
There was a problem hiding this comment.
Random thought - should we have a 'set_type_context' and 'update_type_context' where the former asserts no existing type context, end the latter asserts that there is? (I feel like it may be easy for us to stomp on type context via bugs, but that it happens to work out)
There was a problem hiding this comment.
args[i] doesn't have a (known) type context by here, because it was checked earlier. We are in case 2, and that case is documented above.
There was a problem hiding this comment.
I am suggesting defensive programming here:
Today set_type_context() can be used in 3(ish) ways:
A) Set a type context where there was previously none (how we have used it in the past.
B) Set a (known) type context where there was a previously existing type context with unknown params.
C) Overwrite an existing type context.
Rather than having one function that can do all of these, let's have two functions (A) set_type_context() should assert there is no existing type context on that node.
(B) update_type_context() which would assert that the node already has an existing type context, and that the new one has parameters.
No function for (C) - I don't think we ever want to overwrite a type_context in any other case.
The new asserts in the new functions would provide checks to know that we didn't accidentally break the code.
Does this make more sense?
There was a problem hiding this comment.
I understood the comment - I am saying I don't know why this is the place to bring it up. It is well-understood. I guess I should have taken the cue from "Random thought".
I am fine with changing this in the future, but it is low value for now.
I don't know if the right thing is to enforce this, or simply to make sure that when B overwrites A, A and B are "compatible". Hard to say without thinking through all the potential cases thoroughly, and it is hard to say that there are no such cases (for C, I mean).
| continue; | ||
| assert(expected_type.custom_type != nullptr); | ||
| const std::string expected_qbn = make_qualified_base_name( | ||
| std::string(extension_name.str, extension_name.length), |
There was a problem hiding this comment.
Would it not be better if the signature already had the qbn encoded, instead of constructing it here? laying the foundation for not having to have TYPE and VDF in the same extension
There was a problem hiding this comment.
I.e have signature be:
vsql_complex.add(vsql_complex.COMPLEX, vsql_complex.COMPLEX) -> vsql_complex.COMPLEX
rather than:
vsql_complex.add(COMPLEX, COMPLEX) -> COMPLEX
3 participants
When multiple arguments to a VDF have the same custom type, then they all have the same concrete type.