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 #17549: Unify how Memoize and Constructors decide what fields need storing. #17560

Merged
merged 1 commit into from
May 23, 2023

Commits on May 23, 2023

  1. Fix scala#17549: Unify how Memoize and Constructors decide what field…

    …s need storing.
    
    The Memoize and Constructors have to work together and agree on
    which `final val`s actually need storing in a field. Previously,
    they used slightly different criteria: one on the result type, and
    one on the rhs (with an additional Scala.js-specific eligibility
    condition). That discrepancy resulted in the crash/wrong codegen
    in the issue.
    
    We now unify both: we avoid a field if and only if all of the
    following apply:
    
    * it is a `final val`,
    * its result *type* is a `ConstantType`, and
    * it is eligible according to Scala.js semantics.
    
    In particular, there is no condition on the right-hand-side. We
    avoid a field even if the right-hand-side has side effects. The
    side effects are moved to the constructor regardless.
    
    ---
    
    This introduces both progressions and regressions in the amount of
    fields we generate. We can avoid fields even for side-effecting
    rhs'es, as long as the result type is constant. On the other hand,
    we now create a field for `final val`s with non-constant result
    type, even if the rhs is a literal.
    
    While the latter is a regression for Scala 3, it aligns with the
    behavior of Scala 2. It also has the nice benefit that whether or
    not a val has a field is now independent of its *implementation*,
    and only dependent on its *API*. Overall, I think this is a
    trade-off worth taking.
    
    We could reintroduce that optimization in the future (but in
    classes only; not in traits), if we really want to, although that
    would require dedicated code.
    sjrd committed May 23, 2023
    Configuration menu
    Copy the full SHA
    ac2b53b View commit details
    Browse the repository at this point in the history