Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix purity check for val inside of object #19598

Merged
merged 1 commit into from
Feb 9, 2024
Merged

Fix purity check for val inside of object #19598

merged 1 commit into from
Feb 9, 2024

Conversation

aherlihy
Copy link
Contributor

@aherlihy aherlihy commented Feb 2, 2024

Fixes #17317

tree.tpe.isInstanceOf[ConstantType] && tree.symbol != NoSymbol && isKnownPureOp(tree.symbol) // A constant expression with pure arguments is pure.
|| fn.symbol.isStableMember && !fn.symbol.is(Lazy) // constructors of no-inits classes are stable
|| fn.symbol.isStableMember && fn.symbol.isConstructor // constructors of no-inits classes are stable
Copy link
Member

Choose a reason for hiding this comment

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

I was a bit confused by what was going on here, but after reading 471fcd9 it seems the issue is that:

  1. Constructors of traits that do not have side-effects will return true to isStableMember
  2. Other members might return true just because they're stable (always return the same value) even if they do have side-effects. The previous check excluding lazy was not broad enough to handle this (in fact, arbitrary methods can be marked stable with scala.annotation.unchecked.uncheckedStable)

If I understand 471fcd9 correctly, I think this check is equivalent to:

Suggested change
|| fn.symbol.isStableMember && fn.symbol.isConstructor // constructors of no-inits classes are stable
|| fn.symbol.isConstructor && fn.symbol.owner.is(NoInits)

Which is more obviously correct. @sjrd: Can you confirm my understanding here?

Copy link
Member

Choose a reason for hiding this comment

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

I think this check is equivalent to:

I don't think so, because the condition required to set the constructor as stable is not just fn.symbol.owner.is(NoInits) but

if myFlags.is(Trait) then myFlags.is(NoInits) else isNoInitsRealClass

where

    def isNoInitsRealClass(using Context): Boolean =
      isRealClass &&
      (asClass.baseClasses.forall(_.is(NoInits)) || defn.isAssuredNoInits(symbol))

So isStableMember is the efficient way to verify that.

Perhaps we can fn.symbol.isStableMember && fn.symbol.isConstructor into a isStableConstructor method, and then throw this detail in its docs. Or we can do that in situ here.

@sjrd sjrd merged commit 5a5ea06 into scala:main Feb 9, 2024
19 checks passed
@Kordyjan Kordyjan added this to the 3.4.1 milestone Feb 14, 2024
WojciechMazur added a commit that referenced this pull request Jul 1, 2024
Backports #19598 to the LTS branch.

PR submitted by the release tooling.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive dead code elimination on pure expressions
5 participants