Incorrect handling of unicode escape sequences when calculating source locations

I believe that at least part of the problem comes from org.codehaus.groovy.antlr.UnicodeEscapingReader, where an escape sequence is translated into a single character. But, I think there is also a problem with the GroovyRecognizer in that it creates source locations based on the translated escape sequences, rather than the raw text.

Please note that this problem is much bigger for Groovy-Eclipse since it is reliant on precise source locations in order to get editor operations (like content assist and underlining) working.

If UnicodeEscapingReader would not replace the sequence with the unicode "character" alone, but instead fills up using spaces, then the source position would be right again. And then no change to GroovyRecognizer would be needed, especially no manual sourceposition translation. Andrew, do you think that could work?

No, this probably won't work. An escape sequence in the middle of an identifier would cause problems. Eg:

foo\u8706foo

Would be sent to the recognizer as:

foo∂     foo

(oops---when I first entered it, &#8706 was an actual unicode character)

Now, what was once 1 identifier would become two.

the problem is how to resolve the issue then... allowing unicode escape sequences in identifiers?

Another possibility (and I admit that I haven't completely thought this through) would be to add a new method to UnicodeEscapingReader that returns the number of chars read on the last call to read (eg- getBytesRead() ). Then GroovyLexer would presumably update its source locations appropriately after it calls getBytesRead().

Since GroovyLexer is a subclass of an antlr class and it looks like the call to read() happens in library code, the suggestion above may not work.

So, another possibility that I can see would be for GroovyLexer to perform the unescaping of unicode chars itself. This would of course require a change to the grammar.

Any chance that we can have this fixed? Users are hitting the occasionally (but regularly) and it is always confusing and disruptive for them.

the current state seems to be that we need to change the grammar and convert unicode by ourselfes

another example:

\u00E9 = new I_AM_WRONG()

I tried modifying the LETTER Lexer rule, but then this swallowed all \, so that alone does not do it.

Here is a potential solution for this problem:

When I say the actual text, I mean the text in the source file. When I say the parser text, I mean the text that the parser sees and that has the text collapsed around unicode escapes.

1. It is easy enough to have the lexer keep track of where unicode characters are in the parser text.
2. This would be some sort of datastructure that can translate from pasrer text locations to actual text locations and is built up during lexing.
3. Then, in GroovyRecognizer.create(), if the passed in AST is of type GroovySourceAST, we use the datastructure created above to translate the source locations from parser text to source text.

I'm not really a big fan of this solution, but it is minimally invasive and would probably work. The hardest part would be to get the datastructure right so that it is efficient in time and in space (although even that shouldn't be too hard).

I committed a fix for this to Groovy-Eclipse. I am attaching the patch that I used. See GRECLIPSE-805 for a description of what the changes are. The changes are minimally invasive, and negligible in overhead. The patch will not apply directly to groovy core since Groovy-Eclipse uses a patched 1.7.8 version. However, you should be able to look at it and determine what should be done.

Forgot to attach the patch.

I attached the pach I am going to apply. It is a bit different from yours, as several parts in your patch don't seem to apply to normal Groovy. That is mainly the SourceBuffer part. We don't have a getLocationSupport method and no lineendings list for example. And we also have no error recovery ast class.
Another change to your code is that our unicdoe reader seems to allow multiple u in a escape sequence. So I added a counter for them instead of having the hard coded 5 offset alone.

Thanks for this. As I mentioned, I had to create the patch off of Groovy-Eclipse, where we have already made quite a few changes in this area.

Do you think that accepting escape sequences like \uuuuuAE09 is a bug? I can't see any reason why this would be acceptable.

Until I hear back from you one way or the other, I will amend the fix for GroovyEclipse so that it can accept multiple 'u' chars.

well, be it bug or not, it was accepted before, so I think it is not good to have that changed now for 1.7. I ahd to change the patch a bit more, because of some tests that use the Reader without a lexer. I added a dummy lexer that is used if no lexer is set that gives some fake line and column numbers, just to satisfy the tests. I committed this changed version to 1.7, 1.8 and 1.9 already