Skip to content

Commit e14a947

Browse files
mcpiromanSpace Team
authored andcommitted
[IR] Do a real copy in copyAttributes, not a link via attributeOwnerId
Such approach is more straightforward and more expected. Because previously two or more elements shared the same map, one could be accidentally changed by modifying the other. Another unexpected behaviour is with chained copyAttributes calls (common example is deepCopy + copyAttributes). Now, even though attributeOwnerId is no longer used for actual IR attributes, it still has many and rather unclear usages. So, while it's not related to "copying attributes" anymore, it was left intact in "copyAttributes()". Its actual purpose should be investigated later. #KT-74331 Fixed
1 parent fe9d9ec commit e14a947

File tree

4 files changed

+92
-30
lines changed

4 files changed

+92
-30
lines changed

compiler/ir/backend.common/test/org/jetbrains/kotlin/backend/common/IrAttributeTests.kt

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package org.jetbrains.kotlin.backend.common
77

88
import org.jetbrains.kotlin.ir.*
9+
import org.jetbrains.kotlin.ir.declarations.copyAttributes
910
import org.jetbrains.kotlin.ir.expressions.IrExpression
1011
import org.jetbrains.kotlin.ir.expressions.impl.IrConstImpl
1112
import org.jetbrains.kotlin.ir.types.impl.IrErrorTypeImpl
@@ -112,16 +113,57 @@ class IrAttributeTests {
112113
}
113114
}
114115

