#136 fix a bug in scoping of the let clause#137
#136 fix a bug in scoping of the let clause#137vfrank66 wants to merge 2 commits intogoogle:mainfrom
Conversation
…ng with inner scoped let clause
|
@evan-gordon I realize maybe this could be broken into 2 PRs but the problem was cascading i was checking a ahrq cql file and the scoping issues had two bugs one for the let clause at the root and a second one for nesteed let clause within a sort clause |
Thanks for the context. |
evan-gordon
left a comment
evan-gordon
left a comment
There was a problem hiding this comment.
Thanks @vfrank66 !
This seems like a pretty good change overall, my main concern below is about making sure that this change properly cleans up aliases after the scope is exited properly. Once comments are addressed I'll approve.
| t.Errorf("ResolveLocal(A) = %v, want %v", got, "updated_value") | ||
| } | ||
|
|
||
| // Test error case - update non-existent alias |
There was a problem hiding this comment.
Mind making this it's own test case?
| } | ||
| return av.Compare(bv) | ||
| case types.Integer: | ||
| av := ap.GolangValue().(int32) |
There was a problem hiding this comment.
for all of these conversions you can use applyToValues(av, bp, result.ToInt32) to do the conversion for both values to the desired type.
| filteredIters := []iteration{} | ||
| for _, iter := range iters { | ||
| for _, relIter := range relIters { | ||
| i.refs.EnterScope() |
There was a problem hiding this comment.
My main concern with this change would be to check if it properly cleans up aliases, refs.EnterScope() and refs.ExitScope() should handle making sure aliases are correctly scoped for the right context.
Does this solution properly clean up it's values when exiting a specific scope? Perhaps we should add tests that:
- inside
TestCrossLibraryAliasResolutionquery1 declares a variableMcalls function that starts query2, that also declares a variableM.Min the outer context should still be the right value after the inner query runs. - An error case where similar to the above case, but the outer query doesn't define
M, ifMis referenced after the inner query it should throw some meaningful error about the identifier not being declared in this scop.
2 participants
No description provided.