Skip to content
This repository has been archived by the owner on Mar 7, 2024. It is now read-only.

fix im union #271

Merged
merged 1 commit into from
Jul 5, 2022
Merged

fix im union #271

merged 1 commit into from
Jul 5, 2022

Conversation

franziskuskiefer
Copy link
Member

This allows us updating im.
I didn't update all union calls. But we probably should. Or maybe we should drop im and use the std::collections instead.

Also see bodil/im-rs#202

@franziskuskiefer franziskuskiefer self-assigned this Jun 28, 2022
@franziskuskiefer franziskuskiefer marked this pull request as ready for review June 28, 2022 12:42
let name_context = new_name_context.union(name_context.clone());
let new_arm = resolve_expression(sess, arm, &name_context, top_level_ctx)?;
let mut updated_name_context = name_context.clone();
for (k, v) in new_name_context.into_iter() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an issue with union in im 15.1.0?

Copy link
Member Author

@franziskuskiefer franziskuskiefer Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, see the link in the PR description bodil/im-rs#202

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmester0 any thoughts on this?
This PR should at least unblock the update, which we should do due to the security issue.
But I think we should revisit the way the hashmaps work later. I feel like we should probably move away from im given this issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like a strange implementation of union, but I think the pull request is fine. I agree that we should probably find something that has the "correct" union implemented.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a little weird. But I'd like to update the dependency at least.
I'll merge this and file a follow-up to replace im.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmester0 can you r+ so I can merge? 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@franziskuskiefer franziskuskiefer enabled auto-merge (rebase) July 5, 2022 12:26
@franziskuskiefer franziskuskiefer merged commit a866512 into master Jul 5, 2022
@franziskuskiefer franziskuskiefer deleted the fix-im branch July 5, 2022 13:47
@franziskuskiefer franziskuskiefer mentioned this pull request Jul 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants