Skip to content

Commit fb172fe

Browse files
abtomerabtomer
committed
.
1 parent 9e8fbb6 commit fb172fe

File tree

4 files changed

+230
-29
lines changed

4 files changed

+230
-29
lines changed

‎src/BreakingChangeDetector.scala‎

Lines changed: 69 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ object BreakingChangeDetector {
3030
newFile: ScalaFile
3131
): List[CompareSummary] =
3232
newFile.classes
33-
.filter(checkIfClassHasDerivingAnnotation(_, serializableClasses))
33+
.filter(checkIfClassHasSerialization(_, serializableClasses))
3434
.map(newClass =>
3535
oldFile.classes
3636
.find(_.name == newClass.name)
@@ -39,7 +39,7 @@ object BreakingChangeDetector {
3939
oldClass.name,
4040
listOfRemovedFields(oldClass, newClass),
4141
listOfAddedFieldsWithoutDefaultValue(oldClass, newClass),
42-
checkIfDerivingAnnotationWasChanged(oldClass, newClass),
42+
checkIfSerializationWasChanged(oldClass, newClass),
4343
listOfFieldsThatDefaultValueWasRemoved(oldClass, newClass),
4444
listOfFieldsThatDefaultValueWasAdded(oldClass, newClass)
4545
)
@@ -64,13 +64,37 @@ object BreakingChangeDetector {
6464
.filter(_.default.isEmpty)
6565
.map(_.name)
6666

67+
/**
68+
* Check if class has serialization via @deriving or derives (old way)
69+
*/
6770
private def checkIfClassHasDerivingAnnotation(
6871
classInfo: ClassInfo,
6972
initArgs: List[String]
7073
): Boolean =
7174
classInfo.annotations.exists(x =>
7275
(x.name == "deriving" || x.name == "derives") && x.args.exists(initArgs.contains)
7376
)
77+
78+
/**
79+
* Check if class has serialization via implicit val instances (new way)
80+
*/
81+
private def checkIfClassHasImplicitSerialization(
82+
classInfo: ClassInfo,
83+
serializationTypes: List[String]
84+
): Boolean =
85+
classInfo.annotations.exists(x =>
86+
x.name == "implicit" && x.args.exists(serializationTypes.contains)
87+
)
88+
89+
/**
90+
* Check if class has serialization via either old or new way
91+
*/
92+
private def checkIfClassHasSerialization(
93+
classInfo: ClassInfo,
94+
serializationTypes: List[String]
95+
): Boolean =
96+
checkIfClassHasDerivingAnnotation(classInfo, serializationTypes) ||
97+
checkIfClassHasImplicitSerialization(classInfo, serializationTypes)
7498
private def listOfFieldsThatDefaultValueWasAdded(
7599
oldClass: ClassInfo,
76100
newClass: ClassInfo
@@ -93,27 +117,52 @@ object BreakingChangeDetector {
93117
)
94118
.map(_.name)
95119

96-
private def checkIfDerivingAnnotationWasChanged(
97-
oldClass: ClassInfo,
98-
newClass: ClassInfo
99-
): Boolean = {
100-
val isOldClassContainsSerializable = !(oldClass.annotations
120+
/**
121+
* Extract serialization types from annotations (old way: deriving/derives)
122+
*/
123+
private def extractDerivingSerializationTypes(classInfo: ClassInfo): Set[String] = {
124+
classInfo.annotations
101125
.filter(x => x.name == "deriving" || x.name == "derives")
102126
.flatMap(_.args)
103-
.filter(x => serializableClasses.contains(x))
104-
.length == 0)
127+
.filter(serializableClasses.contains)
128+
.toSet
129+
}
105130

106-
val oldClassDerivingAnnotations =
107-
oldClass.annotations.filter(x => x.name == "deriving" || x.name == "derives")
108-
val newClassDerivingAnnotations =
109-
newClass.annotations.filter(x => x.name == "deriving" || x.name == "derives")
131+
/**
132+
* Extract serialization types from implicit annotations (new way)
133+
*/
134+
private def extractImplicitSerializationTypes(classInfo: ClassInfo): Set[String] = {
135+
classInfo.annotations
136+
.filter(_.name == "implicit")
137+
.flatMap(_.args)
138+
.filter(serializableClasses.contains)
139+
.toSet
140+
}
110141

111-
isOldClassContainsSerializable &&
112-
!oldClassDerivingAnnotations
113-
.forall(oldAnnotation =>
114-
newClassDerivingAnnotations.exists(newAnnotation => {
115-
oldAnnotation.args.toSet.subsetOf(newAnnotation.args.toSet)
116-
})
117-
)
142+
/**
143+
* Get all serialization types (both old and new ways)
144+
*/
145+
private def getAllSerializationTypes(classInfo: ClassInfo): Set[String] = {
146+
extractDerivingSerializationTypes(classInfo) ++ extractImplicitSerializationTypes(classInfo)
147+
}
148+
149+
/**
150+
* Check if serialization was changed (handles both old and new ways, and migration between them)
151+
*/
152+
private def checkIfSerializationWasChanged(
153+
oldClass: ClassInfo,
154+
newClass: ClassInfo
155+
): Boolean = {
156+
val oldSerializationTypes = getAllSerializationTypes(oldClass)
157+
val newSerializationTypes = getAllSerializationTypes(newClass)
158+
159+
// If old class had serialization, check if it changed
160+
if (oldSerializationTypes.nonEmpty) {
161+
// Serialization types changed (e.g., ReadWriter -> Writer, or deriving -> implicit)
162+
oldSerializationTypes != newSerializationTypes
163+
} else {
164+
// Old class didn't have serialization, so this is not a breaking change
165+
false
166+
}
118167
}
119168
}

‎src/FileParser.scala‎

Lines changed: 59 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,30 @@ object FileParser {
3636
}.flatten
3737
}
3838

39+
/**
40+
* Parse implicit val declarations from the AST.
41+
* Returns a map from class name to serialization type (ReadWriter, Writer, Reader, etc.)
42+
*/
43+
private def parseImplicitSerializationInstances(tree: Tree): Map[String, String] = {
44+
def extractFromType(tpe: Type): Option[(String, String)] = tpe match {
45+
case Type.Apply(Type.Name(serializationType), List(Type.Name(className))) =>
46+
Some((className, serializationType))
47+
case Type.Apply(Type.Select(_, Type.Name(serializationType)), List(Type.Name(className))) =>
48+
Some((className, serializationType))
49+
case _ => None
50+
}
51+
52+
def collectImplicitVals(tree: Tree): List[(String, String)] = {
53+
tree.children.flatMap {
54+
case v: Defn.Val if v.mods.exists(_.isInstanceOf[Mod.Implicit]) =>
55+
v.decltpe.flatMap(extractFromType).toList
56+
case x => collectImplicitVals(x)
57+
}
58+
}
59+
60+
collectImplicitVals(tree).toMap
61+
}
62+
3963
def addSuffixToDuplicates(list: List[ClassInfo]): List[ClassInfo] = {
4064
var countMap = collection.mutable.Map[String, Int]().withDefaultValue(0)
4165
list.map { c =>
@@ -45,24 +69,50 @@ object FileParser {
4569
}
4670
}
4771

72+
/**
73+
* Extract annotations from @deriving or derives syntax (old way)
74+
*/
75+
private def extractDerivingAnnotations(classDef: Defn.Class): List[Annotation] = {
76+
val derivingFromAnnotation =
77+
classDef.mods
78+
.flatMap(_.children)
79+
.collect { case Init(tpe, _, args) =>
80+
Annotation(tpe.toString, args.flatten.map(_.toString))
81+
}
82+
val derivingFromDerives =
83+
classDef.templ.derives.map(d => Annotation("derives", List(d.toString)))
84+
derivingFromAnnotation ++ derivingFromDerives
85+
}
86+
87+
/**
88+
* Extract annotations from implicit val instances (new way)
89+
*/
90+
private def extractImplicitSerializationAnnotations(
91+
className: String,
92+
implicitInstances: Map[String, String]
93+
): List[Annotation] = {
94+
implicitInstances
95+
.get(className)
96+
.map(serializationType => Annotation("implicit", List(serializationType)))
97+
.toList
98+
}
99+
48100
def parse(
49101
content: String,
50102
dialect: Dialect = dialects.Scala213Source3
51103
): ScalaFile = {
52104
val input = Input.String(content)
53105
val exampleTree: Source = dialect(input).parse[Source].get
54106

107+
// Parse implicit serialization instances (new way)
108+
val implicitInstances = parseImplicitSerializationInstances(exampleTree)
109+
110+
// Parse classes with their annotations (old way)
55111
val tree =
56112
parseTreeClasses(exampleTree)
57113
.map(c => {
58-
val derivingFromAnnotation =
59-
c.mods
60-
.flatMap(_.children)
61-
.collect { case Init(tpe, _, args) =>
62-
Annotation(tpe.toString, args.flatten.map(_.toString))
63-
}
64-
val derivingFromDerives =
65-
c.templ.derives.map(d => Annotation("derives", List(d.toString)))
114+
val derivingAnnotations = extractDerivingAnnotations(c)
115+
val implicitAnnotations = extractImplicitSerializationAnnotations(c.name.value, implicitInstances)
66116
ClassInfo(
67117
c.name.value,
68118
c.ctor.paramss.headOption.getOrElse(Nil).map(p =>
@@ -72,7 +122,7 @@ object FileParser {
72122
p.default.map(_.toString)
73123
)
74124
),
75-
derivingFromAnnotation ++ derivingFromDerives
125+
derivingAnnotations ++ implicitAnnotations
76126
)
77127
})
78128
ScalaFile(

‎test/BreakingChangeDetector.test.scala‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,4 +256,27 @@ class BreakingChangeDetectorTest extends munit.FunSuite {
256256
runWithFiles("V10.scala_test.prev", "V10.scala_test")
257257
runWithFiles("V10.scala_test", "V10.scala_test.prev")
258258
}
259+
260+
test("Should detect breaking change with implicit ReadWriter - added field without default") {
261+
val oldFile = Thread
262+
.currentThread()
263+
.getContextClassLoader
264+
.getResource("scala-3/implicit-breaking-change.scala_test.prev")
265+
.getPath
266+
val oldFileParsed = FileParser.fromPathToClassDef(oldFile)
267+
val newFile = Thread
268+
.currentThread()
269+
.getContextClassLoader
270+
.getResource("scala-3/implicit-breaking-change.scala_test")
271+
.getPath
272+
val newFileParsed = FileParser.fromPathToClassDef(newFile)
273+
val compared =
274+
BreakingChangeDetector.detectBreakingChange(oldFileParsed, newFileParsed)
275+
println(compared)
276+
assert(
277+
compared.find(_.isBreakingChange).nonEmpty
278+
)
279+
val breakingChange = compared.find(_.isBreakingChange).get
280+
assert(breakingChange.addedFieldsWithoutDefaultValues.contains("email"))
281+
}
259282
}

‎test/FileParser.test.scala‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,4 +91,83 @@ class FileParserTest extends munit.FunSuite {
9191
assert(derivesAnnotation.isDefined)
9292
assert(derivesAnnotation.get.args.contains("ReadWriter"))
9393
}
94+
95+
test("parses implicit ReadWriter instances") {
96+
val file = Thread
97+
.currentThread()
98+
.getContextClassLoader
99+
.getResource("scala-3/implicit-serialization.scala_test")
100+
.getPath
101+
val parsedFile = FileParser.fromPathToClassDef(file)
102+
103+
// Should find all classes
104+
assert(parsedFile.classes.length == 5)
105+
106+
// AchOriginationJobArgs should have implicit ReadWriter
107+
val achClass = parsedFile.classes.find(_.name == "AchOriginationJobArgs").get
108+
assert(achClass.fields.length == 3)
109+
assert(achClass.fields(0).name == "cutoff")
110+
assert(achClass.fields(2).name == "companyName")
111+
assert(achClass.fields(2).default.isDefined, "companyName should have default value")
112+
// Should have ReadWriter annotation from implicit instance
113+
val achReadWriter = achClass.annotations.find(a =>
114+
(a.name == "deriving" || a.name == "derives" || a.name == "implicit") &&
115+
a.args.contains("ReadWriter")
116+
)
117+
assert(achReadWriter.isDefined, "AchOriginationJobArgs should have ReadWriter from implicit instance")
118+
119+
// UserWithWriter should have implicit Writer
120+
val userClass = parsedFile.classes.find(_.name == "UserWithWriter").get
121+
assert(userClass.fields.length == 2)
122+
val userWriter = userClass.annotations.find(a =>
123+
(a.name == "deriving" || a.name == "derives" || a.name == "implicit") &&
124+
a.args.contains("Writer")
125+
)
126+
assert(userWriter.isDefined, "UserWithWriter should have Writer from implicit instance")
127+
128+
// ProductWithReader should have implicit Reader
129+
val productClass = parsedFile.classes.find(_.name == "ProductWithReader").get
130+
assert(productClass.fields.length == 2)
131+
val productReader = productClass.annotations.find(a =>
132+
(a.name == "deriving" || a.name == "derives" || a.name == "implicit") &&
133+
a.args.contains("Reader")
134+
)
135+
assert(productReader.isDefined, "ProductWithReader should have Reader from implicit instance")
136+
137+
// OrderWithReadWriter should have implicit ReadWriter
138+
val orderClass = parsedFile.classes.find(_.name == "OrderWithReadWriter").get
139+
assert(orderClass.fields.length == 3)
140+
assert(orderClass.fields(2).name == "status")
141+
assert(orderClass.fields(2).default.isDefined, "status should have default value")
142+
val orderReadWriter = orderClass.annotations.find(a =>
143+
(a.name == "deriving" || a.name == "derives" || a.name == "implicit") &&
144+
a.args.contains("ReadWriter")
145+
)
146+
assert(orderReadWriter.isDefined, "OrderWithReadWriter should have ReadWriter from implicit instance")
147+
148+
// NotSerializable should NOT have any serialization annotation
149+
val notSerializableClass = parsedFile.classes.find(_.name == "NotSerializable").get
150+
assert(notSerializableClass.fields.length == 2)
151+
val hasSerialization = notSerializableClass.annotations.exists(a =>
152+
(a.name == "deriving" || a.name == "derives" || a.name == "implicit") &&
153+
(a.args.contains("ReadWriter") || a.args.contains("Writer") || a.args.contains("Reader"))
154+
)
155+
assert(!hasSerialization, "NotSerializable should not have any serialization annotation")
156+
}
157+
158+
test("parses implicit ReadWriter with qualified type (upickle.default.ReadWriter)") {
159+
val file = Thread
160+
.currentThread()
161+
.getContextClassLoader
162+
.getResource("scala-3/implicit-qualified-type.scala_test")
163+
.getPath
164+
val parsedFile = FileParser.fromPathToClassDef(file)
165+
166+
val testClass = parsedFile.classes.find(_.name == "TestClass").get
167+
assert(testClass.fields.length == 1)
168+
val readWriter = testClass.annotations.find(a =>
169+
a.name == "implicit" && a.args.contains("ReadWriter")
170+
)
171+
assert(readWriter.isDefined, "TestClass should have ReadWriter from implicit instance with qualified type")
172+
}
94173
}

0 commit comments

Comments
 (0)