Skip to content

Commit 942f179

Browse files
author
EnzeXing
committed
Address comments
1 parent b05e054 commit 942f179

File tree

2 files changed

+83
-60
lines changed

2 files changed

+83
-60
lines changed

compiler/src/dotty/tools/dotc/transform/init/Objects.scala

Lines changed: 82 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import util.{ SourcePosition, NoSourcePosition }
1818
import config.Printers.init as printer
1919
import reporting.StoreReporter
2020
import reporting.trace as log
21-
import reporting.trace.force as forcelog
2221
import typer.Applications.*
2322

2423
import Errors.*
@@ -30,6 +29,7 @@ import scala.collection.mutable
3029
import scala.annotation.tailrec
3130
import scala.annotation.constructorOnly
3231
import dotty.tools.dotc.core.Flags.AbstractOrTrait
32+
import dotty.tools.dotc.util.SrcPos
3333

3434
/** Check initialization safety of static objects
3535
*
@@ -55,10 +55,10 @@ import dotty.tools.dotc.core.Flags.AbstractOrTrait
5555
* This principle not only put initialization of static objects on a solid foundation, but also
5656
* avoids whole-program analysis.
5757
*
58-
* 2. The design is based on the concept of "cold aliasing" --- a cold alias may not be actively
59-
* used during initialization, i.e., it's forbidden to call methods or access fields of a cold
60-
* alias. Method arguments are cold aliases by default unless specified to be sensitive. Method
61-
* parameters captured in lambdas or inner classes are always cold aliases.
58+
* 2. The design is based on the concept of "TopWidenedValue" --- a TopWidenedValue may not be actively
59+
* used during initialization, i.e., it's forbidden to call methods or access fields of a TopWidenedValue.
60+
* Method arguments are TopWidenedValues by default unless specified to be sensitive. Method
61+
* parameters captured in lambdas or inner classes are always TopWidenedValues.
6262
*
6363
* 3. It is inter-procedural and flow-sensitive.
6464
*
@@ -94,12 +94,11 @@ class Objects(using Context @constructorOnly):
9494
* | OfArray(object[owner], regions)
9595
* | Fun(..., env) // value elements that can be contained in ValueSet
9696
* | SafeValue // values on which method calls and field accesses won't cause warnings. Int, String, etc.
97-
* | UnknownValue
9897
* vs ::= ValueSet(ve) // set of abstract values
9998
* Bottom ::= ValueSet(Empty)
100-
* val ::= ve | TopWidenedValue | vs | Package // all possible abstract values in domain
99+
* val ::= ve | TopWidenedValue | UnknownValue | vs | Package // all possible abstract values in domain
101100
* Ref ::= ObjectRef | OfClass // values that represent a reference to some (global or instance) object
102-
* ThisValue ::= Ref | UnknownValue // possible values for 'this'
101+
* ThisValue ::= Ref | TopWidenedValue // possible values for 'this'
103102
*
104103
* refMap = Ref -> ( valsMap, varsMap, outersMap ) // refMap stores field informations of an object or instance
105104
* valsMap = valsym -> val // maps immutable fields to their values
@@ -233,36 +232,59 @@ class Objects(using Context @constructorOnly):
233232
* Assumption: all methods calls on such values should not trigger initialization of global objects
234233
* or read/write mutable fields
235234
*/
236-
case class SafeValue(tpe: Type) extends ValueElement:
237-
// tpe could be a AppliedType(java.lang.Class, T)
238-
val baseType = if tpe.isInstanceOf[AppliedType] then tpe.asInstanceOf[AppliedType].underlying else tpe
239-
assert(baseType.isInstanceOf[TypeRef], "Invalid creation of SafeValue! Type = " + tpe)
240-
val typeSymbol = baseType.asInstanceOf[TypeRef].symbol
241-
assert(SafeValue.safeTypeSymbols.contains(typeSymbol), "Invalid creation of SafeValue! Type = " + tpe)
235+
case class SafeValue(typeSymbol: Symbol) extends ValueElement:
236+
assert(SafeValue.safeTypeSymbols.contains(typeSymbol), "Invalid creation of SafeValue! Type = " + typeSymbol)
242237
def show(using Context): String = "SafeValue of " + typeSymbol.show
243-
override def equals(that: Any): Boolean =
244-
that.isInstanceOf[SafeValue] && that.asInstanceOf[SafeValue].typeSymbol == typeSymbol
245238

