Range selection for scala 3

[Quafadas]

I had a look at the good first issues list, and settled on this,

#5894

I believe the 15 lines of code I have thus far, are a failing test. Reading the issue, it is suggested that

We can match on DefDef while going through tree path and in that case add an additional SelectionRange for all the parameters

Would anyone be able to offer me a hint on this. I can see that in the example below, the Tree is actually empty - it's essentially the entire line. It's not clear to me, how I could decompose it further?

[kasiaMarek]

In Scala 2 val _ = locateUntyped(pos) will set lastVisitedParentTrees to the correct thing. Just wrap def in an object. Also for Scala 3, it seems we already have a fix that just needs to be ported from compiler repo: scala/scala3#19777.

[Quafadas]

Ah - I think I see. I asked the wrong question. I think you're telling me that the Tree I was getting back had no information, because the Compilation Unit was not complete, and so it didn't compile, and so was empty... I'll have another go with a complete version.

Whether or not is should need to compile for this to work? Let's set that to one side as I guess it opens a bit can of horrible that I would doubt I can deal with.

Thankyou for your response 🙏 . I'll have another go with this in mind.

Finally: In the scala3 case should I be attempting to "sniff" if we're using scala3 and delegate to the presentation compiler directly, or do you in fact mean the much blunter "copy that code into metals" strategy?

[kasiaMarek]

Whether or not is should need to compile for this to work?

It doesn't need to compile. Interactive driver can deal with some errors/missing code but apparently not with that.

"copy that code into metals" strategy

Yes, this one. So presentation compiler for Scala 3 is being moved from metals to the compiler, so currently for newer Scala 3 versions e.g. 3.4.1 we use pc from the compiler but for older versions e.g. 3.3.3 we still need to add the bug fixes in metals. The pc code duplication is a temporary situation.

[Quafadas]

Thanks for the responses - which make sense. Given the opportunity to copy and paste someone else's code to make a glorious first contribution, I figured best to start with that, and scala 3.

I've adjusted the tests, to your excellent suggestions above (thanks)...

and I've manage to write a successful test, for scala 3 braceless syntax (yey!).

to my dismay (!), the "braced" version of scala 3 failed though. I've stared at it quite a lot, and I honestly think the test I've written, is the behaviour I'd expect.

Here the region encloses <<region>> App { at the start, but <<region>> } at the end - i.e. not the trailing brace. Firstly, would you agree with that? I'm basically claiming that this test,
https://github.com/scala/scala3/blob/27a3f80cefb774193b555568269f09cd25eb85f6/presentation-compiler/test/dotty/tools/pc/tests/SelectionRangeSuite.scala#L123

Should contain one or two extra entries in the list, which I think the current implementation would fail. Given the relative "value added" of getting that last brace... I'm open to simply truncating my own test, to match the compiler repo. It feels naught though - I'd accept advice.

[kasiaMarek]

Firstly, would you agree with that?

Looking at the other tests, that seems to have been the case for Scala 3 all along. Though, it does seem more sensible to include the brace IMO. However, even if so, it can be treated as a separate issue and you can just truncate the test for this PR.

[Quafadas]

@kasiaMarek

So, my "contribution" thus far is essentially copying and pasting the code you've linked into metals and adding the tests in scala-3 compatibility mode.

I would note that I did, in good faith spend some hours looking at the scala-2 version - I struggled. It looks "hard" to me without quite a clear semantic understanding of the scala-2 Tree, which I don't have. Having committed to scala-3, it's hard to be motivated to obtain it either, sadly.

Said differently, I don't think I can make this better, without some heavyweight help, which I'm not sure would make sense for either of us.

So I think I would choose to tie this off here. A little embarrassing perhaps, but I don't think this "makes things worse", and it should be mergeable as is, given that it's someone else code!

Thanks for the time you've offered :-), it's much appreciated

[kasiaMarek]

Sure, if you don't want to continue working on this that's fine. Thanks for giving it a try and porting things from dotty. I see some tests are failing on CI, theubuntu-latest jdk-11 unit are flaky but Scala cross tests seems relevant. Would you still want to take a look at this?

[Quafadas]

but Scala cross tests seems relevant. Would you still want to take a look at this?

Yes :-) ... I'll look in the coming days and see if I can figure it out.

[Quafadas]

Well, I had a first look at this bonus problem, and left rather confused! It seems, that the struggle lies in the publishLocal command, for scala 3.1.3.

++3.3.3 publishLocal works, as expected.

++3.1.3 publishLocal does not, which is perhaps a hint on why the cross tests failed. In CI, it appears related to the selection range provider, which makes sense. Locally, I get a completely different set of errors.

[error] -- [E008] Not Found Error: /Users/simon/Code/metals/mtags-shared/src/main/scala/scala/meta/internal/pc/ScalaHover.scala:37:32 
[error] 37 |      contentType = ContentType.MARKDOWN
[error]    |                    ^^^^^^^^^^^^^^^^^^^^
[error]    |      value MARKDOWN is not a member of object scala.meta.pc.ContentType

Confusingly VSCode does resolve this symbol, and from what I can tell of the build, mtags-shared depends on tags interface, leaving unclear on where the error above comes from. I strongly suspect, that I am diagnosing the wrong problem here :-(. However, without being able to reproduce locally, I'm scratching my head a little about what to do next.

[Quafadas]

I'm still occasionally merging the main branch into this in the hope of getting CI green again...

[tgodzik]

I'm still occasionally merging the main branch into this in the hope of getting CI green again...

Mill might break, but that should not be related. I have a fix ready.

[Quafadas]

🥳 successful code copying 🚀 !

[Quafadas]

@tgodzik thanks for the feedback :-) - I've removed the comment, created a new issue, tagged the tests scala 3 and updated this against main.

[tgodzik]

Some minor stuff from I noticed, sorry for not mentioning that yesterday!

[tgodzik]

LGTM!

[Quafadas]

@kasiaMarek @tgodzik Thankyou very much for your collective support. I hope that this was ultimately helpful.