Interactive.pathTo contains Literal(Constant(null)) as a head of path for incomplete program

[tanishiking]

area:ide

Compiler version

3.1.3-RC2 and later

Minimized code

See: https://github.com/tanishiking/dotty-interactive-playground/blob/50ea5babab3d6d124b98597873af371765b97bf3/src/main/scala/Main.scala#L45-L64

val sourceDefinition = "object Definition { def x    }"
val uriDefinition = new URI("file:///def")
driver.run(uriDefinition, sourceDefinition) // where driver: InteractiveDriver
given ctx: Context = driver.currentCtx

val pos = new SourcePosition(driver.openedFiles(uriDefinition), Spans.Span(sourceDefinition.indexOf("def x") + "def x".length))
val path = Interactive.pathTo(driver.openedTrees(uriDefinition), pos)
path.foreach(println)

Output

with 3.1.3-RC2 or later

Literal(Constant(null))
DefDef(x,List(),...)
Template(...)
TypeDef(...)

with 3.1.2

DefDef(x,List(),...)
Template(...)
TypeDef(...)

Expectation

Scala 3.1.3 should return the same result as Scala 3.1.2. There shouldn't be a Literal(Constant(null)).

Additional context

This is a bit of a problem for Metals, for example, when Metals try to invoke advanced completion based on the path to the cursor position something like def | (| represents the cursor position).
As a workaround Metals ignore Literas(Constant(null)).
https://github.com/scalameta/metals/blob/92d8d9ecdaead1a6db2a9ebb1a85500c22c4714d/mtags/src/main/scala-3/scala/meta/internal/pc/completions/Completions.scala#L136-L139

[adampauls]

@tanishiking I think this is at least arguably working as intended? I recently made some changes (some of which @odersky largely rewrote) to add the errorTermTree in more places for incomplete parses. The motivation in those PRs (e.g. #14463) was to differentiate between e.g. foo(x,<EOF> and foo(x). Before the PR, these two had the same parse, but the PR changed it so that the former gives foo(x, Literal(Constant(null)).

I think Metals should expect to see the errorTermTree (Literal(Constant(null))) and ignore it? Note that I did not add errorTermTree, I just made it occur in more places. It's possible some of my changes caused this particular change, but I'm not sure.

[adampauls]

As an example, suppose you had

object Definition { def x =   }

It would be nice to know whether the cursor is on the left or the right side of the = (because on the left, you might want to autocomplete a type ascription, but on the right, you might want to propose a term). I think the behavior of the parser now is that you will get errorTermTree representing the missing body after =, so if the cursor is anywhere between = and }, you will know you are in a "missing" tree.

Now that I think about it, there's a pretty good chance that the errorTermTree was always there, but its span was just different, which changed as of #14492.

[tanishiking]

@adampauls Thank you for the input!

t would be nice to know whether the cursor is on the left or the right side of the =

In this usecase, I assume the cursor is on the right side of = (somewhere between = and }).

I think it's ok to have errorTermTree for a "missing" tree, but it might be better to drop from Interactive.pathTo as it's useless 🤔 (Anyway, we don't need to rush, since Metals already ignoring this errorTermTree).

[adampauls]

Well, I was specifically trying to argue it is useful in general :) I agree it's not for your use case and you are doing the right thing by ignoring.

But suppose you have

object Object {
  def foo: Unit = •
}

where • is the cursor. Unless I'm mistaken, without the error term tree, you might as well have the cursor on the def keyword. But you might want to autocomplete () to fill in the missing body if (and only if) you have your cursor on the missing body. It's totally reasonable to have the IDE code in general react to an error term tree by proposing values that would belong in that context -- this is what we did in our internal tooling that uses the compiler and it has been very handy!

So I'd hate to change pathTo since I would argue it's doing the right thing here. Though of course if you prefer one can have the current pathTo chop off the error and add another pathToWithError, or have similarly pathTo and pathToExcludingError.

[smarter]

this is what we did in our internal tooling that uses the compiler and it has been very handy!

Off-topic, but this is intriguing :), is this something you could share more information on?

More on-topic: I think it's worth adding a comment to pathTo mentioning that it might return errorTermTree (but presumably only if ctx.reporter.errorsReported is true), and it seems like we could then close this issue.

[adampauls]

Off-topic, but this is intriguing :), is this something you could share more information on?

The short story is that we had a need for a programming language targeting an internal runtime. One camp wanted to build something from scratch and one camp wanted to do something like ScalaJS/Native where we reuse the Scala compiler as the front end, and we ended up in a middle ground where we reuse some parts of the Scala ecosystem but also built some stuff on our own.

[tanishiking]

So I'd hate to change pathTo since I would argue it's doing the right thing here. Though of course if you prefer one can have the current pathTo chop off the error and add another pathToWithError, or have similarly pathTo and pathToExcludingError.

Ok, it's reasonable to keep the pathTo behavior, as it is doing the right thing and there's a usecase of errorTermTree.

Also, we don't need to something like pathTiExcludingError as it's quite easy to ignore.
As @smarter mentioned we can close the issue with some documentation 👍

[tanishiking]

I'll work on adding comments to Interactive.pathTo as

More on-topic: I think it's worth adding a comment to pathTo mentioning that it might return errorTermTree