fix(jsii-pacmak): underscore arguments conflict in Java #5011
Conversation
The `_` identifier is treated as a keyword since Java 9, and we used to escape it to `__`. The problem is that but it is somewhat likely that people will use `_, __, ___` as multiple indifferent arguments, and now the identifiers will conflict. We have to pick something else. Ideally we would look at the surrounding argument names and pick something guaranteed to be unique, but unfortunately the code isn't quite structured that way so we'll pick something unlikely to collide instead. We escape it to `_under_` instead. Changing from `__` -> `_under_` would be a breaking change if applied to public property names, but most likely this will be used for function parameters (unfortunately the code has been structured in such a way that property and parameter names are strongly tied together, in a way that would take more time to unwind than I care to invest right now), where it doesn't matter.
| if (!name) { | ||
| // toCamelCase will return an empty string from a string like `_(_+)`. Confirm that | ||
| // that is what is happening, then return the original string. | ||
| if (original.match(/^__+$/)) { |
There was a problem hiding this comment.
This change is not explained in the PR body - why do we need to return __ here instead of the empty string like it was?
There was a problem hiding this comment.
Because an empty string is not a valid variable name, and it leads to compiler errors.
I saw the following after my change:
public void method(String _, String , String ) {
}We didn't have a test on this before. I have to agree I don't quite understand how this didn't trigger on real-world code that used the _, __, ___ idiom... but nevertheless this was a latent bug.
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
|
Merging (with squash)... |
|
Merging (with squash)... |
Merge Queue Status✅ The pull request has been merged at 4b57963 This pull request spent 6 seconds in the queue, with no time running CI. Required conditions to merge
|
2 participants
The
_identifier is treated as a keyword since Java 9, and we used to escape it to__.The problem is that but it is somewhat likely that people will use
_, __, ___as multiple indifferent arguments, and now the identifiers will conflict. We have to pick something else.Ideally we would look at the surrounding argument names and pick something guaranteed to be unique, but unfortunately the code isn't quite structured that way so we'll pick something unlikely to collide instead. We escape it to
_under_instead.Changing from
__->_under_would be a breaking change if applied to public property names, but most likely this will be used for function parameters (unfortunately the code has been structured in such a way that property and parameter names are strongly tied together, in a way that would take more time to unwind than I care to invest right now), where it doesn't matter.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.