Fork me on GitHub
#cljs-dev
<
2016-09-16
>
plexus17:09:17

I know similar stuff is being done in the closure compiler, but this seemed like an interesting vector to explore regardless

dnolen17:09:04

@plexus can you say a bit more on what that ticket is about? I’m familiar with Rollup but I don’t see how it could help ClojureScript more broadly

plexus17:09:49

This way we could package any lib that uses rollup as a closure library, so cljsjs or similar efforts don't need to deal with externs, modules can be referenced without js/

martinklepsch17:09:43

@dnolen essentially it approaches the module conversion issue at a different spot: GCC vs. Rollup

dnolen17:09:25

hrm it wasn’t my impression that Rollup was that popular yet

dnolen17:09:55

at first glance it just seems like moving the CLJSJS tradeoff (we have to curate) to Rollup (we have to curate)

martinklepsch17:09:28

It's not "the solution" but might be a neat immediate step compared to :foreign-libs. Finalizers seem relatively easy to write. Also it might increase Closure popularity in JS which we'd benefit from I guess. (re: easy to write https://gist.github.com/plexus/e3971543bce8840f2e076bbefeec5016)

plexus17:09:24

Exactly, it might help people that want to consume es2015 code today

dnolen18:09:24

@martinklepsch hrm I’m not following the later statement, are you saying rollup would allow people to consume Closure modules?

juhoteperi18:09:10

It think issue proposes that rollup would output Closure modules

martinklepsch18:09:14

@dnolen No but rollup could make it easier to produce Closure compatible code, which might be a nudge in the direction of using GCC.js

dnolen18:09:35

ok, but that seems unlikely to me 🙂

dnolen18:09:43

if this was bi-directional - maybe

dnolen18:09:10

I’m certainly not saying this is a bad idea

dnolen18:09:29

just trying to understand the scope of the impact - seems limited to me at the moment

dnolen18:09:24

is int coercion just to clamp floats?

dnolen18:09:43

ok sorry yes I see that’s the case in the tests

dnolen18:09:15

@anmonteiro somehow this one still needs to be rebased 🙂

anmonteiro18:09:17

@dnolen so yes, and it’s what has been done before

anmonteiro18:09:27

let me take care of that

anmonteiro18:09:39

@dnolen probably because of a lot of trailing whitespace in core.cljs

dnolen18:09:09

trying to go through a bunch of tickets today

anmonteiro18:09:12

oh no, I see it now, it’s the tests at the end of the file

dnolen18:09:28

if you have something you want me to look at, lemme know

anmonteiro18:09:47

@dnolen replaced attachment with rebased one

dnolen18:09:52

thanks applying

anmonteiro18:09:26

I think that covers mine

dnolen18:09:04

@anmonteiro the first one, there’s no test that destructuring actually works

dnolen18:09:13

just that it doesn’t fail

anmonteiro18:09:59

@dnolen you’re right, I’ll add some

anmonteiro18:09:56

@dnolen attached CLJS-1600-3.patch to that ticket with more tests + rebased

dnolen18:09:07

@anmonteiro for the last one 1536, does ((let [x 1] (defn x [] x))) work for you?

dnolen18:09:17

that’s the unit test case I would have expected

dnolen18:09:33

(you don’t need to invoke it right away of course)

anmonteiro18:09:53

@dnolen I can probably include that test case

anmonteiro18:09:05

trying it out now

anmonteiro18:09:12

@dnolen hrm doesn’t seem to be working

cljs.user=> (let [x 1] (defn x [] x))
#'cljs.user/x
cljs.user=> (x)
#object[cljs$user$x "function cljs$user$x(){
return cljs$user$x;
}"]

anmonteiro18:09:50

invoking x should return 1, I suppose?

dnolen18:09:16

so that’s the problem with dissoc’ing locals like that I think

anmonteiro18:09:58

@dnolen right because then the function body can’t see x = 1?

dnolen18:09:25

so this also seems like a weird edge case in Clojure

dnolen18:09:35

if there’s a local it shadows the function self-reference

anmonteiro18:09:02

@dnolen ?

Clojure 1.8.0
user=> ((let [x 1] (defn x [] x)))
1

dnolen18:09:23

((defn x [] x))

anmonteiro18:09:18

I’m not sure what to expect anymore 😄

anmonteiro18:09:13

@dnolen I can see that ((defn x [] x)) works fine

anmonteiro18:09:18

it returns the function

dnolen18:09:24

but that’s weird

dnolen18:09:38

a local shadows a later local (the self-reference)

anmonteiro18:09:51

yeah, I get the weirdness now

dnolen18:09:09

as as been said by the Racketeers

dnolen18:09:12

the top level is hopeless

anmonteiro18:09:19

maybe it deserves an issue in the Clojure JIRA and following up on the CLJS issue?

dnolen18:09:29

that will never change

dnolen18:09:42

so I’m fine with replicating the behavior

anmonteiro18:09:51

@dnolen sorry that deserved it ^

dnolen18:09:01

so I think I know why Clojure works the way that it does

dnolen18:09:13

or a hunch anyway

dnolen18:09:30

I bet defs side effect the top level, which is why the local takes precedence

dnolen18:09:46

side-effect the top level before analyzing the expression

anmonteiro18:09:11

that makes sense to me

anmonteiro18:09:22

I wonder what it means for CLJS

dnolen18:09:08

@anmonteiro so I think the only thing is that in parse ‘def case we need to swap! the compiler environment with what we know before we analyze the init-expr

anmonteiro18:09:04

@dnolen so ((let [x 1] (defn x [] x))) should definitely return 1 as in Clojure

anmonteiro18:09:13

that wasn’t very clear to me, it is now

dnolen18:09:24

it’s been doing that for almost a decade - so we’re stuck with that