246239
object SafeValue:
247240
val safeTypeSymbols =
241+
defn.StringClass ::
248242
(defn.ScalaNumericValueTypeList ++
249-
List(defn.UnitType, defn.BooleanType, defn.StringType.asInstanceOf[TypeRef], defn.NullType, defn.ClassClass.typeRef))
243+
List(defn.UnitType, defn.BooleanType, defn.NullType, defn.ClassClass.typeRef))
250244
.map(_.symbol)
251245

246+
def getSafeTypeSymbol(tpe: Type): Option[Symbol] =
247+
val baseType = if tpe.isInstanceOf[AppliedType] then tpe.asInstanceOf[AppliedType].underlying else tpe
248+
if baseType.isInstanceOf[TypeRef] then
249+
val typeRef = baseType.asInstanceOf[TypeRef]
250+
val typeSymbol = typeRef.symbol
251+
val typeAlias = typeRef.translucentSuperType
252+
if safeTypeSymbols.contains(typeSymbol) then
253+
Some(typeSymbol)
254+
else if typeAlias.isInstanceOf[TypeRef] && typeAlias.asInstanceOf[TypeRef].symbol == defn.StringClass then
255+
// Special case, type scala.Predef.String = java.lang.String
256+
Some(defn.StringClass)
257+
else None
258+
else
259+
None
260+
261+
def apply(tpe: Type): SafeValue =
262+
// tpe could be a AppliedType(java.lang.Class, T)
263+
val typeSymbol = getSafeTypeSymbol(tpe)
264+
assert(typeSymbol.isDefined, "Invalid creation of SafeValue with type " + tpe)
265+
new SafeValue(typeSymbol.get)
266+
252267
/**
253268
* Represents a set of values
254269
*
255270
* It comes from `if` expressions.
256271
*/
257-
case class ValueSet(values: ListSet[ValueElement]) extends Value:
272+
case class ValueSet(values: Set[ValueElement]) extends Value:
258273
def show(using Context) = values.map(_.show).mkString("[", ",", "]")
259274

260-
case class Package(packageSym: Symbol) extends Value:
261-
def show(using Context): String = "Package(" + packageSym.show + ")"
275+
case class Package(packageModuleClass: ClassSymbol) extends Value:
276+
def show(using Context): String = "Package(" + packageModuleClass.show + ")"
277+
278+
object Package:
279+
def apply(packageSym: Symbol): Package =
280+
assert(packageSym.is(Flags.Package), "Invalid symbol to create Package!")
281+
Package(packageSym.moduleClass.asClass)
262282

263283
/** Represents values unknown to the checker, such as values loaded without source
284+
* UnknownValue is not ValueElement since RefSet containing UnknownValue
285+
* is equivalent to UnknownValue
264286
*/
265-
case object UnknownValue extends ValueElement:
287+
case object UnknownValue extends Value:
266288
def show(using Context): String = "UnknownValue"
267289

268290
/** Represents values lost due to widening
@@ -636,22 +658,26 @@ class Objects(using Context @constructorOnly):
636658

637659
extension (a: Value)
638660
def join(b: Value): Value =
661+
assert(!a.isInstanceOf[Package] && !b.isInstanceOf[Package])
639662
(a, b) match
640663
case (TopWidenedValue, _) => TopWidenedValue
641664
case (_, TopWidenedValue) => TopWidenedValue
642-
case (Package(_), _) => UnknownValue // should not happen
643-
case (_, Package(_)) => UnknownValue
665+
case (UnknownValue, _) => UnknownValue
666+
case (_, UnknownValue) => UnknownValue
644667
case (Bottom, b) => b
645668
case (a, Bottom) => a
646669
case (ValueSet(values1), ValueSet(values2)) => ValueSet(values1 ++ values2)
647670
case (a : ValueElement, ValueSet(values)) => ValueSet(values + a)
648671
case (ValueSet(values), b : ValueElement) => ValueSet(values + b)
649-
case (a : ValueElement, b : ValueElement) => ValueSet(ListSet(a, b))
672+
case (a : ValueElement, b : ValueElement) => ValueSet(Set(a, b))
673+
case _ => Bottom
650674

651675
def remove(b: Value): Value = (a, b) match
652676
case (ValueSet(values1), b: ValueElement) => ValueSet(values1 - b)
653677
case (ValueSet(values1), ValueSet(values2)) => ValueSet(values1.removedAll(values2))
654678
case (a: Ref, b: Ref) if a.equals(b) => Bottom
679+
case (a: SafeValue, b: SafeValue) if a == b => Bottom
680+
case (a: Package, b: Package) if a == b => Bottom
655681
case _ => a
656682

657683
def widen(height: Int)(using Context): Value = log("widening value " + a.show + " down to height " + height, printer, (_: Value).show) {
@@ -664,7 +690,7 @@ class Objects(using Context @constructorOnly):
664690
values.map(ref => ref.widen(height)).join
665691

666692
case Fun(code, thisV, klass, env) =>
667-
Fun(code, thisV.widenRefOrCold(height), klass, env.widen(height - 1))
693+
Fun(code, thisV.widenThisValue(height), klass, env.widen(height - 1))
668694

669695
case ref @ OfClass(klass, outer, _, args, env) =>
670696
val outer2 = outer.widen(height - 1)
@@ -691,8 +717,10 @@ class Objects(using Context @constructorOnly):
691717
val klass = sym.asClass
692718
a match
693719
case UnknownValue | TopWidenedValue => a
694-
case Package(packageSym) =>
695-
if packageSym.moduleClass.equals(sym) || (klass.denot.isPackageObject && klass.owner.equals(sym)) then a else Bottom
720+
case Package(packageModuleClass) =>
721+
// the typer might mistakenly set the receiver to be a package instead of package object.
722+
// See pos/packageObjectStringInterpolator.scala
723+
if packageModuleClass == klass || (klass.denot.isPackageObject && klass.owner == packageModuleClass) then a else Bottom
696724
case v: SafeValue => if v.typeSymbol.asClass.isSubClass(klass) then a else Bottom
697725
case ref: Ref => if ref.klass.isSubClass(klass) then ref else Bottom
698726
case ValueSet(values) => values.map(v => v.filterClass(klass)).join
@@ -701,8 +729,8 @@ class Objects(using Context @constructorOnly):
701729
if klass.isOneOf(AbstractOrTrait) && klass.baseClasses.exists(defn.isFunctionClass) then fun else Bottom
702730

703731
extension (value: ThisValue)
704-
def widenRefOrCold(height : Int)(using Context) : ThisValue =
705-
assert(height > 0, "Cannot call widenRefOrCold with height 0!")
732+
def widenThisValue(height : Int)(using Context) : ThisValue =
733+
assert(height > 0, "Cannot call widenThisValue with height 0!")
706734
value.widen(height).asInstanceOf[ThisValue]
707735

708736
extension (values: Iterable[Value])
@@ -714,6 +742,12 @@ class Objects(using Context @constructorOnly):
714742
*/
715743
val reportUnknown: Boolean = false
716744

745+
def reportWarningForUnknownValue(msg: => String, pos: SrcPos)(using Context): Value =
746+
if reportUnknown then
747+
report.warning(msg, pos)
748+
Bottom
749+
else
750+
UnknownValue
717751

