Add Stable Presentation Compiler

[rochala]

This PR adds a stable presentation compiler implementation to dotty. This will ensure that each future version of Scala 3 is shipped with presentation compiler compiled against it, guaranteeing that IDE will support it. It will also improve support for projects relying on nonbootstrapped compiler such as scaladoc or dotty-language-server, as it is now possible to easily publish presentation compiler for those versions from dotty repository.

It also adds vast of tests suits ported from metals, which will also help to detect unintended changes before they are merged. More information about this initiative can be found here: https://contributors.scala-lang.org/t/stable-presentation-compiler-api/6139

I recommend doing review commit by commit. I cleaned them up to clearly separate what was straight copied from mtags, and what changes I had to make. I added comments in commits which also should help in the review process.

[test_java8] [test_non_bootstrapped] [test_windows_full]

[tgodzik]

It should be ok to remove scala3-language-server now, no?

[rochala]

I am almost sure that the coverage of scala3-language-server is not a subset of mtags tests, which may result in lost test cases. We would either have to verify if each test has its substitute in presentation-compiler suite and migrate missing cases.

It should be done, but may require additional time to verify all cases.

[tgodzik]

Sure, but would be good to have an issue or a plan about it. This might be done even by someone less proficient in the compiler.

[tgodzik]

A lot of great work! I don't see anything else aside from those few comments since most of the code is just moved plus test utilities, which are fine to duplicate to avoid additional burden (we don't really modify them much)

One thing we should add after everything is testing in Metals on the most recent nightly (instead of the locally compiled mtag one) to make sure we are consistent between versions.

We will also need to keep the mtags version until we stop supporting compiler versions lower than when this is released.

[smarter]

Would you mind adding an entry to https://github.com/lampepfl/dotty/blob/main/NOTICE.md listing which directories contain code adapted from metals?

[lrytz]

IIUC, this will bring IDE feature implementations into the compiler repository. These implementations directly use the compiler-internal data structures (Trees / Types / Symbols). Separating them from the compiler internals would be a big undertaking, but was it considered do that by createing a "minimal" presentation compiler interface? Or could tasty query be useful for that?

Besides the advantages, there are some drawbacks in having IDE implementations in the compiler repo. IDE changes will be tied to compiler releases, which can complicate the workflow and slow things down. Improvements and fixes cannot be done for existing Scala versions. Maybe it's worth considering a separate repository with some automation to publish releases against Scala 3 nightlies?

[sjrd]

tasty-query requires a fully typechecked classpath. Most IDE features need to work with partially incorrect projects.

[smarter]

was it considered do that by createing a "minimal" presentation compiler interface?

Well, this is supposed to be the minimal interface, presumably if the interface could be simpler with more code shared between scala 2 and scala 3, then metals would have already gone with that design. Maybe there's specific things we can reconsider now but I'm not familiar enough with the requirements of metals to say.

[lrytz]

I guess there could be an abstraction over the compiler data types to implement the IDE functionalities in this PR externally, I was curious if that was ever considered. But again, it would be a lot of work and it might well not be worth it.

[tgodzik]

I guess there could be an abstraction over the compiler data types to implement the IDE functionalities in this PR externally, I was curious if that was ever considered. But again, it would be a lot of work and it might well not be worth it.

This would be the ideal scenario, but it would be really hard to design the API. We use loads of methods from symbols, so this would be a very large API and to do it in Java would make it even harder to use.

The current approach is the least amount of effort that we can spend, since the API is ready and we needed to just port it. Turns out it was a bit more involved than that, but still should be our best shot for a stable Java compiler API in a reasonable amount of time.

This has some drawbacks, since will need to fix things in the compiler, but the fixes should really be quick since the plan for Scala 3 is to have a release every 6 weeks and should be easier for patch releases than it was for 3.3.0.

An added bonus is that other tool authors will be able to use the API and any fixes will work for all of them. The LSP support was initially in Dotty, we ported it to Metals as it was easier to iterate over it and now since it's stable again it will be back in dotty again. So this is kind of a full circle.

[smarter]

Why is there sometimes a ":" and sometimes not?

[rochala]

It depends on whether we use a normal standard library or a bootstrapped one.
A default scala standard library, returns different canditate for completion.
In case of standard library, it returns an ExprType of Nil. Scala stdlib-bootstrapped on the other hands, returns a TermRef for value Nil, thus has different label is created.

The deletion of ":" in those tests is caused, because I moved back from stdlib-bootstrapped to standard one as I no longer need it for tests.

[smarter]

In case of standard library, it returns an ExprType of Nil. Scala stdlib-bootstrapped on the other hands, returns a TermRef for value Nil, thus has different label is created.

Does that impact what is shown to the user or is this an implementation detail? It seems like the tests would be more stable and easier to read if the ':' was always present.

[rochala]

Yes, this is the label for the completion, so it impacts what is shown to the users.
This is strictly connected to how different types are printed.

[smarter]

Sounds like an issue with the printer then?

[rochala]

To be honest, I'm not sure if this is an issue with the printer or rather the fact that Scala 2 library unpickled symbols are different from symbols that come from Scala 2 library compiled with Scala 3 aka stdlib-bootstrapped.

[smarter]

I don't know why the symbol types are different (maybe because objects are represented slightly differently in Scala 2 and 3) but in general I don't think that whether the type of something is an ExprType or a TermRef should affect what the pretty-printer does.

[rochala]

I see, so this is a bug in printer indeed. As it was present in metals before, I propose that we create an issue and handle this in the next PR as this one is already enormous.

[smarter]

It looks like this file is also working around and duplicating things from Dotty and should be folded into the regular RefinedPrinter/PlainPrinter (it looks like ForInferredType below uses metals-specific stuff and could stay separate), but this can be done in a separate PR if you prefer.

[smarter]

The fact that the PC defines its own subclass of Type is very weird, what is this used for?

[tgodzik]

It's used for showing a shorter type without the prefix if that prefix is imported into scope. We will show more of the prefix if that is not imported. We could also figure out a way to fold that into the normal printer. We could have something like ShortPrinter

[smarter]

Yeah ideally I'd love for the normal printer to also use short names by default for nicer error messages. Also I believe the shortType method below is incomplete:

        case mt @ MethodTpe(pnames, ptypes, restpe) if mt.isImplicitMethod =>
          ImplicitMethodType(
            pnames,
            ptypes.map(loop(_, None)),
            loop(restpe, None)
          )

isImplicitMethodType is true for both (implicit ...) blocks created with ImplicitMethodType and (using ...) blocks created with ContextualMethodType, so it's likely that the result here will be pretty-printed incorrectly. I think this is a good illustration that the current solution is not maintainable and we need to find a better way to handle short names across the compiler.

[smarter]

As a first step, we could try to figure out what needs to change in Dotty to let the PC implement name shortening without hacks. PlainPrinter already defines isOmittablePrefix which could be overridden by the PC, but it needs to be generalized to also take the name of the thing we want to omit the prefix of as argument to support named imports. Once we get the PC to use this and remove PrettyType we can see what tests fail and proceed from there.

[smarter]

Suggested change

	.filter(p => p.name.toString.startsWith(EvidenceParamName.separator))
	.filter(p => p.name.is(EvidenceParamName))

[rochala]

It seems that this check is not working. Also, the solution in https://github.com/lampepfl/dotty/blob/5c66d7b49421e0b7d399bcb506361b13c73db2ce/compiler/src/dotty/tools/dotc/util/Signatures.scala#L409-L411 also handles it that way, so I'm leaving it for now.

[smarter]

There's similar logic in https://github.com/lampepfl/dotty/blob/5c66d7b49421e0b7d399bcb506361b13c73db2ce/compiler/src/dotty/tools/dotc/util/Signatures.scala#L379, this can be done later but we should try to see if we can reuse Signatures here too.

[tgodzik]

Congrats @rochala ! 🎉

[ckipp01]

This is fantastic. Great work @rochala 🚀

[Kordyjan]

Note to the future: when we are backporting this to 3.3.x, remember to check if there are no removed APIs.