Fix #17201: do no call ensureCompleted for import symbol

[dos65]

Fixes #17201

Previously, typedImport was calling retrieveSym which was triggering completion of import symbol and its owner.
In case of having export stm in package object it was leading to cyclic reference error if import symbol was required for export.
This situation was happening only in case if import was the first one.

Examples:

• Error case

  // cycle/Cycle.scala
  package cycle:
    import scala.concurrent.Future // <- first import triggers package completion + export comp
                                   //     which needs `Future` to complete
                                  // - error: cyclic reference
  
    object Cycle:
      given heyInt: Future[Int] = ???
  
  // cycle/package.scala
  package object cycle:
    export Cycle.heyInt

• non - error case

  // cycle/Cycle.scala
  package cycle:
    import scala.concurrent.duration // <- first import triggers package completion + export comp
    import scala.concurrent.Future   // everything is fine at the moment when `export Cycle.heyInt` completes
                                                           // no-cycle because `Future` wasn't touched
  
    object Cycle:
      given heyInt: Future[Int] = ???
  
  // cycle/package.scala
  package object cycle:
    export Cycle.heyInt

I have doubts that this fix is correct but it's anyway seems a bit weird that first import triggers package completion.

[smarter]

I have doubts that this fix is correct but it's anyway seems a bit weird that first import triggers package completion.

In fact, even with your change, package completion is still triggered by typedImportQualifier on the next line, just in a different order.
Previously, the completion order we got was:

completing <import> ->
  typing scala.concurrent ->
     completing cycle.package$ ->
       completing export Cycle.heyInt ->
         completing <import>

But with your change we instead get:

typing scala.concurrent ->
   completing cycle.package$ ->
     completing export Cycle.heyInt ->
       completing <import> ->
         typing scala.concurrent ->
         !! Here,  even though cycle.package$ isn't done being completed (because
            we're still generating its exports) we don't run into a cycle because
            it's info has already been set

This can be observed with the following code:

package cycle:
  object Cycle:
    given heyInt: Future[Int] = ???

package object cycle:
  export Cycle.heyInt
  export scala.concurrent.Future

-- [E006] Not Found Error: try/impq.scala:3:18 ---------------------------------
3 |    given heyInt: Future[Int] = ???
  |                  ^^^^^^
  |                  Not found: type Future
  |

If we move the Future export before the heyInt export, then the code compiles.

I think the behavior introduced by your change is fine: existing cyclic errors either start to compile or become "not found" error, which are less scary than cyclic errors even if they can be puzzling. In fact, your change also fixes the example from #17076 (comment) where the error message was very scary ("Something's wrong: missing original symbol for type tree"), so it looks like a net positive.

EDIT: Actually it doesn't fix #17076, nevermind.

/cc @odersky for a second opinion since this is fairly critical stuff.

[odersky]

This could typecheck an expression in a context which has NoSymbol as owner, which will cause havoc if imp has any local symbols.

I think we need to dig deeper. Either accept the cyclic error but avoid <import> in the error message or somehow heal the situation. But I disagree that a NotFound is necessarily better. I think it could be more puzzling than the cyclic error.

Generally, I think trying export for package management is doomed. Even if it worked, I would find it dubious since it would just add another layer of complexity.

[smarter]

Generally, I think trying export for package management is doomed

That's a pity, because it seems that people naturally tend to use that way (as in https://contributors.scala-lang.org/t/why-the-current-export-is-problematic-for-import/6147), and because cyclic errors involving exports might not mention export/import at all, like in #17076, where the problem suddenly started manifesting itself long after the user had started using export, making it extremely hard to debug.

[odersky]

Yes, I think the best thing to do now is warn people that that's an anti-pattern.

[dos65]

Thanks.
I will look at that a bit more.

Having an explanation from @smarter I'm wondering if we can also try to trigger package completion (which includes package export) in advance before visiting first first import. In theory it should change the completion order and at least fix #17201

[odersky]

I believe any change in the order would cause other cycles. This whole thing (trying to avoid cycles) is very, very finely balanced. And it is also what it is, by now. Any change in the order would likely reject existing code, and therefore is infeasible at this point.

One can delay evaluation at some point if one can arrange things that certain info is not needed at this point and can be accessed later. For instance, a recent change of mine moved some checks to PostTyper, since they needed info that caused cycles in Typer. But I don't think that's applicable for exports.

The other room for improvement is in better diagnostics if there is a CyclicReference. I believe there is still a lot to be done, but it is not easy.

[smarter]

This could typecheck an expression in a context which has NoSymbol as owner, which will cause havoc if imp has any local symbols.

I don't think this is a behavior change from what happened before, retrieveSym can also return NoSymbol, but import qualifiers don't define new symbols so I'm not sure that withOwner does anything anyway.

[robmwalsh]

This could typecheck an expression in a context which has NoSymbol as owner, which will cause havoc if imp has any local symbols.

I think we need to dig deeper. Either accept the cyclic error but avoid <import> in the error message or somehow heal the situation. But I disagree that a NotFound is necessarily better. I think it could be more puzzling than the cyclic error.

Generally, I think trying export for package management is doomed. Even if it worked, I would find it dubious since it would just add another layer of complexity.

I've just run into an error likely caused by this -
What would you consider the use of export other than package management?

[smarter]

What would you consider the use of export other than package management?

According to https://docs.scala-lang.org/scala3/reference/other-new-features/export.html#motivation, the point of export is to make it easier to choose composition over inheritance. See also historical discussions https://contributors.scala-lang.org/t/request-for-comments-on-exports/4051 and https://contributors.scala-lang.org/t/having-another-go-at-exports-pre-sip/2982

[odersky]

I have doubts that this fix is correct but it's anyway seems a bit weird that first import triggers package completion.

That's not so weird. Processing an import means we have to find what's in the referenced structure. Or else we could miss something that's imported.

[smarter]

Generally, I think trying export for package management is doomed. Even if it worked, I would find it dubious since it would just add another layer of complexity.
[...]
Yes, I think the best thing to do now is warn people that that's an anti-pattern.

This is going to be tough, just from a cursory search it seems like an already common pattern:

• https://github.com/VirtusLab/iskra/blob/39792d2b69f89e0e690e2be758bcbc7de6ed7387/src/main/api/api.scala
• https://github.com/getkyo/kyo/blob/3a2d6e33139106aa5366775adc5df9236c48494e/kyo-core/shared/src/main/scala/kyo/all.scala
• https://github.com/aappddeevv/scalajs-reaction/blob/19c9ae146456dc212a62b039d99b8a285b22979f/jshelpers/src/main/scala/syntax.scala
• https://github.com/CiaraOBrien/dottytags/blob/4a7917641bb2af1ef90f5adcbfac505db52d3bb9/src/main/scala/dottytags/predefs/All.scala
• https://github.com/erikerlandson/coulomb/blob/cc1f38a4586f0f843cc239053fa2ea82820761e4/spire/src/main/scala/coulomb/ops/algebra/spire/all.scala

I'm also not sure what counts as "export for package management". Is it any situation where we use export for convenience to avoid having to write longer qualifiers or manually import things? Because in dotty itself we already use export for convenience to more easily access enum cases or opaque types defined in objects.

[robmwalsh]

from discussion on discord

Probably too late to enforce, but would exports that are restricted to higher levels of their own path, or otherwise empty paths (i.e an object or package that only contains exports) solve many of the cycling issues? i.e. I think that foo.bar.baz.Baz.x could be safely exported to foo, foo.bar, foo.bar.baz or foo.api (assuming nothing else is in foo.api). perhaps there could be a compiler warning for cases other than these?