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

Bugfix: Prefix generated static variables to avoid reserved keyword clashing #559

Closed

Conversation

mcos
Copy link

@mcos mcos commented Dec 15, 2016

I noticed earlier while playing around with this library that if a protobuf file namespace has reserved keywords, then a variables are generated with reserved keyword names. Importing the file then causes errors.

syntax = 'proto3';

package var.for.switch;

message Sampler {
	string sample = 1;
}

Compiling this and requiring it gives the following:

var var = {};
            ^^^
SyntaxError: Unexpected token var

To get around this, I simply prefixed the generated variable name with $, which seemed to be in keeping with the conventions of this library. These variables are generated within closures, so they are not assigned anywhere that they may be accessed by a consumer of the generated code.

I understand that this fix may be over-simplified, so I'm happy to work on a better solution than this. I also understand that there are no tests for this module yet, but if some were added, I'd be happy to contribute a test case for this.

@dcodeIO
Copy link
Member

dcodeIO commented Dec 15, 2016

There are a few issues with this:

  • While the declaration is renamed, subsequent uses are not (i.e. when returning from the closure or assigning properties).
  • If a namespace is named protobuf, it will hide the global $protobuf var for example.

@dcodeIO
Copy link
Member

dcodeIO commented Dec 15, 2016

Does this fix it?

@mcos
Copy link
Author

mcos commented Dec 15, 2016

Ah yeah, that was hacky of me, and I see now that $protobuf would be shadowed, which is really not good. Thanks for the fixes in be3e0d9 and 9b7b92a!

@mcos mcos closed this Dec 15, 2016
@mcos mcos deleted the bugfix/static-variable-keyword-clashing branch January 10, 2017 18:05
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.

None yet

2 participants