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

Obfuscate variable names generated by the compiler #1601

Merged
merged 7 commits into from Oct 29, 2019

Conversation

charles-cooper
Copy link
Collaborator

@charles-cooper charles-cooper commented Sep 5, 2019

This blocks the user from creating a collision with a compiler-generated
variable at the parser level.

What I did

Fix #1600

How I did it

How to verify it

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

This blocks the user from creating a collision with a compiler-generated
variable at the parser level.
@davesque davesque self-requested a review Sep 5, 2019
@@ -146,8 +146,8 @@ def is_valid_varname(self, name, pos):

# TODO location info for errors
# Add a new variable
def new_variable(self, name, typ, pos=None):
if self.is_valid_varname(name, pos):
def new_variable(self, name, typ, anon=False, pos=None):
Copy link
Contributor

@davesque davesque Sep 5, 2019

Choose a reason for hiding this comment

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

Should we rename anon to be something more descriptive? For example, maybe we could have check_varname=True instead. It would also be nice to add a note about why that option exists and why it's useful.

Copy link
Contributor

@davesque davesque Sep 5, 2019

Choose a reason for hiding this comment

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

I'm also seeing a couple of other places (like here) where internal variables are being created without the new_placeholder method. So it might actually make more sense to have a new method, new_internal_variable, which does the variable name mangling and ensures that the provided name won't collide with the namespace in the vyper source file. Then new_placeholder could just make a call to that without manually mangling its variable name.

Copy link
Collaborator Author

@charles-cooper charles-cooper Sep 6, 2019

Choose a reason for hiding this comment

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

Good catch and suggestion. I'm thinking of just putting the mangling option into new_variable instead of a whole new function.

Copy link
Collaborator Author

@charles-cooper charles-cooper Sep 6, 2019

Choose a reason for hiding this comment

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

@davesque let me know what you think of be9fc1d

find all places where internal variables get created and set the flag to
mangle.
@charles-cooper charles-cooper changed the title WIP Obfuscate variable names generated by the compiler Obfuscate variable names generated by the compiler Sep 6, 2019
vyper/parser/context.py Outdated Show resolved Hide resolved
@charles-cooper
Copy link
Collaborator Author

@charles-cooper charles-cooper commented Sep 8, 2019

@davesque anything else you would like to see on this?

@fubuloubu fubuloubu requested a review from davesque Sep 10, 2019
Copy link
Member

@fubuloubu fubuloubu left a comment

LGTM, I think can merge pending @davesque's follow-up

@charles-cooper charles-cooper merged commit 865ea44 into vyperlang:master Oct 29, 2019
3 checks passed
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.

4 participants