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
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@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

This comment has been minimized.

Owner

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 added a commit that referenced this pull request Dec 15, 2016

dcodeIO added a commit that referenced this pull request Dec 15, 2016

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 15, 2016

Does this fix it?

@mcos

This comment has been minimized.

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 mcos:bugfix/static-variable-keyword-clashing branch Jan 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment