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

Fix abi_type_strategies.sized_list_strats #43

Merged
merged 1 commit into from
Apr 5, 2018

Conversation

davesque
Copy link
Contributor

@davesque davesque commented Apr 4, 2018

What was wrong?

The values of type_str and type_strat are not determined until the lambdas are evaluated.
Therefore, every resulting lambda references the values in the last tuple in all_basic_raw_strats which happen to be for address sampling.

This causes the resulting strategy to only generate address lists instead of a list for any possible type in all_basic_raw_strats.

Also, the shared value key should probably be more unique.

How was it fixed?

Said values are now explicitly captured in lambdas. Also changed shared value key.

Cute Animal Picture

Cute animal picture

The values of `type_str` and `type_strat` are not captured in the
closures of each lambda.  Therefore, every resulting lambda references
the values in the last tuple in `all_basic_raw_strats` which happen to
be for address sampling.

Also, the shared value key should probably be more unique.
@carver
Copy link
Collaborator

carver commented Apr 4, 2018

Interesting, I couldn't have told you what this would do without trying it:

> [m() for m in [lambda: v for v in range(3)]]
[2, 2, 2]

So the idea is that the lambda closes around v and the for loop mutates v, so all three lambdas here return the final value, right?

But this works, because it would invoke before v changes:

> [(lambda: v)() for v in range(3)]
[0, 1, 2]

Good catch!

@davesque
Copy link
Contributor Author

davesque commented Apr 4, 2018

@carver Yep. Actually, my choice of words was poor and the way you're talking about it is more correct. The problem is not that the variables are not closed over. Quite the opposite. Adding a default value for a keyword argument forces Python to evaluate the variable when the lambda is defined.

@carver carver merged commit 81b2835 into ethereum:master Apr 5, 2018
@davesque davesque deleted the fix-sized-list-strats branch April 9, 2018 07:01
carver pushed a commit to carver/eth-abi that referenced this pull request Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants