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

Drop scala shadowing and DottyPredef #10428

Merged
merged 13 commits into from Nov 24, 2020

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Nov 21, 2020

Several steps:

1. Make bootstrapped DottyPredef an empty object

Once we have a new bootstrap compiler we can drop it entirely.

Instead of using DottyPredef, patch the Predef class when unpickling it.
The patch adds all methods from a patch class

 scala.runtime.stdLibPatches.Predef

to scala.Predef.

2. Drop scalaShadowing after bootstrap

Instead, patch the scala.language object in the same way we patch scala.Predef.
We leave a dummy file in scalaShadowing, since the build requires the package to exist.
When we have a new bootstrap compiler, we should drop scalaShadowing entirely.

@odersky odersky force-pushed the drop-scalaShadowing branch 3 times, most recently from 37f1203 to 3b46af1 Compare November 21, 2020 13:49
def withFinalizer(finalizer: Finalizer): SymbolLoader =
val cpy = clone().asInstanceOf[SymbolLoader]
cpy.finalizer = { sym =>
this.finalizer(sym)
Copy link
Member

Choose a reason for hiding this comment

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

should there be a local reference to this.finalizer to avoid capturing the whole SymbolLoader if unnecessary?

@odersky odersky changed the title Drop scala shadowing Drop scala shadowing and DottyPredef Nov 22, 2020
@odersky odersky marked this pull request as ready for review November 22, 2020 16:41
@odersky odersky requested a review from sjrd November 22, 2020 17:55
@odersky
Copy link
Contributor Author

odersky commented Nov 22, 2020

@abgruszecki Can you review this with an eye whether some doc generation needs to be adapted?

@@ -1199,6 +1199,7 @@ class Namer { typer: Typer =>
cls.setNoInitsFlags(parentsKind(parents), untpd.bodyKind(rest))
if (cls.isNoInitsClass) cls.primaryConstructor.setFlag(StableRealizable)
processExports(using localCtx)
defn.patchStdLibClass(denot.asClass)
Copy link
Member

Choose a reason for hiding this comment

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

What happens when we pickle Predef.scala to Tasty, then unpickle it? As far as I can tell, since we're only adding symbols and not trees, the pickled Predef won't have the extra definitions, so we need to run patchStdlibClass in TreeUnpickler too.

Copy link
Contributor Author

@odersky odersky Nov 23, 2020

Choose a reason for hiding this comment

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

But do we ever unpickle Predef from Tasty? Why would we want to do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

The doctool should probably see the unpickled Predef, for definition lookup purposes. I'd also worry about any user of Tasty Inspector that wants to look up definitions in Predef.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But so far nobody pickles Predef into Tasty, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked and it seems a Predef.tasty file is created, we also document it in the doctool already:
https://scala3doc.virtuslab.com/pr-master/scala3-stdlib/api/scala/-predef/index.html?query=%20object%20Predef%20extends%20LowPriorityImplicits

Copy link
Contributor

@abgruszecki abgruszecki left a comment

Choose a reason for hiding this comment

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

The PR looks good to me, aside from the remark that Guillaume already had. If we can ensure in this PR that definitions loaded from Tasty will have the same patches applied, I'd say we should do that. Otherwise, I can try doing that myself when doing more work on definition lookup in the doctool.

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

OK, makes sense :)

@odersky
Copy link
Contributor Author

odersky commented Nov 24, 2020

I will merge this as soon as the CI is green. I have been spending about 50% of my time yesterday and today to re-apply the same patc with thishes endlessly to the CB only to see them being rendered outdated by other changes.

It's been replaced a while ago by `-source 3.0-migration`
Once we have a new bootstrap compiler we can drop it entirely.

Instead of using DottyPredef, patch the Predef class when unpickling it.
The patch adds all methods from a patch class

     scala.runtime.stdLibPatches.Predef

to scala.Predef.
Instead, patch the `scala.language` object in the same way we patch `scala.Predef`.
We leave a dummy file in scalaShadowing, since the build requires the package to exist.
When we have a new bootstrap compiler, we should drop `scalaShadowing` entirely.
Patch its members recursively instead.
@odersky odersky merged commit 7bdf89e into scala:master Nov 24, 2020
@odersky odersky deleted the drop-scalaShadowing branch November 24, 2020 12:38
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
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.

Discussion: what should we do with DottyPredef before the release?
6 participants