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

Jump to bottom

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion compiler/src/dotty/tools/dotc/ast/TreeInfo.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,8 +578,9 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] =>
sym.owner.isPrimitiveValueClass
|| sym.owner == defn.StringClass
|| defn.pureMethods.contains(sym)

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.


/** The purity level of this reference.
* @return
Expand Down
1 change: 1 addition & 0 deletions tests/run/i17317-b.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hello world
12 changes: 12 additions & 0 deletions tests/run/i17317-b.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
object foo {
object HelloGen {
println("hello world")
}
val Hello = HelloGen
}

import foo.Hello

object Test {
def main(args: Array[String]): Unit = Hello: Unit
}
1 change: 1 addition & 0 deletions tests/run/i17317.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hello world
12 changes: 12 additions & 0 deletions tests/run/i17317.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package object foo {
object HelloGen {
println("hello world")
}
val Hello = HelloGen
}

import foo.Hello

object Test {
def main(args: Array[String]): Unit = Hello: Unit
}
Loading