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

Make initialization checker see through synthetic applys #14283

Merged
merged 7 commits into from Jan 25, 2022

Conversation

liufengyun
Copy link
Contributor

@liufengyun liufengyun commented Jan 17, 2022

Make initialization checker see through synthetic applys

The initialization checker supports non-hot arguments to constructors, but not for methods.
For Scala to benefit from the expressiveness, we need to make the initialization checker see through synthetic applys.

We currently don't allow parameter-sensitivity for normal method arguments, as they would require complications of the abstract domains and it may cause unexpected usability/performance issues. Instead, we expect users to use inline def or @unchecked for such complex initialization code.

case class A(b: B)

object A:
  def foo(b: B) = new A(b)
  inline def bar(b: B) = new A(b)

class B:
  val a = A(this)
  val a2 = A.foo(this)   // error
  val a3 = A.bar(this)

An alternative to #13999.

Copy link
Contributor

@olhotak olhotak left a comment

Choose a reason for hiding this comment

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

LGTM

withEnv(if isLocal then env else Env.empty) {
eval(ddef.rhs, ref, cls, cacheResult = true) ++ checkArgs
}
if isSyntheticApply(meth) then
Copy link
Contributor

Choose a reason for hiding this comment

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

I wondered whether it would make sense to instantiate a synthetic apply even if target.hasSource is false. But if we don't have source for the synthetic apply, then we probably also don't have source for the class constructor, so instantiate wouldn't be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wondered whether it would make sense to instantiate a synthetic apply even if target.hasSource is false. But if we don't have source for the synthetic apply, then we probably also don't have source for the class constructor, so instantiate wouldn't be useful.

Yes, exactly.

I found one bug in the code above: the outer for instantiate is incorrect. I'll come up with a test to show the bug and fix it in this PR.

instantiate(klass, klass.primaryConstructor, args, source)
val outerCls = klass.owner.lexicallyEnclosingClass.asClass
val outer = resolveOuterSelect(outerCls, ref, 1, source)
Semantic.instantiate(outer)(klass, klass.primaryConstructor, args, source)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not outer.instantiate(...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch, this is now updated.

@olhotak olhotak merged commit bcc3d2b into scala:master Jan 25, 2022
@olhotak olhotak deleted the init-synth-apply branch January 25, 2022 06:36
@Kordyjan Kordyjan added this to the 3.1.2 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.

None yet

3 participants