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 missing changesParents in PostTyper #20062

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

liufengyun
Copy link
Contributor

@liufengyun liufengyun commented Apr 1, 2024

The following two problems lead to crash of -Ysafe-init-global while compiling scala2-library-bootstrapped:

  • Fix missing changesParents in PostTyper
  • Fix mismatch of parents tree and parents in symbol info

[test_scala2_library_tasty]

@liufengyun liufengyun marked this pull request as ready for review April 1, 2024 21:14
@liufengyun
Copy link
Contributor Author

I cannot assign reviewers. Could you have a look at this PR @nicolasstucki ?

/cc: @olhotak

@nicolasstucki
Copy link
Contributor

I will have a look.

@hamzaremmal could you add @liufengyun to the new Scala 3 contributors group?

@hamzaremmal
Copy link
Member

Done. @liufengyun you should be able to ask for reviewers and merge PRs now.

@nicolasstucki
Copy link
Contributor

@liufengyun could you push this PR again to trigger the tests on [test_scala2_library_tasty]. Rebase and push usually works.

Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

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

It looks functionally good to me. I am not sure if we have a better way to fix this. I will reasing to @odersky to make sure we do not break or degrade preforence for the compiler when not compiling the Scala 2 library.

@@ -36,12 +36,16 @@ class Checker extends Phase:
traverser.traverse(unit.tpdTree)

override def runOn(units: List[CompilationUnit])(using Context): List[CompilationUnit] =
val checkCtx = ctx.fresh.setPhase(this.start)
val checkCtx = ctx.fresh.setPhase(this)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure this could have unintended consecuences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously we did something wrong --- otherwise, the tree and symbol info will not be in sync.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

/**
* Serializable and AbstractFunction are added for scala2-library companion object of case class
*
* Ideally `compilingScala2StdLib` should be used, but it is initialized too late to be effective.
Copy link
Contributor

Choose a reason for hiding this comment

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

We could try to move these three statements in Run.compileUnits earlier in front of ctx.base.usePhases(phases):

val runCtx = ctx.fresh
runCtx.setProfiler(Profiler())
unfusedPhases.foreach(_.initContext(runCtx))

That should allow us to use compilingScala2StdLib.

@odersky odersky assigned liufengyun and unassigned odersky Apr 2, 2024
val pluginPlan = ctx.base.addPluginPhases(ctx.base.phasePlan)
val phases = ctx.base.fusePhases(pluginPlan,
ctx.settings.Yskip.value, ctx.settings.YstopBefore.value, stopAfter, ctx.settings.Ycheck.value)
ctx.base.usePhases(phases)
ctx.base.usePhases(phases, runCtx)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about this change? @odersky

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that looks good to me.

@liufengyun liufengyun requested a review from odersky April 3, 2024 08:43
@liufengyun liufengyun assigned odersky and unassigned liufengyun Apr 3, 2024
val pluginPlan = ctx.base.addPluginPhases(ctx.base.phasePlan)
val phases = ctx.base.fusePhases(pluginPlan,
ctx.settings.Yskip.value, ctx.settings.YstopBefore.value, stopAfter, ctx.settings.Ycheck.value)
ctx.base.usePhases(phases)
ctx.base.usePhases(phases, runCtx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that looks good to me.

@odersky odersky assigned liufengyun and unassigned odersky Apr 7, 2024
@liufengyun liufengyun merged commit 62e0641 into scala:main Apr 11, 2024
18 checks passed
@liufengyun liufengyun deleted the fix-stdlib-compile branch April 11, 2024 20:07
@Kordyjan Kordyjan added this to the 3.5.0 milestone May 10, 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.

5 participants