Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #17201: do no call ensureCompleted for import symbol #17304

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Fix #17201: do no call ensureCompleted for import symbol #17304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/Typer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2727,7 +2727,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
else typd(imp.expr, AnySelectionProto)

def typedImport(imp: untpd.Import)(using Context): Tree =
val sym = retrieveSym(imp)
val sym = imp.removeAttachment(SymOfTree).getOrElse(NoSymbol)
val expr1 = typedImportQualifier(imp, typedExpr(_, _)(using ctx.withOwner(sym)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

checkLegalImportPath(expr1)
val selectors1 = typedSelectors(imp.selectors)
Expand Down
10 changes: 10 additions & 0 deletions tests/pos/17201/17201.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// cycle/Cycle.scala
package cycle:
import scala.concurrent.Future

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

// cycle/package.scala
package object cycle:
export Cycle.heyInt