Fix Scaladoc crash when extending non-Scala-3 classes (long-term fix for 3.4.0)

[jan-pieter]

Extend the PositionMethods with an exists method, since the other methods are not safe to call on non-existing Spans.

Not eligible for backport to 3.3 because of API addition. See #16759 for a workaround that uses the current API.

Fixes #15927

[jan-pieter]

As expected, MiMa fails for this change.

[SethTisue]

(marking as draft for now since it's #16759 that should be merged first)

[SethTisue]

As expected, MiMa fails for this change

I believe we need to do something about that, but I'm not clear on what the right thing to do is.

If we believe the change is safe, then we can mollify CI by adding it an exclusion to project/MiMaFilters.scala.

But I think the change isn't safe, because adding a method to a trait isn't backwards binary compatible unless you include a default implementation. And it matters because Quotes is part of the standard library, it's not compiler internals where we're free to break stuff.

Can we just def exists: Boolean = false (or = true? 🤔), so a default implementation is there to satisfy MiMa, and be done with it?

In practice, I don't think there is any realistic scenario in which the default method — or the abstract method, if we leave it abstract — would end up being called. The only call to exists that exists is the one we ourselves are adding to ClassLikeSupport.scala. In order for that call to end up being routed through the abstract or default at runtime, someone would have to be using ClassLikeSupport directly rather than in the context of the Scaladoc tool, but ClassLikeSupport is itself in an impl package that is considered internal.

In short, I think it would be safe to simply add a MiMa exclusion. I also think it would be safe to have def exists: Boolean = false (and this is probably marginally preferable to defaulting to true, since false doesn't promise anything to the caller) — but there is perhaps a slight danger of providing a default implementation, which is that it makes it possible to forget to override it, perhaps in future refactoring of the codebase.

In short, I suggest we add the MiMa exclusion and be done with it.

@dwijnand Do you agree with this analysis? I've written the above without hedging, as if I were 100% sure, but I'm not really that sure my reasoning is correct and complete.

[dwijnand]

But I think the change isn't safe, because adding a method to a trait isn't backwards binary compatible unless you include a default implementation. And it matters because Quotes is part of the standard library, it's not compiler internals where we're free to break stuff.

QuotesImpl is the only implementation of the interface, so we don't need to worry about defaulting it.

Also, we don't need to worry about forwards compatibility across minor versions because we are free to break it: code that was compiled against 3.4.0 (and used Position#exists) cannot be consumed by code being compiled with 3.3.0 because of tasty versions.

[julienrf]

Also, we don't need to worry about forwards compatibility across minor versions because we are free to break it

Adding an abstract method to a trait breaks the compatibility in the backwards direction: existing code like new TypeThatNowHasAnAbstractMethod() will fail at run-time because the new version of that class can’t be instantiated.

But I agree with your point that we should probably not worry too much about the Quotes API because it is unlikely that there are other implementations of it… We could still add a point about that in the release notes, though.

[dwijnand]

Adding an abstract method to a trait breaks the compatibility in the backwards direction: existing code like new TypeThatNowHasAnAbstractMethod() will fail at run-time because the new version of that class can’t be instantiated.

You're right.

But I agree with your point that we should probably not worry too much about the Quotes API because it is unlikely that there are other implementations of it… We could still add a point about that in the release notes, though.

Don't even think it's worth release noting. Perhaps it's worth a sentence in the Quotes doc, as in "not intended to be implemented inherited/implemented". It also has a self type with traits that are in scala.quotes.runtime which, if we use scala.runtime as well as their docs, implies it's not public for extending/implementing, making it impossible to legally implement Quotes.

[Sporarum]

We had a similar issue with position when working on #14810,

Since this is the long term solution, I encourage you to also refactor
https://github.com/lampepfl/dotty/blob/main/scaladoc/src/dotty/tools/scaladoc/tasty/SymOps.scala#L157-L173
And other snippets going around the previous limitations
(having the wrong way to do something in the code might make people simply copy it without looking for the right way)

This might also be the place to solve #16619, for example by adding an assert to exists ?

[Sporarum]

Could these be an extension export ?

[nicolasstucki]

We should not add Position.exists to the reflection API. All trees should have a position. If a tree has a nonexistent position, it is a bug that needs to be fixed at the source.

[dwijnand]

What's the issue with accepting that some positions don't exist? I'm all with you on your ideal, but the reality seems like trees with non-existing positions crop up and either there's some support for identifying them or people are stuck doing hack arounds or making their users suffer crashes.

Seems like an uphill battle and not a hill to die on - to me.

[nicolasstucki]

The missing position bug was already fixed.

We follow up with #19250, where we make the API resilient to missing positions. There is no need to have exists method.