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.

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

Avoid infinite recursion when looking for suggestions #22361

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

Conversation

mbovel
Copy link
Member

@mbovel mbovel commented Jan 13, 2025

Keep a a set of seen parent symbols between rootsIn and rootsStrictlyIn to avoid infinitely growing paths that refer to the same value, such as Collection.this.O.O.O.O.O.O.O.... when computing import suggestions for:

trait Collection:
  val base: Collection = ???
  base.foo()

  object O extends Collection:
    def foo(): Int = ???

Fixes #22145.

@mbovel mbovel requested review from odersky and removed request for odersky January 13, 2025 17:42
@mbovel
Copy link
Member Author

mbovel commented Jan 13, 2025

Reverting to draft for now, as we should look for more cases. For example, as noted by @natsukagami, this still fails:

trait Collection:

  def bar(base: Collection) = base.foo
  object a extends Collection:
    def foo: Int = 0
  object b extends Collection:
    def foo: Int = 1

@mbovel mbovel marked this pull request as draft January 13, 2025 18:14
@olhotak
Copy link
Contributor

olhotak commented Jan 13, 2025

Reverting to draft for now, as we should look for more cases. For example, as noted by @natsukagami, this still fails:

trait Collection:

  def bar(base: Collection) = base.foo
  object a extends Collection:
    def foo: Int = 0
  object b extends Collection:
    def foo: Int = 1

I wonder whether this kind of recursion is what the seenNames inside def nestedRoots is intended to catch. Maybe the correct fix is that seenNames needs to be lifted out somewhere to the level of rootsIn/rootsStrictlyIn.

If I recall correctly, we did not figure out the original motivation for seenNames.

@mbovel
Copy link
Member Author

mbovel commented Jan 14, 2025

I continue to wonder if we couldn't make seen a set of Symbols instead of TermRefs.

I initially thought this could be problem when several members have the same type symbol but with different params, for example:

class C[T]:
  extension (x: T) def f(): Int = 1

object O:
  val c0: C[String] = new C[String]
  val c1: C[Int] = new C[Int]
  //import c1.f
  2.f()

But there actually is already no suggestion for this example, even without c0.

So if we only consider objects and packages, as the documentation of suggestionRoots seems to imply, maybe it's okay to never visit the same symbol twice?

Co-authored-by: Martin Odersky <[email protected]>
Co-authored-by: Ondřej Lhoták <[email protected]>
Co-authored-by: Nguyen Pham <[email protected]>
@mbovel
Copy link
Member Author

mbovel commented Jan 14, 2025

Discussed with @odersky, who gave this example where making seen a set of Symbols would miss suggestions:

package foo

class C[T]:
  object Ext:
    extension (x: T) def f(): Int = 1

object O1 extends C[String]
object O2 extends C[Int]

def main =
  2.f()

Martin instead suggested to keep a set of seen parents symbols between rootsIn and rootsStrictlyIn.

@mbovel mbovel marked this pull request as ready for review January 14, 2025 16:19
@mbovel mbovel requested a review from odersky January 14, 2025 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack overflow while compiling a nested given
2 participants