116+
@Test
117+
fun copyToEmpty() {
118+
val element1 = createIrElement()
119+
element1.foo = 10
120+
element1.baz = "test"
121+
122+
val element2 = createIrElement()
123+
element2.copyAttributes(element1)
124+
125+
assertEquals(element1.attributes, element2.attributes)
126+
}
127+
128+
@Test
129+
fun copyFromEmpty() {
130+
val element1 = createIrElement()
131+
132+
val element2 = createIrElement()
133+
element2.foo = 10
134+
element2.baz = "test"
135+
element2.copyAttributes(element1)
136+
137+
assertEquals(element2.foo, 10)
138+
assertEquals(element2.baz, "test")
139+
}
140+
141+
@Test
142+
fun copyWithOverride() {
143+
val element1 = createIrElement()
144+
element1.foo = 10
145+
element1.baz = "new"
146+
147+
val element2 = createIrElement()
148+
element2.baz = "prev"
149+
element2.bar = true
150+
element2.copyAttributes(element1)
151+
152+
assertEquals(element2.foo, 10)
153+
assertEquals(element2.bar, true)
154+
assertEquals(element2.baz, "new")
155+
}
156+
115157
@Test
116158
fun stressTest() {
117159
val element = createIrElement()
118-
val attributes = List(10) { irAttribute<IrExpression, Int>(followAttributeOwner = false).create(null, "attr$it") }
160+
val attributes = List(10) { irAttribute<IrExpression, Int>(followAttributeOwner = true).create(null, "attr$it") }
119161
val realAttributeValues = mutableMapOf<IrAttribute<IrExpression, Int>, Int?>()
120162

121163
val rng = Random(1)
122164
repeat(1000) { i ->
123165
val attr = attributes.random(rng)
124-
when (rng.nextInt(5)) {
166+
when (rng.nextInt(6)) {
125167
0 -> {
126168
element[attr] = i
127169
realAttributeValues[attr] = i
@@ -131,6 +173,12 @@ class IrAttributeTests {
131173
realAttributeValues.remove(attr)
132174
}
133175
2 -> {
176+
val srcEl = createIrElement()
177+
srcEl[attr] = i
178+
element.copyAttributes(srcEl)
179+
realAttributeValues[attr] = i
180+
}
181+
3 -> {
134182
assertEquals<Map<out IrAttribute<*, *>, Any?>>(element.attributes, realAttributeValues)
135183
}
136184
else -> {

compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/IrAttribute.kt

Lines changed: 2 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ fun <E : IrElement> irFlag(followAttributeOwner: Boolean): IrAttribute.Flag.Dele
5050
* Returns a value of [attribute], or null if the value is missing.
5151
*/
5252
operator fun <E : IrElement, T : Any> E.get(attribute: IrAttribute<E, T>): T? {
53-
return (unwrapAttributeOwner(attribute) as IrElementBase).getAttributeInternal(attribute)
53+
return (this as IrElementBase).getAttributeInternal(attribute)
5454
}
5555

5656
/**
@@ -59,30 +59,9 @@ operator fun <E : IrElement, T : Any> E.get(attribute: IrAttribute<E, T>): T? {
5959
* @return The previous value associated with the attribute, or null if the attribute was not present.
6060
*/
6161
operator fun <E : IrElement, T : Any> E.set(attribute: IrAttribute<E, T>, value: T?): T? {
62-
return (unwrapAttributeOwner(attribute) as IrElementBase).setAttributeInternal(attribute, value)
62+
return (this as IrElementBase).setAttributeInternal(attribute, value)
6363
}
6464

65-
private fun <E : IrElement> E.unwrapAttributeOwner(attribute: IrAttribute<E, *>): IrElement {
66-
// This mechanism is intended to aid with future migration off of attributeOwnerId.
67-
// The main change would be that [copyAttributes] wouldn't just set attributeOwnerId to the copied element,
68-
// but do a proper clone of an attribute array.
69-
// Details:
70-
// This place allows us to record if/when a given property is accessed via attributeOwnerId or not.
71-
// If it always is, it means that this particular property should be copied in [copyAttributes] by default.
72-
// Otherwise, we should probably still seek for a possibility to copy it by default. For that, we can also add tracking
73-
// of properties' gets/sets, and check,during compiler execution, whether a value returned from a copy always
74-
// matches the one returned via `attributeOwnerId.get`. If it does, it is probably safe to always copy it as well.
75-
// There is one more peculiarity in the current implementation of [copyAttributes] via attributeOwnerId - it is
76-
// essentially not a copy, but a link between two objects which share the same attributes - if
77-
// those are accessed via attributeOwnerId, this is. However, attributes will employ a hard copy.
78-
// To ensure the new behavior is the-same-or-better, the tracking and comparison can be used as well.
79-
80-
return if (attribute.followAttributeOwner)
81-
this.attributeOwnerId
82-
else this
83-
}
84-
85-
8665
/**
8766
* A key for storing additional data inside [IrElement].
8867
*

compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/IrElementBase.kt

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package org.jetbrains.kotlin.ir
1818

1919
import org.jetbrains.kotlin.ir.visitors.IrTransformer
2020
import org.jetbrains.kotlin.ir.visitors.IrVisitor
21+
import java.util.IdentityHashMap
2122

2223
abstract class IrElementBase : IrElement {
2324
/**
@@ -151,4 +152,39 @@ abstract class IrElementBase : IrElement {
151152
attributes[lastKeyIndex] = null
152153
attributes[lastKeyIndex + 1] = null
153154
}
155+
156+
internal fun copyAttributesFrom(other: IrElementBase, includeAll: Boolean) {
157+
val srcAttributes = other._attributes ?: return
158+
var dstAttributes = _attributes
159+
val mergedAttributes = IdentityHashMap<IrAttribute<*, *>, Any?>((srcAttributes.size + (dstAttributes?.size ?: 0)) / 2)
160+
161+
if (dstAttributes != null) {
162+
for (i in dstAttributes.indices step 2) {
163+
val attr = dstAttributes[i] as IrAttribute<*, *>? ?: break
164+
mergedAttributes[attr] = dstAttributes[i + 1]
165+
}
166+
}
167+
for (i in srcAttributes.indices step 2) {
168+
val attr = srcAttributes[i] as IrAttribute<*, *>? ?: break
169+
if (attr.followAttributeOwner || includeAll) {
170+
mergedAttributes[attr] = srcAttributes[i + 1]
171+
}
172+
}
173+
174+
if (mergedAttributes.isEmpty()) {
175+
return
176+
}
177+
178+
if (dstAttributes == null || dstAttributes.size <= mergedAttributes.size * 2) {
179+
dstAttributes = arrayOfNulls(mergedAttributes.size * 2)
180+
this._attributes = dstAttributes
181+
}
182+
183+
var i = 0
184+
for ((attr, value) in mergedAttributes) {
185+
dstAttributes[i] = attr
186+
dstAttributes[i + 1] = value
187+
i += 2
188+
}
189+
}
154190
}

compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/declarations/IrDeclarations.kt

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,18 +9,17 @@ import org.jetbrains.kotlin.CompilerVersionOfApiDeprecation
99
import org.jetbrains.kotlin.DeprecatedForRemovalCompilerApi
1010
import org.jetbrains.kotlin.descriptors.InlineClassRepresentation
1111
import org.jetbrains.kotlin.descriptors.MultiFieldValueClassRepresentation
12-
import org.jetbrains.kotlin.descriptors.ParameterDescriptor
13-
import org.jetbrains.kotlin.descriptors.ValueParameterDescriptor
12+
import org.jetbrains.kotlin.ir.IrAttribute
1413
import org.jetbrains.kotlin.ir.IrElement
15-
import org.jetbrains.kotlin.ir.ObsoleteDescriptorBasedAPI
16-
import org.jetbrains.kotlin.ir.expressions.IrExpressionBody
14+
import org.jetbrains.kotlin.ir.IrElementBase
1715
import org.jetbrains.kotlin.ir.originalBeforeInline
1816
import org.jetbrains.kotlin.ir.types.IrSimpleType
1917
import org.jetbrains.kotlin.name.Name
2018
import org.jetbrains.kotlin.name.NameUtils.getPackagePartClassNamePrefix
2119
import java.io.File
2220

23-
fun IrElement.copyAttributes(other: IrElement) {
21+
fun IrElement.copyAttributes(other: IrElement, includeAll: Boolean = false) {
22+
(this as IrElementBase).copyAttributesFrom(other as IrElementBase, includeAll)
2423
attributeOwnerId = other.attributeOwnerId
2524
originalBeforeInline = other.originalBeforeInline
2625
}

0 commit comments

Comments
 (0)