Proposal: Widen List AttributeTypes to Seq

[NthPortal]

Currently, the four array AttributeTypes are required to be Lists; however, varargs is statically of type Seq and at runtime (for literals) of type i.ArraySeq. I opine that any immutable Seq is just as safe and well-behaved as List, and that we shouldn't force a conversion to List.

[iRevive]

varargs is statically of type Seq

But where do we build an Attribute from varargs?

[iRevive]

The hashing function of the Attribute relies on the universal hash code:

otel4s/core/common/src/main/scala/org/typelevel/otel4s/Attribute.scala

Lines 79 to 82 in 526fd2d

	implicit val hashAttributeExistential: Hash[Attribute[_]] = {
	implicit val hashAny: Hash[Any] = Hash.fromUniversalHashCode
	Hash.by(a => (a.key, a.value))
	}

Seq is an open interface that can be extended. E.g. :

class FixedSizeLongSeq(value: Long) extends Seq[Long] {
  def apply(i: Int): Long = if (i == 0) value else sys.error("out of bounds")
  def length: Int = 1
  def iterator: Iterator[Long] = Iterator(value)
  override def hashCode(): Int = value.hashCode()
}

And as a result, we can break the hashing logic:

val seq1: Seq[Long] = Seq(1L)
val seq2: Seq[Long] = new FixedSizeLongSeq(1L)

println(Attribute("test", seq1).hashCode() == Attribute("test", seq2).hashCode()) // false

---

List, on the other hand, is sealed. Therefore, we can trust the universal hashcode.

[NthPortal]

we can break the hashing logic

you make a good point. that said, Seq does provide a default and correct hashCode() implementation, so an implementer has to go out of their way to be incorrect. also, to be clear, the implementation you gave is incorrect for all use cases, not just this (see scala/bug#12922 (comment) for a related discussion).

I'm not sure I agree that it needs to be sealed, but I also do see the concern. hmm. I will think on this more

But where do we build an Attribute from varargs

nowhere, actually, and it would probably be difficult to do. good shout

[NthPortal]

I've been thinking about this a lot, and I'm not convinced that bad Seq implementations are an issue worth worrying about. the Java implementation allows arbitrary implementations of j.u.List (which is an open interface), and even if we require scala.List, there's no shortage of other ways someone can deliberately implement something wrong and break everything

[iRevive]

I will try to summarize the changes.

Let's say we change the type to Seq:

object AttributeType {
-  case object StringList extends AttributeType[List[String]]
+  case object StringSeq extends AttributeType[Seq[String]]
}

Then we update the AttributeKey and KeySelect:

object AttributeKey {

  object KeySelect {
-  implicit val stringListKey: KeySelect[List[String]] =
+  implicit val stringListKey: KeySelect[Seq[String]] = 
  }
  
- def stringList(name: String): AttributeKey[List[String]] =
+ def stringSeq(name: String): AttributeKey[Seq[String]] =
}

And we can test the new API:

Attribute("test", List("value")) // does not compile
Attribute("test", Seq("value")) // compiles

// the following examples compile
val listAsSeq: Seq[String] = List("value")
val vectorAsSeq: Seq[String] = Vector("value")

Attribute("test", listAsSeq)
Attribute("test", vectorAsSeq)

// or even via type ascription
Attribute("test", LazyList("value1", "value2"): Seq[String])
Attribute("test", Queue("value"): Seq[String])

---

What do we gain from these changes?

Pros

a) We can build an array-like attribute from (nearly) any collection: Seq, Vector, List

Cons

a) Some implementation of the Seq may be problematic. For example, a hash calculation can be broken, an infinite collection can be used, etc:

Attribute("test", LazyList.continually("123"): Seq[String])

However, we can achieve the same error in the current implementation as well:

Attribute("test", LazyList.continually("123").toList)

But as you mentioned, a user can always find a way to break something :)

[NthPortal]

in terms of the compilation issue, we can probably fix it by tweaking variance.

in terms of someone using an infinite LazyList, as far as I'm concerned that's on them. this problem technically exists with all collection interface/trait types, it's just that LazyList is the most well-known example. Iterable has a single abstract method iterator, and it is trivial to create an infinite iterator to implement that using Iterator.continually.

edit: to put it differently, your argument doesn't seem specific to Attribute to me, and I'm not sure that it's worth throwing out the collections hierarchy/framework

[iRevive]

in terms of the compilation issue, we can probably fix it by tweaking variance.

I think it's fine, let's keep it as is.

So we can move forward with #512, right?

[NthPortal]

So we can move forward with #512, right?

yup, I'm gonna jump on fixing the merge conflicts right now