Fork me on GitHub
#clojure-dev
<
2016-02-03
>
ragge15:02:45

@alexmiller: hey, thanks for your comments on CLJ-440. I had not considered AOT and compatibility.

alexmiller15:02:10

yeah, there are a few places that I am extra-sensitive about (having screwed it up myself in the past)

ragge15:02:31

@alexmiller: that seems to mean I can't actually change the current getMethods as it would return stuff older versions can't handle

alexmiller15:02:42

no, you can change it

alexmiller15:02:55

but the existing arity needs to continue working like it did

ragge15:02:17

but if it starts returning an extra method that the caller does not expect (with a mismatch between requested arity and actual arity)

ragge15:02:26

that could cause issues, no?

alexmiller15:02:28

true, might need to be a new method then

alexmiller15:02:38

the old method is then deprecated but continues to work

ragge15:02:58

yes, I don't see any other way

alexmiller15:02:46

this is why we screen patches :)

ragge15:02:06

I also have to have a think about the case when two or more methods are returned from getMethods, I'm not even convinced it will be possible to resolve in a good way tbh

ragge15:02:25

I appreciate the quick feedback on the first patch

alexmiller15:02:44

I would greatly prefer an error to ambiguity / arbitrary decision

ragge15:02:06

oh absolutely

ragge15:02:22

I guess my concern is that something that would previously resolve without error now has to be an error

ragge15:02:38

but completely unsubstantiated yet

alexmiller15:02:05

that's definitely the kind of thing that should be well-documented in the ticket

alexmiller15:02:09

but not necessarily a deal-breaker

ragge15:02:33

noted. I'm really just getting to know this part of the compiler right now...

alexmiller15:02:55

so what happens in the case of calling a java vararg (1 fixed, 1 vararg) with more than 2 params

alexmiller15:02:17

do all args >1 get put into the vararg array?

ragge15:02:53

i currently limited the patch to allowing the varargs argument to be omitted, nothing else

alexmiller15:02:11

and alternately what happens if you invoke that with an arg and a seq - does the seq go in the vararg as the first element or does it become the vararg part?

ragge15:02:19

if you want to provide the varargs, you still have to put them in an object-array

ragge15:02:25

but maybe that was not the desired outcome

alexmiller15:02:58

well, I don't think it's well-prescribed (yet). these alternatives should be mentioned in the description

alexmiller15:02:13

and we should make choices

ragge15:02:17

ok. i'll certainly have a think about the implications

alexmiller15:02:21

and prepare for Rich to overrule them :)

bronsa15:02:42

@ragge: not sure if this is relevant at all with what you're doing, but be aware that c.l.Reflector has some non-deterministic behaviour

ragge15:02:09

@bronsa: thanks, my fear is that this will hit exactly that

ragge15:02:15

@bronsa: any specific cases that come to mind (or are documented somewhere)?

bronsa15:02:38

unfortunately I don't remember the exact case and don't think I ever logged that in a ticket

bronsa15:02:46

I'll let you know if I remember

alexmiller15:02:20

you can definitely run into ambiguity finding multiple candidate methods from different ancestors at the same level of the class hierarchy

alexmiller15:02:55

unless I'm confusing that with protocol matching ambiguities

ragge15:02:05

i was going to start by reminding myself with how varargs actually work in java, and how things like overload ambiguities work there

ragge15:02:46

like void blah(String... args) and void blah(String arg1, String... args)

bronsa15:02:09

@alexmiller: yes, the issue is that in the case of protocol that ambiguity is documented, in the case of reflection it is not. even worse, most of the time it throws at compile-time, but in a few cases it just emits a reflection warning and picks a random method if more than one matches at runtime

alexmiller15:02:23

yeah, that bugs me too

alexmiller15:02:17

if it were actually picked according to some ordering criteria, that would be slightly better

bronsa15:02:37

problem is that that order varies between jvms

alexmiller15:02:46

well... then :)

bronsa15:02:07

IIRC it picks the first method returned by Class/getMethods

alexmiller15:02:27

"first" is the problem there

alexmiller16:02:24

there might be a ticket for this, don't know. I vaguely recall talking to Rich about it a long long time ago.

ragge20:02:28

@alexmiller added a brief survey of varargs in java and possible clojure options, if we want to explore more than omitting varargs

alexmiller20:02:56

I will take a look when I have a chance

ragge20:02:24

cheers, I totally realise this is not at the top of your pile simple_smile

alexmiller20:02:57

I just do a timeslice every day :)

bronsa20:02:57

wow, I didn't realise void bar(String... x) and void bar(String x, String... y) are valid in java

ragge21:02:19

yeah... 😕

alexmiller21:02:20

languages are fun :)

ghadi21:02:01

ragge: I was looking at this earlier -- the multi-match case is unfortunate. I wonder if allowing the omission of varargs only when it's unambiguous is sufficient

ragge21:02:35

@ghadi: yeah, possibly. if the scope is limited to omission only, that would simplify things. otoh, I'm almost starting to think the current way (forcing arrays) is actually clearer. void bar(String... x) is really just void bar(String[] x), except the java calling syntax... afaict

ghadi21:02:56

that's right

ghadi21:02:16

I'm sure you noticed the ticket hasn't been vetted yet.

ragge21:02:40

i just saw alexs comment and thought it looked interesting

ghadi21:02:58

Make sure to also handle the case of int[] with tests too

ragge21:02:03

haven't really looked at the reflection and host invocation stuff before

ragge21:02:52

Good point, thanks. I think it'd be good to get some feedback on the desired scope of the ticket before proceeding too much. Just aim for omitting args, or try to match what you can do in Java closer?

ghadi21:02:46

Well done with the CLJ-440 table. I'm sure richhickey would appreciate that.

ghadi21:02:14

I don't think it's in scope to make (SingleVarargMethod/m "a" "b" "c") work

ghadi21:02:26

with automatically collecting varargs

ragge21:02:43

I think it would definitely be trickier, and doesn't seem to be what the ticket was originally about.

ragge21:02:23

However, if the goal is really to reduce "source of confusion when doing interop" with regards to varargs, I think dropping varargs is probably not the only case

ragge21:02:38

that is confusing, that is

ragge21:02:02

at least if you're coming from Java

ragge21:02:15

I'm not necessarily arguing for doing that though... but seems worth considering

ragge21:02:38

just fix this one case, or tackle varargs interop in general

ghadi21:02:22

if we're scope.creepin(), it is worthwhile to see whether some of the arguments invocations would change semantics / break code

alexmiller21:02:27

to be clear, I'm asking for the scope creep at least in terms of consideration

ragge21:02:36

yep, or even work at all... that's why I'm sort of wondering if that direction is worth pursuing at all.

bronsa21:02:37

TBH I never understood why we don't handle varargs automatically

alexmiller21:02:57

it's just never been worked on, so let's do that

ragge21:02:11

but if there's some indication it's worth exploring at a "can it be done" level, I'm happy to continue down that path

ghadi21:02:17

:thumbsup:

ragge21:02:29

i might come back running though 😉

ghadi21:02:33

probably haha

ghadi21:02:56

This might lead to a positive place regarding a spec for Reflector

ghadi21:02:06

not a formal spec, but something

alexmiller21:02:50

I'd say anything in that table that is a match for what works now should be assumed still valid

alexmiller21:02:14

it might be useful to annotate those somehow (bold them or comments)

ragge21:02:33

yeah, good point, can update

bronsa21:02:54

there's an old ticket with a huge patch doing some heavy refactoring on Reflector.java and the reflect/method matching logic in Compiler.java

bronsa21:02:14

if this works turns out to be in scope for 1.9, might be worth looking into that one aswell

bronsa21:02:36

not saying that patch in particular, just that ticket

alexmiller21:02:43

to be clear, this is me trying to get something in shape that it could be considered

bronsa21:02:02

yes, that one

bronsa21:02:38

although I see from an older comment of mine that that issue might not be an issue anymore

alexmiller21:02:17

some of those look like they were split out into CLJ-666 (hrm) and CLJ-792

alexmiller21:02:07

I've spent enough time in Reflector that I'm sure there is room for refactoring improvement :)

bronsa21:02:14

I'd love to see CLJ-792 handled

bronsa21:02:01

unfortunately I think that any change to Reflector/method matching methods in Compiler will cause some existing code to break

alexmiller21:02:14

agreed to both parts of that

alexmiller21:02:42

so I'm going to focus on the visible thing that comes up pretty regularly here which is the varargs stuff