-
Notifications
You must be signed in to change notification settings - Fork 3.1k
multiple fixes for java mirrors #1054
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
Conversation
In Scala there are some methods that only exist in symbol tables, but don't have corresponding method entries in Java class files. To the best of my knowledge, these methods can be subdivided into four groups: 1) stuff weaved onto Any, AnyVal and AnyRef, 2) magic methods that Scala exposes to fix Java arrays, 3) compile-time methods (such as classOf and all kinds of macros), 4) miscellaneous stuff (currently only String_+). To support these magic symbols, I've modified the `checkMemberOf` validator to special case Any/AnyVal/AnyRef methods and adjusted MethodMirror and ConstructorMirror classes to use special invokers for those instead of relying on Java reflection.
|
The pull request depends on #1048, because both of them touch |
|
It is likely we will be able to override vars in the future. (You can do it now with -Yoverride-vars.) It would not be good to bake it into the API that we can't. |
First of all in 911bbc4 I've completely overlooked the fact that reflectConstructor exists and that is also needs sanity checks. Now reflectConstructor checks that the incoming symbol is actually a ctor, and that it is actually a ctor of the class reflected by the current mirror. Secondly while fixing reflectConstructor I've found out that reflectXXX methods don't accept inherited, but overriden members. That's also fixed now. Haha that was a nice one. After a fixed the problem with inherited members, I've uncovered another issue. If we have a field overriden in a descendant, then (unlike with methods) overriding and overriden members make sense independently of each other. Hence in `FieldMirror.get` we need a way of specifying whether we want the base value or the overriding value. To do that I introduced an optional parameter. The same parameter has been added to `FieldMirror.set`. Despite the fact that Scala currently doesn't normally allow to override vars (not counting the -Yoverride-vars internal flag), I think, it's better to be forward-compatible, since it induces zero usage overhead.
|
@paulp updated the pull request |
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/65/ |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/769/ |
|
jenkins job pr-rangepos: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/65/ |
|
jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/769/ |
|
PLS REBUILD ALL |
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/68/ |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/772/ |
|
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/68/ |
|
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/772/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need the filter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true that, I don't. fixed
|
overlapping with #1052, btw |
|
PLS REBUILD ALL |
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/71/ |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/774/ |
|
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/71/ |
|
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/774/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what little it is worth, my opinion has not changed in the least that Boolean parameters in public API are awful and Booleans with defaults are worse.
get(true)
WTF is that? Why kid ourselves that everyone everywhere will write "get(preferOverriding = true)" when it's so obviously not true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Maybe two littles make enough of worth?
|
I am against having special logic in Field.get. Instead, we should steer people away from fields. Have a doc comment like: "FieldMirrors are the only way to get at private[this] vals and vars and might be useful to inspect the data of underlying Java fields. For all other uses, it's better to go through the fields accessor. In particular, there should be no need to ever access a field mirror when reflecting on just the public members of a class or trait. Note also that only accessor MethodMirrors, but not FieldMirrors will accurately reflect overriding behavior." Another note: isClassConstructor/isMixinConstructor should be combined into one method: isConstructor. You can tell which one it is by looking atthe parent. |
|
Superceded by #1067 |
First of all in 911bbc4 I've completely overlooked the fact that
reflectConstructor exists and that is also needs sanity checks.
Now reflectConstructor checks that the incoming symbol is actually a ctor,
and that it is actually a ctor of the class reflected by the current mirror.
Secondly while fixing reflectConstructor I've found out that reflectXXX methods
don't accept inherited, but overriden members. That's also fixed now.
Haha that was a nice one. After a fixed the problem with inherited members,
I've uncovered another issue. If we have a field overriden in a descendant,
then (unlike with methods) overriding and overriden members make sense
independently of each other. Hence in
FieldMirror.getwe need some wayof specifying whether we want the base value or the overriding value.
To do that I introduced an optional parameter. Luckily we don't need to change
FieldMirror.setbecause vars cannot be overriden.