718752
/** Handle method calls `e.m(args)`.
719753
*
@@ -730,13 +764,9 @@ class Objects(using Context @constructorOnly):
730764
report.warning("Value is unknown to the checker due to widening. " + Trace.show, Trace.position)
731765
Bottom
732766
case UnknownValue =>
733-
if reportUnknown then
734-
report.warning("Using unknown value. " + Trace.show, Trace.position)
735-
Bottom
736-
else
737-
UnknownValue
767+
reportWarningForUnknownValue("Using unknown value. " + Trace.show, Trace.position)
738768

739-
case Package(packageSym) =>
769+
case Package(packageModuleClass) =>
740770
if meth.equals(defn.throwMethod) then
741771
Bottom
742772
// calls on packages are unexpected. However the typer might mistakenly
@@ -750,17 +780,18 @@ class Objects(using Context @constructorOnly):
750780
val packageObj = accessObject(meth.owner.moduleClass.asClass)
751781
call(packageObj, meth, args, receiver, superType, needResolve)
752782

753-
case v @ SafeValue(tpe) =>
783+
case v @ SafeValue(_) =>
754784
// Assume such method is pure. Check return type, only try to analyze body if return type is not safe
755785
val target = resolve(v.typeSymbol.asClass, meth)
756786
if !target.hasSource then
757787
UnknownValue
758788
else
759789
val ddef = target.defTree.asInstanceOf[DefDef]
760790
val returnType = ddef.tpt.tpe
761-
if SafeValue.safeTypeSymbols.contains(returnType.typeSymbol) then
791+
val typeSymbol = SafeValue.getSafeTypeSymbol(returnType)
792+
if typeSymbol.isDefined then
762793
// since method is pure and return type is safe, no need to analyze method body
763-
SafeValue(returnType)
794+
SafeValue(typeSymbol.get)
764795
else
765796
val cls = target.owner.enclosingClass.asClass
766797
// convert SafeType to an OfClass before analyzing method body
@@ -864,7 +895,7 @@ class Objects(using Context @constructorOnly):
864895
value
865896
else
866897
// In future, we will have Tasty for stdlib classes and can abstractly interpret that Tasty.
867-
// For now, return `Cold` to ensure soundness and trigger a warning.
898+
// For now, return `UnknownValue` to ensure soundness and trigger a warning when reportUnknown = true.
868899
UnknownValue
869900
end if
870901
end if
@@ -925,11 +956,7 @@ class Objects(using Context @constructorOnly):
925956
report.warning("Value is unknown to the checker due to widening. " + Trace.show, Trace.position)
926957
Bottom
927958
case UnknownValue =>
928-
if reportUnknown then
929-
report.warning("Using unknown value. " + Trace.show, Trace.position)
930-
Bottom
931-
else
932-
UnknownValue
959+
reportWarningForUnknownValue("Using unknown value. " + Trace.show, Trace.position)
933960

934961
case v @ SafeValue(_) =>
935962
if v.typeSymbol != defn.NullClass then
@@ -938,13 +965,13 @@ class Objects(using Context @constructorOnly):
938965
end if
939966
Bottom
940967

941-
case Package(packageSym) =>
968+
case Package(packageModuleClass) =>
942969
if field.isStaticObject then
943970
accessObject(field.moduleClass.asClass)
944971
else if field.is(Flags.Package) then
945972
Package(field)
946973
else
947-
report.warning("[Internal error] Unexpected selection on package " + packageSym.show + ", field = " + field.show + Trace.show, Trace.position)
974+
report.warning("[Internal error] Unexpected selection on package " + packageModuleClass.show + ", field = " + field.show + Trace.show, Trace.position)
948975
Bottom
949976

950977
case ref: Ref =>
@@ -1016,11 +1043,9 @@ class Objects(using Context @constructorOnly):
10161043
case TopWidenedValue =>
10171044
report.warning("Value is unknown to the checker due to widening. " + Trace.show, Trace.position)
10181045
case UnknownValue =>
1019-
if reportUnknown then
1020-
report.warning("Assigning to unknown value. " + Trace.show, Trace.position)
1021-
end if
1046+
val _ = reportWarningForUnknownValue("Assigning to unknown value. " + Trace.show, Trace.position)
10221047
case p: Package =>
1023-
report.warning("[Internal error] unexpected tree in assignment, package = " + p.packageSym.show + Trace.show, Trace.position)
1048+
report.warning("[Internal error] unexpected tree in assignment, package = " + p.show + Trace.show, Trace.position)
10241049
case fun: Fun =>
10251050
report.warning("[Internal error] unexpected tree in assignment, fun = " + fun.code.show + Trace.show, Trace.position)
10261051
case arr: OfArray =>
@@ -1063,11 +1088,7 @@ class Objects(using Context @constructorOnly):
10631088
Bottom
10641089

10651090
case UnknownValue =>
1066-
if reportUnknown then
1067-
report.warning("Instantiating when outer is unknown. " + Trace.show, Trace.position)
1068-
Bottom
1069-
else
1070-
UnknownValue
1091+
reportWarningForUnknownValue("Instantiating when outer is unknown. " + Trace.show, Trace.position)
10711092

10721093
case outer: (Ref | TopWidenedValue.type | Package) =>
10731094
if klass == defn.ArrayClass then
@@ -1091,7 +1112,7 @@ class Objects(using Context @constructorOnly):
10911112
report.warning("[Internal error] top-level class should have `Package` as outer, class = " + klass.show + ", outer = " + outer.show + ", " + Trace.show, Trace.position)
10921113
(Bottom, Env.NoEnv)
10931114
else
1094-
(thisV.widenRefOrCold(1), Env.NoEnv)
1115+
(thisV.widenThisValue(1), Env.NoEnv)
10951116
else
10961117
// klass.enclosingMethod returns its primary constructor
10971118
Env.resolveEnv(klass.owner.enclosingMethod, thisV, summon[Env.Data]).getOrElse(UnknownValue -> Env.NoEnv)
@@ -1154,8 +1175,10 @@ class Objects(using Context @constructorOnly):
11541175
case fun: Fun =>
11551176
given Env.Data = Env.ofByName(sym, fun.env)
11561177
eval(fun.code, fun.thisV, fun.klass)
1157-
case UnknownValue | TopWidenedValue =>
1158-
report.warning("Calling on unknown value. " + Trace.show, Trace.position)
1178+
case UnknownValue =>
1179+
reportWarningForUnknownValue("Calling on unknown value. " + Trace.show, Trace.position)
1180+
case TopWidenedValue =>
1181+
report.warning("Calling on value lost due to widening. " + Trace.show, Trace.position)
11591182
Bottom
11601183
case _: ValueSet | _: Ref | _: OfArray | _: Package | SafeValue(_) =>
11611184
report.warning("[Internal error] Unexpected by-name value " + value.show + ". " + Trace.show, Trace.position)
@@ -1607,7 +1630,7 @@ class Objects(using Context @constructorOnly):
16071630
end if
16081631
end if
16091632
// TODO: receiverType is the companion object type, not the class itself;
1610-
// cannot filter scritunee by this type
1633+
// cannot filter scrutinee by this type
16111634
(receiverType, scrutinee)
16121635

16131636
case Ident(nme.WILDCARD) | Ident(nme.WILDCARD_STAR) =>

compiler/test/dotty/tools/dotc/CompilationTests.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ class CompilationTests {
233233
implicit val testGroup: TestGroup = TestGroup("checkInitGlobal")
234234
compileFilesInDir("tests/init-global/warn", defaultOptions.and("-Ysafe-init-global"), FileFilter.exclude(TestSources.negInitGlobalScala2LibraryTastyExcludelisted)).checkWarnings()
235235
compileFilesInDir("tests/init-global/pos", defaultOptions.and("-Ysafe-init-global", "-Xfatal-warnings"), FileFilter.exclude(TestSources.posInitGlobalScala2LibraryTastyExcludelisted)).checkCompile()
236-
if Properties.usingScalaLibraryTasty then
236+
if Properties.usingScalaLibraryTasty && !Properties.usingScalaLibraryCCTasty then
237237
compileFilesInDir("tests/init-global/warn-tasty", defaultOptions.and("-Ysafe-init-global"), FileFilter.exclude(TestSources.negInitGlobalScala2LibraryTastyExcludelisted)).checkWarnings()
238238
compileFilesInDir("tests/init-global/pos-tasty", defaultOptions.and("-Ysafe-init-global", "-Xfatal-warnings"), FileFilter.exclude(TestSources.posInitGlobalScala2LibraryTastyExcludelisted)).checkCompile()
239239
end if

0 commit comments

Comments
 (0)