-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Drop scala shadowing and DottyPredef #10428
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
Conversation
37f1203
to
3b46af1
Compare
def withFinalizer(finalizer: Finalizer): SymbolLoader = | ||
val cpy = clone().asInstanceOf[SymbolLoader] | ||
cpy.finalizer = { sym => | ||
this.finalizer(sym) |
There was a problem hiding this comment.
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?
1cab33b
to
623ddd2
Compare
@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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, makes sense :)
7c35990
to
60b5ac6
Compare
e26fd1e
to
77ecfea
Compare
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.
With so much churn it's almost impossible to keep up.
18b375f
to
c351094
Compare
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
to scala.Predef.
2. Drop scalaShadowing after bootstrap
Instead, patch the
scala.language
object in the same way we patchscala.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.