Properly handle AnyVals as refinement members of Selectables

[G1ng3r]

• add dynamic access to value classes field

[G1ng3r]

@prolativ Hi! Could you look into it? I'm new to compiler and not quite sure if this solution is ok or not

[prolativ]

As this used to fail at runtime, we need to have a test checking if this works at runtime. Tests in tests/pos are only checked for successful compilation. You need to move this test to tests/run/i14340.scala and make it a runnable program (e.g. by creating a @main method). Also for extra certainty that this actually works as expect you could add a test check file tests/run/i14340.check containing the expected console output of the program. In this case it would be simply

1

[G1ng3r]

Thanks! I did that and solution is not working. Will try something else

[G1ng3r]

I've found solution, but again I'm not sure about it. I think we need to wrap called members of Selectable. But it doesn't look perfect for me

[prolativ]

I have some doubts if Dynamic.scala is really the proper place for the fix. I tried to play with the issue a bit and it seems specific to reflect.Selectable rather than to Selectable in general. For sure we need more tests here to make sure we won't break something else. Consider cases like this:

• What if we need to deal with nested value classes?
• What if what we call from a Selectable should desugar to applyDynamic rather than selectDynamic?
• What if we provide a custom implementation of selectDynamic which doesn't use reflection?

An extended test case could look like this

class Foo(val value: Int) extends AnyVal
class Bar[A](val value: A) extends AnyVal

class Container1 extends reflect.Selectable

class Container2 extends Selectable:
  def selectDynamic(name: String) = Bar(name)

val cont1 = new Container1:
  val foo = new Foo(1)
  val bar = new Bar(Foo(2))
  def fooFromInt(i: Int) = new Foo(i)

val cont2 = (new Container2).asInstanceOf[Container2 { def qux: Bar[String] }]

@main def Test: Unit =
  println(cont1.foo.value)
  println(cont1.bar.value)
  println(cont1.fooFromInt(3).value)
  println(cont2.qux.value)

with expected output

1
2
3
qux

[G1ng3r]

Thanks a lot. I will think about another approach

[odersky]

LGTM. Thanks for the fix!

[prolativ]

@odersky as stated in the later comments, this doesn't seem to be fixed yet

[prolativ]

@G1ng3r do you plan to carry on with trying to fix this issue?

[G1ng3r]

@prolativ yep, I'm working on it. Stuck on last case, need some time

[prolativ]

OK, great. No pressure though. If you're having troubles with some specific issue, you can share your idea here so that we find the solution together

[G1ng3r]

@prolativ I pushed my current state, and I have a problem with cont2.qux.value. I can't find the way to distinct between cases when value class erased and when it's not. In case with cont2.qux.value compiler does not erase value class. But I also think that this case is really weird and we should not consider it. And I could not find the way to solve this issue within reflect.Selectable because it works during runtime and there is no information about value classes. I need help with last case if it should be fixed.

[prolativ]

I'll have a look at it.
Actually cont2.qux.value seems to be the only case that worked before your changes, so breaking this would be a regression - which we can't let into the compiler

[G1ng3r]

Oh, you're right. Didnt think about it. I'll try to come up with something else. Thanks

[prolativ]

I tried to dig into that a bit and here are my thoughts:

The fix should not even touch typer.Dynamic at all.

Probably the cleanest solution would be to change the implementation of selectDynamic and applyDynamic in reflect.Selectable in such a way that if the return type declared in a refinement (e.g. Foo for val foo = new Foo(1) in our test snippet) is a value class then instead of simply returning the value obtained via reflection we additionally wrap it into this class. The problem is that in the body of selectDynamic and applyDynamic we have no information about the type we're supposed to return as the cast to this type is handled later by the compiler. This is a problem I have encountered myself while working on some other stuff. It would be really nice if we could get this information somehow but that would probably require a change to how we desugar access to structural members and to what the signatures of selectDynamic and applyDynamic should look like. At this moment I don't know if this would be possible in terms of backward compatibility and how much effort it would require. @odersky any thoughts on this?

There's some chance the problem could be alternatively fixed by leaving the implementation of reflect.Selectable as it is now but adding a special case to how value classes are erased. If you have a look at VCElideAllocations.scala you'll see case ValueClassUnbox(NewWithArgs(_, List(u))) =>. Now let's look at our test case. cont1.foo.value expands to cont1().selectDynamic("foo").asInstanceOf[Foo].value(). Unfortunately the shape of the tree we get from .asInstanceOf[Foo].value in is not matched by NewWithArgs so this part doesn't get removed by the compiler, causing a runtime exception. So we might either need to add a new case for matching a cast followed by a call to the value class unwrapping method or add some extra wrapping earlier to match the existing pattern

[G1ng3r]

@prolativ Hi! I found the reason why my solution was not working for last case and I fixed it.
I've looked into VCElideAllocations.scala and I think we cant fix it there cause at this phase we are not able to box primitives.
Changing API of reflect.Selectable is beyond this issue, I guess. So I go with typer.Dynamic cause the reason of this issue is selectDynamic/applyDynamic result type, and in my opinion we can do it only here if do not change reflect.Selectable.

[prolativ]

Unfortunately this new approach doesn't work either. Just see how the test would crash if you change

def selectDynamic(name: String) = Bar(name)

to

def selectDynamic(name: String): Any = Bar(name)

(actually I would expect the return type of selectDynamic to be Any rather than something more specific in most cases).
It also looks like your approach doesn't really work well with nonreflective Selectables when some operations are chained.
Here is an extended test suite that should pass to make sure subtypes of reflect.Selectable and custom implementations of Selectable work the same way.

class Container1 extends reflect.Selectable

class Container2(values: Map[String, Any], methods: Map[String, Int => Any]) extends Selectable:
  def selectDynamic(name: String) = values(name)
  def applyDynamic(name: String)(arg: Int) = methods(name)(arg)

class Foo(val value: Int) extends AnyVal
class Bar[A](val value: A) extends AnyVal

object Helpers:
  def foo = Foo(1)
  def bar = Bar(Foo(2))
  def qux1 = Bar(new Container1 { def foo = Foo(10) })
  def qux2 = Bar(new Container2(Map("foo" -> Foo(20)), Map.empty).asInstanceOf[Container2 { def foo: Foo }])

  def cont1 = new Container1:
    def foo = Helpers.foo
    val bar = Helpers.bar
    def qux1 = Helpers.qux1
    def qux2 = Helpers.qux2
    def fooFromInt(i: Int) = Foo(i)

  def cont2values = Map(
    "foo" -> Helpers.foo,
    "bar" -> Helpers.bar,
    "qux1" -> Helpers.qux1,
    "qux2" -> Helpers.qux2
  )

  def cont2methods = Map(
    "fooFromInt" -> { (i: Int) => Foo(i) }
  )

  def cont2 = Container2(cont2values, cont2methods).asInstanceOf[Container2 {
    def foo: Foo
    def bar: Bar[Foo]
    def qux1: Bar[Container1 { def foo: Foo }]
    def qux2: Bar[Container2 { def foo: Foo }]
    def fooFromInt(i: Int): Foo
  }]

@main def Test: Unit =
  val cont1 = Helpers.cont1
  val cont2 = Helpers.cont2

  println(cont1.foo.value)
  println(cont2.foo.value)

  println(cont1.bar.value.value)
  println(cont2.bar.value.value)

  println(cont1.qux1.value.foo.value)
  println(cont2.qux1.value.foo.value)
  
  println(cont1.qux2.value.foo.value)
  println(cont2.qux2.value.foo.value)

  println(cont1.fooFromInt(100).value)
  println(cont2.fooFromInt(100).value)

Feel free to our own test cases.
Meanwhile we have to figure out how to fix the current implementation.
I still have a feeling Dynamic.scala is not what should be changed (or at least not only) as the desugaring performed here is a part of the specification of the language and changing it might break backward compatibility

[prolativ]

Just for clarity: this should print

1
1
2
2
10
10
20
20
100
100

[G1ng3r]

Thanks a lot! I'll think about another approach, and will get back :)

[prolativ]

@G1ng3r I think I've managed to slightly modify your approach to make this work - the main point is that special handling of selectDynamic and applyDynamic should be applied only for subtypes of reflect.Selectable - in other cases we should preserve the old behaviour.

@odersky do you think this workaround is acceptable? Currently I can see no other way to fix this without changing the specification of desugaring of Selectables

[dwijnand]

Seems good to me. A couple of suggestions.

[dwijnand]

Suggested change

	/** The implementations of `selectDynamic` and `applyDynamic` in `scala.reflect.SelectDynamic` have no information about the type of a value obtained via reflection before erasure
	/** The implementations of `selectDynamic` and `applyDynamic` in `scala.reflect.Selectable` both return `Any`

[dwijnand]

Suggested change

	* so the can't know if the value should be wrapped in a value class constructor call or not.
	* so they can't know if the value should be wrapped in a value class constructor call or not.

[dwijnand]

Suggested change

	val underlying = ValueClasses.valueClassUnbox(tpe.classSymbol.asClass).info.resultType.asSeenFrom(tpe, tpe.classSymbol)
	val genericUnderlying = ValueClasses.valueClassUnbox(tpe.classSymbol.asClass)
	val underlying = tpe.select(genericUnderlying).widen.resultType