Skip to content

Infix operation desugaring should preserve original span #22703

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rochala
Copy link
Contributor

@rochala rochala commented Mar 4, 2025

Found during debugging of #22566

Basically, we created new apply based on arg inside Parens that led to shorter span.

        case Parens(arg) =>
          Apply(sel, assignToNamedArg(arg) :: Nil)

@som-snytt
Copy link
Contributor

som-snytt commented Mar 6, 2025

Sample diff of my suggestion that moves the point

--- a/tests/explicit-nulls/neg/byname-nullables1.check
+++ b/tests/explicit-nulls/neg/byname-nullables1.check.out
@@ -1,4 +1,4 @@
--- Error: tests/explicit-nulls/neg/byname-nullables1.scala:10:6 --------------------------------------------------------
+-- Error: tests/explicit-nulls/neg/byname-nullables1.scala:10:12 -------------------------------------------------------
 10 |    f(x.fld != null) // error
    |      ^^^^^^^^^^^^^
    |      This argument was typed using flow assumptions about mutable variables

showing point (column of the error pos) is the op of the infix op, not start of left operand.

Copy link
Contributor

@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

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

I'll look at the other pos diffs from my suggestion, to check if they also make sense.

I agree with the logic of the original change here, but can't explain why "composition of positions" is so hard. I understand my suggested change without thinking much.

@@ -1665,6 +1665,7 @@ object desugar {
Apply(sel, arg :: Nil).setApplyKind(ApplyKind.InfixTuple)
case _ =>
Apply(sel, arg :: Nil)
apply.withSpan(apply.span.union(arg.span))
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried

apply.withSpan(Span(left.span.start, right.span.end, point = op.span.start))

at the end of binop. That corrects some points in errors, and corresponds to "preserve the span". I'm not sure where point comes from in this expression. (Trying to make position calculations "read" correctly in Scala 2 was also a challenge.) Here union preserves point of the receiver, which is correct; I'll post the diffs.

Copy link
Contributor

@som-snytt som-snytt Mar 6, 2025

Choose a reason for hiding this comment

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

deceptively, Span(start, end) is not the same as Span(start, end, point = start) as I assumed for some reason. I have drilled down into synthetic spans for the "check unused" lint, but I really don't want to hold that arcana in my head. Positions should "just work", if we can figure out the best syntax.

(The doc for def Span does say which flavor is synthetic, but frankly, "pointless" does not say "synthetic" to me, unless I've been recently imbibing the Kool-Aid.)

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