Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Golang: fix flow from a map value via a range statement #15613

Merged

Conversation

smowton
Copy link
Contributor

@smowton smowton commented Feb 14, 2024

Flow such as the following was broken:

var someMap map[string]string
someMap["someKey"] = source()

for _, val := range someMap {
  sink(val)
}

This is because the store-step relating to the map store used content-kind "map.value", while the read via the range statement used content-kind "array".

Note the case for reading array content now always constrains the read node to have array or slice type, as array content was previously being read from the base over a range statement iterating over a map.

@smowton smowton requested a review from a team as a code owner February 14, 2024 15:01
@github-actions github-actions bot added the Go label Feb 14, 2024
@smowton
Copy link
Contributor Author

smowton commented Feb 14, 2024

DCA: no changes.

@owen-mc
Copy link
Contributor

owen-mc commented Feb 15, 2024

Good catch! How did you notice this? (Edit: I just found where it was reported.) Could you add a test that failed before and now passes?

@mbg
Copy link
Member

mbg commented Feb 15, 2024

@owen-mc FYI, @smowton asked me to review this yesterday and I ended up writing a test for the review which seems to suggest this isn't working (unless I have done something silly). I should have commented that on here rather than on Slack.

@smowton
Copy link
Contributor Author

smowton commented Feb 23, 2024

Found the additional change needed to make this work for a stateful map -- the case I'd fixed previously was the case of a map literal, where the literal node is its own post-update node. Also added a test as requested.

owen-mc
owen-mc previously approved these changes Feb 27, 2024
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Suggestion for improvement: also have a test for a map literal.

@smowton
Copy link
Contributor Author

smowton commented Feb 27, 2024

@owen-mc done, please re-approve

@smowton smowton merged commit 9f84653 into github:main Feb 27, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants