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.

Alex Miller (Clojure team)15: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

Alex Miller (Clojure team)15: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?

Alex Miller (Clojure team)15:02:28

true, might need to be a new method then

Alex Miller (Clojure team)15:02:38

the old method is then deprecated but continues to work

ragge15:02:58

yes, I don't see any other way

Alex Miller (Clojure team)15: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

Alex Miller (Clojure team)15: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

Alex Miller (Clojure team)15:02:05

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

Alex Miller (Clojure team)15: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...

Alex Miller (Clojure team)15:02:55

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

Alex Miller (Clojure team)15: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

Alex Miller (Clojure team)15: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

Alex Miller (Clojure team)15:02:58

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

Alex Miller (Clojure team)15:02:13

and we should make choices

ragge15:02:17

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

Alex Miller (Clojure team)15: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

Alex Miller (Clojure team)15:02:20

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

Alex Miller (Clojure team)15: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

Alex Miller (Clojure team)15: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

bronsa15:02:07

IIRC it picks the first method returned by Class/getMethods

Alex Miller (Clojure team)15:02:27

"first" is the problem there

Alex Miller (Clojure team)16: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

Alex Miller (Clojure team)20: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

Alex Miller (Clojure team)20: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... 😕

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

Alex Miller (Clojure team)21: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

Alex Miller (Clojure team)21: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

Alex Miller (Clojure team)21:02:50

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

Alex Miller (Clojure team)21: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

Alex Miller (Clojure team)21: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

Alex Miller (Clojure team)21:02:17

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

Alex Miller (Clojure team)21: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

Alex Miller (Clojure team)21:02:14

agreed to both parts of that

Alex Miller (Clojure team)21:02:42

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