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

nameSpan is incorrect for backquoted names with escaped characters #16234

Open
TheElectronWill opened this issue Oct 23, 2022 · 4 comments
Open

Comments

@TheElectronWill
Copy link
Member

TheElectronWill commented Oct 23, 2022

Compiler version

3.2.1-RC2

Minimized code

def `\r\n`() = ()

Output

val methodDef: DefDef = ... // get the DefDef of the method
val namePos = methodDef.namePos
println(namePos.span)
[45..47] // the span is only 2 chars long

The span is wrong because Trees#nameSpan computes the span's end from realName.length, but since the characters were escaped the count is wrong.

This is annoying for coverage reports, where namePos is used to "highlight" the method name in the coverage report.

Expectation

The correct span should match the original source, thus it should be [45..49] (not sure whether the quotes should be included or not).
The Parser produces an Ident with the correct span (here, I think), could this be used instead?

@TheElectronWill TheElectronWill added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Oct 23, 2022
@Kordyjan Kordyjan added area:parser and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Oct 24, 2022
@odersky
Copy link
Contributor

odersky commented Oct 25, 2022

The problem is that doing this would grow significantly the size of all trees, lowering performance, whereas the use case where this matters is very marginal. If we can come up with a scheme that does not increase the size of definition nodes, fine. Otherwise it should be a won't fix.

@TheElectronWill
Copy link
Member Author

TheElectronWill commented Oct 26, 2022

Mitigation idea: when computing the namePos, escape the characters of the name if needed. In the example I gave, we know that the name must have been written `\r\n` because CRLF isn't a valid name without backticks and escapement.

Speaking of backticks, the REPL could automatically add backticks and escapes where needed, to avoid this:
image

that would be another issue, but the two might be related because the automatic backticking+escaping would be the same.

@dwijnand
Copy link
Member

dwijnand commented Oct 26, 2022

Mitigation idea: when computing the namePos, escape the characters of the name if needed.

That doesn't help def `foo` = 1, which doesn't need backticks, but is still 5 characters long rather than 3.

@TheElectronWill
Copy link
Member Author

That doesn't help def `foo` = 1, which doesn't need backticks, but is still 5 characters long rather than 3.

Without having a "backticked" flag there's not much we can do then 🤔 I've thought about making the name start at ` instead of f, and then detecting the backtick, but removing the backtick everytime we use the name is too heavy.

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

No branches or pull requests

4 participants