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

repl - new heuristic for import shadowing #14951

Closed
wants to merge 2 commits into from

Conversation

rochala
Copy link
Contributor

@rochala rochala commented Apr 15, 2022

Fixes #5074

Link to previous fix discussions: #4915

The main point of this pull request is changing how we handle imports in the following repl evaluations.
On the contrary to the compiler, repl should allow definition shadowing by imports and vice-versa.

The previous heuristic was handling only one-way shadowing which was the result of import handling in the following way:

  1. We import previous run wrapper eg. import rs$line${i <- 0 until nextId}._
  2. We import previous top-level imports

It could handle shadowing, when import shadowed the definition:

scala> object A { def f = 1 }
scala> def f = 2; import A._
scala> f
res0: Int = 2

However, this couldn't work the way around as top-level imports are imported after wrapper:

scala> import A._; def f = 2
scala> f
res0: Int = 1

The new proposed heuristic works in the following way:

  1. Collect all top-level definitions ( top-level in context of repl wrapper object )
  2. Find all top-level imports
  3. For each top-level import find its members and all selectors with rename. We now filter wildcard members from the selector, and then we sum names after renaming with remaining members of the imported type.
  4. Filter import definitions by removing those defined before the import clause.
    We only need to check forward references as import handling in the following object stays the same (the first wrapper, then import ) hence all definitions which were defined before the import will be shadowed anyway.
  5. Find the intersection between definitions after import and member names from the Import clause ( including renames ).
  6. Leave existing trees as they are, but transform imports before collecting them for next run imports -
    for each intersection member we transform the import by prepending its exclusion ( ImportSelector(Ident(name), Ident(Wildcard)) to existing selectors.
  7. the Last step is filtering duplications to eliminate possible double imports of the same Name and eliminating imports in which all members have been shadowed.

@som-snytt
Copy link
Contributor

Previously, the challenge was not in selecting named things, but detecting whether an import is needed for implicit resolution.

@rochala
Copy link
Contributor Author

rochala commented Apr 22, 2022

@som-snytt could you elaborate why implicit resolution is a problem here ?

@som-snytt
Copy link
Contributor

I still haven't had a chance to read this PR, which I look forward to, but to explain my comment:

If I import util.chaining.*, how does it know when to include that import in a subsequent line-compilation? The previous answer was always import, with a nesting level that reflects REPL line order so that Scala 2-style implicit shadowing works. Maybe in Scala 3, it can always import, or always use given imports.

Apologies in advance if that issue has nothing to do with this PR. It came up in every discussion, such as Ammonite, on how REPL imports should work.

@SethTisue
Copy link
Member

SethTisue commented Dec 1, 2022

The solution here is complicated enough that it makes me wonder if it's worth it. (Worth the sum of all the future maintenance cost.)

Dale suggested maybe just forbidding mixing import with other things in a single REPL entry.

It also occurred to both of us that another solution path could be to notice when import is present and automatically split the entry up into multiple entries.

@dwijnand
Copy link
Member

dwijnand commented Dec 1, 2022

It also occurred to both of us that another solution path could be to notice when import is present and automatically split the entry up into multiple entries.

This would fix your "import shadowing should only work on toplevel definitions" cases too, I believe.

@ckipp01
Copy link
Member

ckipp01 commented May 11, 2023

Is this something that is still waiting on another review or something that can be pushed forward?

@rochala
Copy link
Contributor Author

rochala commented Apr 12, 2024

I'm closing this as both Dale and Seths idea seems to be a better way to solve this issue.

@rochala rochala closed this Apr 12, 2024
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.

REPL: Import shadowing issue
5 participants