dnolen19:09:37

@mfikes landed your $macros fix, looked good to me

mfikes19:09:29

@dnolen Cool. I’ll be on the lookout for any regressions 🙂

dnolen19:09:51

is this one subsumed now?

mfikes19:09:13

@dnolen I think item (1) in that ticket is probably the one case I’m aware of that the $macros fix doesn’t do anything to resolve, while item (2) may very well be an example that is subsumed. (too bad I conflated two items in one ticket). If you want you could kick it back to me for further investigation

dnolen19:09:42

hrm, then means the bug is elsewhere

dnolen19:09:58

this one might be tricky then - I suspect we may have implemented the wrong logic

dnolen19:09:26

where we always give the fn a local name even if doesn’t have one

dnolen19:09:48

(defn foo []) -> (defn foo (fn foo [])), or the moral equivalent

anmonteiro19:09:53

hrm I might have seen that somewhere, maybe in core.cljc?

dnolen19:09:34

well ’fn case in analyzer.cljc may also be a culprit

anmonteiro19:09:09

looking at that rn

dnolen19:09:39

@anmonteiro I don’t think this particular fubar is in core.cljc after a quick review

anmonteiro19:09:50

I meant looking at the analyzer

anmonteiro19:09:01

I had already ruled core.cljc out, sorry miscommunication

anmonteiro20:09:19

or so I think

dnolen20:09:57

@anmonteiro need to be careful here

dnolen20:09:05

(defn foo []) vs. (fn foo [])

dnolen20:09:18

the later should introduce a local

dnolen20:09:43

we may want to introduce meta on the symbol to distinguish the 2 cases

anmonteiro20:09:55

@dnolen might be tricky because the fn macro is imported from Clojure

dnolen20:09:40

@anmonteiro I don’t think so

dnolen20:09:48

look at line 1394

anmonteiro20:09:53

right I might just not have the full picture

dnolen20:09:08

the passed in argument name for that multimethod must be from defn

dnolen20:09:22

if there’s a local name we use that instead

dnolen20:09:33

so actually this looks simple fix!

dnolen20:09:39

we can distinguish right here

anmonteiro20:09:47

yeah that’s right because we ruled out earlier that we don’t do fn foo whenever we fn

anmonteiro20:09:09

@dnolen attached CLJS-1536-2.patch

anmonteiro20:09:34

it seems we need both fixes, the one I did before (`(dissoc env :locals)`) and this parse ‘fn* one

anmonteiro20:09:58

let me know if anything still doesn’t look quite right

dnolen20:09:38

so the implementation seems a bit strange to me (but I’m not sure what you tried to get here)

dnolen20:09:56

by strange only because the implementation doesn’t really a give a clue to the semantics

anmonteiro20:09:35

@dnolen do you mean the shadow? naming

dnolen20:09:52

shadow? and the fact that we need to dissoc locals

anmonteiro20:09:26

they solve different problems

anmonteiro20:09:34

one for the def, one for the defn

dnolen20:09:46

lets set aside your current implementation for now, so I can explain how I think it should work

dnolen20:09:50

and then you can tell me why it doesn’t 🙂

anmonteiro20:09:59

let’s do it

dnolen20:09:00

so I understand why you did it this way

dnolen20:09:04

so in my mind

dnolen20:09:35

if parse ‘fn gets name and there is not name element in the (fn* …) form

dnolen20:09:44

we do not change locals

dnolen20:09:59

only if we have a name element in (fn* …) should we change locals

dnolen20:09:03

that’s it

dnolen20:09:18

now that I’ve said it

dnolen20:09:26

the only thing that matters is the last case I’ve said here

dnolen20:09:38

the only time we should ever change locals is if we have name from (fn* …)

anmonteiro20:09:45

oh wow. now that you said it, it seems I took the long way

dnolen20:09:23

no worries, it’s some pretty weird semantics here 🙂

anmonteiro20:09:51

@dnolen it does seem that (dissoc env :locals) is still needed for the (let [x 1] (def x x)) case

anmonteiro20:09:09

but that’s a whole different case if I understand correctly

dnolen20:09:46

at the moment, yes, I think it's OK to treat that as a different case

dnolen20:09:54

the non-fn case

dnolen20:09:34

though I’m confused why you need to dissoc locals here

dnolen20:09:37

would be nice to understand that

anmonteiro21:09:56

I understood it when I worked on this a month ago. can’t seem to remember now

anmonteiro21:09:00

I’ll dig in again

anmonteiro21:09:19

for the (let [x 1] (def x x)) case, you want to get the var’s AST, but the x local is shadowing it

anmonteiro21:09:27

so resolve-var will return the local instead

anmonteiro21:09:43

so the solution is to either dissoc locals or to change resolve-var

dnolen21:09:42

I know you mean - (def x x) not (def x x)

dnolen21:09:41

@anmonteiro ok, we need a comment in var-ast about this, a couple sentences at least 🙂

dnolen21:09:01

so this approach sounds good to me

anmonteiro21:09:12

awesome, glad we’re on the same page now

anmonteiro21:09:29

or rather, that I actually understand things now 🙂

anmonteiro21:09:58

@dnolen btw, because :def-emits-var is only true for REPLs, my test case in core-test.cljs doesn’t really fail without the fix

anmonteiro21:09:14

but I have one that fails when running lein test so it’s all good

dnolen21:09:33

I run both before releasing so that’s great

anmonteiro21:09:56

@dnolen attached CLJS-1536-3.patch

anmonteiro21:09:15

thanks for all the help

dnolen21:09:22

np, thanks for the patch!

dnolen21:09:41

(I obviously didn’t remember all the details either, LOL)

anmonteiro21:09:00

well it’s a lot of nitty gritty stuff there

dnolen21:09:04

certainly is