Skip to content

Conversation

@xeno-by
Copy link
Contributor

@xeno-by xeno-by commented Aug 4, 2012

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 some way
of 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.set because vars cannot be overriden.

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.
@xeno-by
Copy link
Contributor Author

xeno-by commented Aug 4, 2012

The pull request depends on #1048, because both of them touch checkMemberOf, so separating them would lead to merge conflicts

@xeno-by
Copy link
Contributor Author

xeno-by commented Aug 4, 2012

review by @adriaanm or @paulp

@paulp
Copy link
Contributor

paulp commented Aug 4, 2012

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.
@xeno-by
Copy link
Contributor Author

xeno-by commented Aug 4, 2012

@paulp updated the pull request

@scala-jenkins
Copy link

@scala-jenkins
Copy link

@scala-jenkins
Copy link

@scala-jenkins
Copy link

@xeno-by
Copy link
Contributor Author

xeno-by commented Aug 4, 2012

PLS REBUILD ALL

@scala-jenkins
Copy link

@scala-jenkins
Copy link

@scala-jenkins
Copy link

@scala-jenkins
Copy link

Copy link
Contributor

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?

Copy link
Contributor Author

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

@adriaanm
Copy link
Contributor

adriaanm commented Aug 5, 2012

overlapping with #1052, btw

@xeno-by
Copy link
Contributor Author

xeno-by commented Aug 5, 2012

That's intended, as described in comments to #1052 and #1054

@xeno-by
Copy link
Contributor Author

xeno-by commented Aug 5, 2012

PLS REBUILD ALL

@scala-jenkins
Copy link

@scala-jenkins
Copy link

@scala-jenkins
Copy link

@scala-jenkins
Copy link

Copy link
Contributor

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?

Copy link
Contributor

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?

@odersky
Copy link
Contributor

odersky commented Aug 6, 2012

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.

@xeno-by
Copy link
Contributor Author

xeno-by commented Aug 6, 2012

Will be resubmitted jointly with #1048, #1052, #1054 and #1057.

@xeno-by xeno-by closed this Aug 6, 2012
@xeno-by
Copy link
Contributor Author

xeno-by commented Aug 6, 2012

Superceded by #1067

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants