Fix cross module constants and comprehension scoping#526
Conversation
…d MIR translation
Al-Kindi-0
changed the title
Soulthym
left a comment
Soulthym
left a comment
There was a problem hiding this comment.
Hey @Al-Kindi-0, I left a few comments below about clarifications and a minor nit regarding Boxing RangeExpr. The code itself looks good to me otherwise, great job!
| let current_item_node_index = self.get_node_index_or_add(¤t_item); | ||
| for (referenced_item, ref_type) in self.referenced.clone().iter() { | ||
| let referenced_item_node_index = self.get_node_index_or_add(referenced_item); | ||
| self.deps_graph.add_edge( |
There was a problem hiding this comment.
I'm assuming this is the part of the issue you encountered, we were overriding values without checking for their existence first?
This edit looks correct, just asking to confirm I understand the problem properly
There was a problem hiding this comment.
Indeed, we were calling add_node unconditionally, which could create duplicate nodes. get_node_index_or_add reuses the existing node index.
| Default, | ||
| /// Access binds a sub-slice of a vector | ||
| Slice(RangeExpr), | ||
| Slice(Box<RangeExpr>), |
There was a problem hiding this comment.
Is there a reason to switching to a Box here instead of an owned RangeExpr?
There was a problem hiding this comment.
Slice(Box<RangeExpr>) is there mainly to avoid the clippy::large_enum_variant warning.
Leo-Besancon
left a comment
Leo-Besancon
left a comment
There was a problem hiding this comment.
Nice catches, thank you!
LGTM, I was wondering if the fix for the bindings from PeriodicColumn to local variable could have any impact but I don't see how, and if the tests don't catch regressions it's probably good!
Al-Kindi-0
deleted the
al-fix-cross-module-constants-and-comprehension-scoping
branch
3 participants
Describe your changes
This PR fixes two bugs related to cross-module imports and comprehension scoping:
Cross-module constant dependencies:
Constants used in comprehension iterables fail when imported across module boundaries.
Comprehension periodic binding scoping:
Iterating over a vector containing periodic column references causes the binding to be mistyped as PeriodicColumn, leading to "no entry found for key" panic.
Checklist before requesting a review
nextaccording to naming convention.CHANGELOG.md