Skip to content

Incorrect treatment of structural type #4496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Glavo opened this issue May 9, 2018 · 5 comments
Closed

Incorrect treatment of structural type #4496

Glavo opened this issue May 9, 2018 · 5 comments

Comments

@Glavo
Copy link
Contributor

Glavo commented May 9, 2018

scala> import scala.reflect.Selectable.reflectiveSelectable 
scala> (new Object { val a: Int = 10} : {val a: Int}).a 
java.lang.NoSuchFieldException: a
	at java.lang.Class.getField(Class.java:1703)
	at scala.reflect.Selectable$.selectDynamic$extension(Selectable.scala:7)
	at scala.reflect.Selectable.selectDynamic(Selectable.scala:4)
	at rs$line$13$.<init>(rs$line$13:1)
	at rs$line$13$.<clinit>(rs$line$13)
	at rs$line$13.res9Show(rs$line$13)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at dotty.tools.repl.Rendering.valueOf(Rendering.scala:58)
	at dotty.tools.repl.Rendering.renderVal(Rendering.scala:79)
	at dotty.tools.repl.ReplDriver.displayMembers$3$$anonfun$3(ReplDriver.scala:284)
	at scala.collection.TraversableLike.$anonfun$map$1(TraversableLike.scala:234)
	at scala.collection.mutable.ResizableArray.foreach(ResizableArray.scala:59)
	at scala.collection.mutable.ResizableArray.foreach$(ResizableArray.scala:52)
	at scala.collection.mutable.ArrayBuffer.foreach(ArrayBuffer.scala:48)
	at scala.collection.TraversableLike.map(TraversableLike.scala:234)
	at scala.collection.TraversableLike.map$(TraversableLike.scala:227)
	at scala.collection.AbstractTraversable.map(Traversable.scala:104)
	at dotty.tools.repl.ReplDriver.displayMembers$7(ReplDriver.scala:284)
	at dotty.tools.repl.ReplDriver.displayDefinitions$$anonfun$3$$anonfun$2(ReplDriver.scala:311)
	at scala.Option.map(Option.scala:146)
	at dotty.tools.repl.ReplDriver.displayDefinitions$$anonfun$1(ReplDriver.scala:311)
	at dotty.tools.dotc.core.Periods.atPhase(Periods.scala:26)
	at dotty.tools.dotc.core.Phases.atPhase(Phases.scala:36)
	at dotty.tools.repl.ReplDriver.displayDefinitions(ReplDriver.scala:316)
	at dotty.tools.repl.ReplDriver.compile$$anonfun$2(ReplDriver.scala:241)
	at scala.util.Either.fold(Either.scala:188)
	at dotty.tools.repl.ReplDriver.compile(ReplDriver.scala:242)
	at dotty.tools.repl.ReplDriver.interpret(ReplDriver.scala:202)
	at dotty.tools.repl.ReplDriver.loop$1(ReplDriver.scala:150)
	at dotty.tools.repl.ReplDriver.runUntilQuit$$anonfun$1(ReplDriver.scala:154)
	at dotty.tools.repl.ReplDriver.withRedirectedOutput$$anonfun$2$$anonfun$1(ReplDriver.scala:163)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:58)
	at scala.Console$.withErr(Console.scala:192)
	at dotty.tools.repl.ReplDriver.withRedirectedOutput$$anonfun$1(ReplDriver.scala:163)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:58)
	at scala.Console$.withOut(Console.scala:163)
	at dotty.tools.repl.ReplDriver.withRedirectedOutput(ReplDriver.scala:163)
	at dotty.tools.repl.ReplDriver.runUntilQuit(ReplDriver.scala:154)
	at dotty.tools.repl.Main$.main(Main.scala:6)
	at dotty.tools.repl.Main.main(Main.scala)

scala> (new Object { def a: Int = 10} : {def a: Int}).a 
java.lang.NoSuchFieldException: a
	at java.lang.Class.getField(Class.java:1703)
	at scala.reflect.Selectable$.selectDynamic$extension(Selectable.scala:7)
	at scala.reflect.Selectable.selectDynamic(Selectable.scala:4)
	at rs$line$14$.<init>(rs$line$14:1)
	at rs$line$14$.<clinit>(rs$line$14)
	at rs$line$14.res10Show(rs$line$14)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at dotty.tools.repl.Rendering.valueOf(Rendering.scala:58)
	at dotty.tools.repl.Rendering.renderVal(Rendering.scala:79)
	at dotty.tools.repl.ReplDriver.displayMembers$3$$anonfun$3(ReplDriver.scala:284)
	at scala.collection.TraversableLike.$anonfun$map$1(TraversableLike.scala:234)
	at scala.collection.mutable.ResizableArray.foreach(ResizableArray.scala:59)
	at scala.collection.mutable.ResizableArray.foreach$(ResizableArray.scala:52)
	at scala.collection.mutable.ArrayBuffer.foreach(ArrayBuffer.scala:48)
	at scala.collection.TraversableLike.map(TraversableLike.scala:234)
	at scala.collection.TraversableLike.map$(TraversableLike.scala:227)
	at scala.collection.AbstractTraversable.map(Traversable.scala:104)
	at dotty.tools.repl.ReplDriver.displayMembers$7(ReplDriver.scala:284)
	at dotty.tools.repl.ReplDriver.displayDefinitions$$anonfun$3$$anonfun$2(ReplDriver.scala:311)
	at scala.Option.map(Option.scala:146)
	at dotty.tools.repl.ReplDriver.displayDefinitions$$anonfun$1(ReplDriver.scala:311)
	at dotty.tools.dotc.core.Periods.atPhase(Periods.scala:26)
	at dotty.tools.dotc.core.Phases.atPhase(Phases.scala:36)
	at dotty.tools.repl.ReplDriver.displayDefinitions(ReplDriver.scala:316)
	at dotty.tools.repl.ReplDriver.compile$$anonfun$2(ReplDriver.scala:241)
	at scala.util.Either.fold(Either.scala:188)
	at dotty.tools.repl.ReplDriver.compile(ReplDriver.scala:242)
	at dotty.tools.repl.ReplDriver.interpret(ReplDriver.scala:202)
	at dotty.tools.repl.ReplDriver.loop$1(ReplDriver.scala:150)
	at dotty.tools.repl.ReplDriver.runUntilQuit$$anonfun$1(ReplDriver.scala:154)
	at dotty.tools.repl.ReplDriver.withRedirectedOutput$$anonfun$2$$anonfun$1(ReplDriver.scala:163)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:58)
	at scala.Console$.withErr(Console.scala:192)
	at dotty.tools.repl.ReplDriver.withRedirectedOutput$$anonfun$1(ReplDriver.scala:163)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:58)
	at scala.Console$.withOut(Console.scala:163)
	at dotty.tools.repl.ReplDriver.withRedirectedOutput(ReplDriver.scala:163)
	at dotty.tools.repl.ReplDriver.runUntilQuit(ReplDriver.scala:154)
	at dotty.tools.repl.Main$.main(Main.scala:6)
	at dotty.tools.repl.Main.main(Main.scala)

@allanrenucci
Copy link
Contributor

Can be reproduced outside of the repl as well:

import scala.reflect.Selectable.reflectiveSelectable

object Test {
  def main(args: Array[String]): Unit = {
    println((new Object { val a: Int = 10} : {val a: Int}).a) // crash
    println((new Object { def a: Int = 10} : {def a: Int}).a) // crash
  }
}

@Blaisorblade Blaisorblade self-assigned this May 9, 2018
@Blaisorblade
Copy link
Contributor

I see two issues:

  • selectDynamic catches NoSuchFieldError (an IncompatibleClassChangeError) rather than NoSuchFieldException (a ReflectiveOperationException). Trivial to fix, and turns
scala> import scala.reflect.Selectable.reflectiveSelectable; class Foo { val a: Int = 10 }; (new Foo: {val a: Int}).a
// defined class Foo
java.lang.NoSuchFieldException: a
	at java.lang.Class.getField(Class.java:1703)
	at scala.reflect.Selectable$.selectDynamic$extension(Selectable.scala:7)
	at scala.reflect.Selectable.selectDynamic(Selectable.scala:4)
	at rs$line$1$.<init>(rs$line$1:1)

into

scala> import scala.reflect.Selectable.reflectiveSelectable; class Foo { val a: Int = 10 }; (new Foo: {val a: Int}).a
// defined class Foo
val res0: Int = 10
  • with that fix the original example fails with a different (and confusing) error. Looking at the JDK sources and decompiling the output suggests that's because the generated anonymous class is private, and that's the one introducing the field and accessor. Once I drop the import scalac produces public classes. Here instead the lambdaLift phase generates private classes.

The access modifier is fixed to private during lambdaLift, and if I crudely drop | Private that the original example works (though that's likely not the appropriate solution).

scala> import scala.reflect.Selectable.reflectiveSelectable; (new Object { val a: Int = 10} : {val a: Int}).a
java.lang.IllegalAccessException: Class scala.reflect.Selectable$ can not access a member of class rs$line$1$$anon$1 with modifiers "public"
	at sun.reflect.Reflection.ensureMemberAccess(Reflection.java:102)
	at java.lang.reflect.AccessibleObject.slowCheckMemberAccess(AccessibleObject.java:296)
	at java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:288)
	at java.lang.reflect.Method.invoke(Method.java:491)
	at scala.reflect.Selectable$.$anonfun$selectDynamicMethod$2(Selectable.scala:22)
	at scala.reflect.Selectable$.selectDynamic$extension(Selectable.scala:12)
	at scala.reflect.Selectable.selectDynamic(Selectable.scala:4)
	at rs$line$1$.<init>(rs$line$1:1)
	at rs$line$1$.<clinit>(rs$line$1)
	at rs$line$1.res0Show(rs$line$1)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
import scala.reflect.Selectable.reflectiveSelectable; (new Object { val a: Int = 10} : {val a: Int}).a

@Blaisorblade
Copy link
Contributor

FWIW the JDK code I consulted is http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/sun/reflect/Reflection.java but beware usual license warnings if you want to work on JRE stdlib alternatives.

@Blaisorblade
Copy link
Contributor

TLDR:

  • Scalac always uses setAccessible to bypass access control for reflective access.
  • Making anonymous classes public (like Scalac) would reduce non-reflective access control and still not solve the problem in enough cases. Moreover, Dotty expends some effort to make the lifted classes private if possible (see for instance discussion in Expand private members if necessary #517).

Scalac behavior

The Scala reflection likes to call setAccessible through ensureAccessible, and calls to it are generated by scalac in
https://github.com/scala/scala/blob/7cb938bb310c9d4af298894c92bac33918179eff/src/compiler/scala/tools/nsc/transform/CleanUp.scala#L92.

By calling getMethod (like us), they ensure they only access public methods.
(I suppose they must also call getField to also handle Java-defined classes, but we should test this).

Reflective field access should succeed even on private classes, at least for fields inherited from public classes

Scalac supports reflective calls even on private classes (example courtesy of @allanrenucci), probably by using the setAccessible API (this fails instead in Dotty with IllegalAccessException):

scala> import scala.language.reflectiveCalls
import scala.language.reflectiveCalls

scala>  trait Foo { def a: Int }
defined trait Foo

// This code appears clearly well-typed.
// XXX for a realistic use case imagine more indirection between the upcast and the access, as in better-files.
scala> def consumeFoo(foo: Foo) = (foo: { def a: Int }).a
consumeFoo: (foo: Foo)Int

// This code appears also clearly well-typed: Even if Bar is private, it can clearly be upcast to Foo
scala> object A { private class Bar extends Foo { def a = 42 }; def tester = consumeFoo(new Bar) }

scala> A.tester
defined class Bar
res0: Int = 42

On the better-files usecase see #1175 (comment).

Existing decisions in Dotty

@allanrenucci suggested we might not want to enable reflective access to a member first introduced in an anonymous class. What has been clearly decided is that we never infer and export structural types in interfaces (e.g. #1743), but I think this is different, and as shown above Dotty fails even accessing members introduced in accessible superclasses.

@Blaisorblade
Copy link
Contributor

Just learned that {var a: Int} is legal in Scalac and illegal in Dotty, though its desugaring is legal (but suffers from #4528).

allanrenucci added a commit that referenced this issue May 28, 2018
Fix #4496: call ensureAccessible to bypass class-level accessibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants