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

Fix REPL clashing with CWD artefacts #14021

Merged
merged 1 commit into from Dec 13, 2021
Merged

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Dec 1, 2021

This reverts the change that moves all the REPL's artefacts into the
"repl$" package (#12607), moving everything back to the empty package.

Then it fixes the class name and package name shadowing issue, by
simulating what the batch compiler does when it compiles a source file
without a package statement: it considers the sources to be in the empty
package. Then the imports for the previous runs and the user-defined
imports will be in sub-contexts of the empty package.

The reason the working directory artefacts were shadowing the REPL
artefacts was because before the imports were being added to
sub-contexts of the root package, afterwhich the REPL tree was being
wrapped in the empty package. That meant that the classes found in the
empty package (i.e. classfiles in the working directory, or even
directories) had precedence over the wildcard imports for the previous
runs.

The tests now cascade up into contexts that don't have the current
compilation unit stored on them, so I had to safeguard against that in
Typer.

Also, I developed a variant of ShadowingTests, using the batch compiler,
to automate the expectations of how the shadowing should and shouldn't
work.

@dwijnand dwijnand force-pushed the repl-shadows-empty branch 2 times, most recently from d85f06a to a2b6e6a Compare December 2, 2021 08:28
This reverts the change that moves all the REPL's artefacts into the
"repl$" package, moving everything back to the empty package.

Then it fixes the class name and package name shadowing issue, by
simulating what the batch compiler does when it compiles a source file
without a package statement: it considers the sources to be in the empty
package.  Then the imports for the previous runs and the user-defined
imports will be in sub-contexts of the empty package.

The reason the working directory artefacts were shadowing the REPL
artefacts was because before the imports were being added to
sub-contexts of the root package, afterwhich the REPL tree was being
wrapped in the empty package.  That meant that the classes found in the
empty package (i.e. classfiles in the working directory, or even
directories) had precedence over the wildcard imports for the previous
runs.

The tests now cascade up into contexts that don't have the current
compilation unit stored on them, so I had to safeguard against that in
Typer.

Also, I developed a variant of ShadowingTests, using the batch compiler,
to automate the expectations of how the shadowing should and shouldn't
work.
@SethTisue
Copy link
Member

SethTisue commented Dec 2, 2021

Fixes #12571.

Note that having the REPL use a special REPL package isn't necessarily a bad change, but it seemed cleanest to restore the status quo, since there's no longer a specific motivation to keep the change.

@som-snytt
Copy link
Contributor

We also have to resolve artefacts clashing with artifacts.

This was an interesting discussion which I hope to understand.

If artefacts are namespaced by line objects in the empty package, then it's similar to a Scala 2 idea to make the wrapper objects the package objects for the line. I guess the current rs$line$2 is just a module and doesn't leverage top-level definitions as I had assumed somehow. If REPL supports packagings, it will be interesting that packaged code won't see history, so it will be a private workspace; code in the empty package would see history, as normal.

@dwijnand dwijnand assigned odersky and unassigned griggt Dec 12, 2021
@dwijnand dwijnand removed the request for review from griggt December 12, 2021 11:30
@griggt
Copy link
Collaborator

griggt commented Dec 13, 2021

I agree that there's no need to keep the repl$ package if it's not solving any particular problem.

@griggt griggt merged commit 8e1054e into scala:master Dec 13, 2021
@griggt
Copy link
Collaborator

griggt commented Dec 13, 2021

@dwijnand Nice detective work btw!

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 not sufficiently isolated from class artifacts in current directory
6 participants