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: Fix select.nameSpan for expressions in parentheses #4907

Merged
merged 1 commit into from
Jan 28, 2023

Conversation

jkciesluk
Copy link
Member

For expressions like (1 + 2).toString, span was incorrect, because they were treated as right associative infix operators.

if span.exists then
val point = span.point
if sel.name.toTermName == nme.ERROR then Span(point)
else if sel.qualifier.span.start - span.start >= realName.length
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is a copy of sel.nameSpan, but here we check if
sel.qualifier.span.start - span.start >= realName.length, so name would fit as infix operator. This is still not perfect for operations nested in multiple parentheses, but should work for simple cases

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding this though I think we can make the solution more reliable.

Comment on lines 390 to 399
private def selectNameSpan(sel: Select)(using Context): Span =
val span = sel.span
val realName = sel.name.stripModuleClassSuffix.lastPart
if span.exists then
val point = span.point
if sel.name.toTermName == nme.ERROR then Span(point)
else if sel.qualifier.span.start - span.start >= realName.length
then // right associative
Span(span.start, span.start + realName.length, point)
else Span(point, span.end, point)
else span
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private def selectNameSpan(sel: Select)(using Context): Span =
val span = sel.span
val realName = sel.name.stripModuleClassSuffix.lastPart
if span.exists then
val point = span.point
if sel.name.toTermName == nme.ERROR then Span(point)
else if sel.qualifier.span.start - span.start >= realName.length
then // right associative
Span(span.start, span.start + realName.length, point)
else Span(point, span.end, point)
else span
private def selectNameSpan(sel: Select)(using Context): Span =
val span = sel.span
if span.exists then
val point = span.point
if sel.name.toTermName == nme.ERROR then Span(point)
else if sel.qualifier.span.start > span.point
then // right associative
val realName = sel.name.stripModuleClassSuffix.lastPart
Span(span.start, span.start + realName.length, point)
else Span(point, span.end, point)
else span

I think we should have sel.qualifier.span.start > span.point instead and that should work reliably. point points to the place between qual and apply.

Otherwise, this will not work for larger amounts of parens, no?

We can most likely just raise the PR with a fix to Dotty, could you take a look into that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way looks more reliable, thanks!

| val a = (1 + 2 + 3).<<toStr@@ing>>
|}""".stripMargin,
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also add:

  check(
    "select-parentheses2",
    """|object Main {
       |  val a = (1 + 2 + 3) <<:@@:>> Nil
       |}""".stripMargin,
  )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was worth adding this test, now I know it doesn't work for scala 2 😅 . I will take a look on that in a follow up, the issue might be a little bit more complicated, because right associative operator is translated to block with synthetic symbol $rightassoc1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uch, yeah let's do a follow up and ignore for now

For expressions like (1 + 2).toString, span was incorrect,
because they were treated as right associative infix operators.
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tgodzik tgodzik merged commit 55f285b into scalameta:main Jan 28, 2023
bishabosha added a commit to scala/scala3 that referenced this pull request Sep 21, 2023
Semanticdb range on selectDynamic in `foo.bar` previously contained
`.bar` instead of `bar`

Connected to scalameta/metals#5621
Also closes #16771 by porting
scalameta/metals#4907
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.

2 participants