Fork me on GitHub
#cljs-dev
<
2017-06-27
>
dnolen02:06:01

building 1.9.655 now

dnolen02:06:37

ok I have the mailing list post prepped but I want to post it after the release blog post (tomorrow)

mfikes10:06:31

These site PRs can be pulled https://github.com/clojure/clojurescript-site/pull/88 (Differences page, ^:const) https://github.com/clojure/clojurescript-site/pull/89 (Updates compiler warnings list)

rauh10:06:49

This isn't supposed to work, right:

IFn
  (-invoke [_ & a]
    (or p (set! p (apply clojure.core/partial f args)))
    (apply p a))

pesterhazy10:06:59

@juhoteperi have you seen this? It looks like 1.9.655 breaks reagent.

pesterhazy10:06:44

I can't claim to understand what partial-ifn does

juhoteperi10:06:54

Only with :static-fns true afaik

juhoteperi10:06:03

Work for me, and I'm not using that option

pesterhazy10:06:32

I though static-fns is enabled by default in the latest release?

rauh10:06:42

This was never supported. See and

juhoteperi10:06:56

It defaults to true with :optimizations :advanced, I haven't tested that yet

juhoteperi11:06:02

(defn partial
  "Works just like clojure.core/partial, except that it is an IFn, and
  the result can be compared with ="
  [f & args]
  (util/partial-ifn. f args nil))

pesterhazy11:06:04

what are the options? Just thinking out loud 1. find a different implementation of partial-ifn and release a patched version of reagent 2. tell people to manually set static-fns to false when using advanced optimization for now 3. modify the clojurescript compiler to allow this 4. revert the default to :static-fns true until this is fixed

juhoteperi11:06:13

The -equiv is the important part, it is used at least by reagent/wrap and partial is also part of Reagent public API.

rauh11:06:24

@pesterhazy I'm 99% sure it'll be option 1. Variadic protocols have never officially been supported in CLJ nor CLJS.

rauh11:06:51

It was just pure luck it worked as long as it did.

rauh11:06:13

FWIW, my apply patch broke this 🙂

pesterhazy11:06:17

maybe so, still many, many clojurescript projects use reagent

pesterhazy11:06:53

so whatever the merits of this change, it will effectively break a good percentage of production builds out there

juhoteperi11:06:12

static-fns true for adv. has been default for at least 2 years

juhoteperi11:06:25

As soon as we have fix for Reagent, I can do a new release

pesterhazy11:06:53

is the "ifn" part even still valid?

pesterhazy11:06:55

cljs.user=> (def prn* (partial prn :foo))
#'cljs.user/prn*
cljs.user=> (ifn? prn)
true
cljs.user=> (ifn? prn*)
true

juhoteperi11:06:39

Normal partial is Fn which means if also implements IFn, Reagent partia is IFn but not Fn

pesterhazy11:06:31

I'm guessing there was a reason for this name "partial-ifn" at some point - maybe partials didn't satisfy IFn with earlier versions of clojurescript?

pesterhazy11:06:10

@rauh, could you link to the patch you mentioned?

juhoteperi11:06:29

IFn part is not important

kommen11:06:58

@pesterhazy @juhoteperi to clarify, it used to be only broken with static-fns true, with 1.9.655 it is now also broken with static-fns false and with the defaults for opt. :none

rauh11:06:05

@juhoteperi FWIW, if you're trying to find an easy fix: You can use MetaFn and store the f and args in the meta data and specify! your IEquiv. That could work.

pesterhazy11:06:10

reagent uses a lot of magic unfortunately

juhoteperi11:06:54

Is MetaFn same as with-meta for a fn?

pesterhazy11:06:32

@kommen oh, so it affects all reagent users? that makes the problem more critical

juhoteperi11:06:06

Oh, reloaded the app and it broke, must have been running cached version

rauh11:06:16

@juhoteperi Like this:

(defn partial-fn
  [f & args]
  (let [pfn (with-meta (apply partial f args) [f args])]
    (specify! pfn
      IEquiv
      (-equiv [f other]
        (and (instance? MetaFn other)
             (= (.-meta f) (meta other)))))
    pfn))
(= (partial-fn prn "foo") (partial-fn prn "foo"))

juhoteperi11:06:37

@rauh That looks good, thanks

kommen11:06:55

@pesterhazy it looks so to me, but only for not so common use cases

juhoteperi11:06:57

I'll write some tests for this and try the change

rauh11:06:16

Or just copy MetaFn, if you don't want a cleaner solution.

pesterhazy11:06:30

@kommen not sure this is uncommon - partial-ifn is used for r/wrap as well

kommen11:06:22

@pesterhazy ok. i assumed more people would have ran into it with static-fns true

juhoteperi11:06:39

So is the original problem using variadic arity for invoke? And to properly implement IFn it would have to declare ~20 arities?

juhoteperi11:06:52

A minor problem with using meta is that it will break if user uses with-meta for partial result

rauh11:06:51

@juhoteperi You didn't support IMeta in your current impl, so that woudln't break anything. But you'd have to be careful when a user passes in a MetaFn already, then you'd have to carefully funnel the original meta data in and out. Probably better to do a new clean deftype.

juhoteperi11:06:08

Yes, doing that now

dnolen12:06:23

looking back over this now - looks like Reagent assumed variadic protocols were a thing? Which they aren’t

dnolen12:06:47

@rauh but it’s not apparent to me why apply changes would have broken this?

rauh12:06:37

It was pure luck that .call properly implemented the variadic ifn.

rauh12:06:13

Now, no more. It'll try to call the constructor of the deftype of reagent

rauh12:06:41

hence the self__ error.

dnolen12:06:32

Reagent was trying to call the constructor as a fn ???

rauh12:06:03

Once we're out of args (`next` is nil) we either do f.IFn$invoke$arity$X or f.call(f, ....)

dnolen12:06:36

still rewinding - that’s not important

dnolen12:06:48

(deftype foo ...)

dnolen12:06:01

and Reagent was relying on (.call foo ...) yes or no?

juhoteperi12:06:17

Reagent used foo. and ->foo, not sure what those compile to

dnolen12:06:19

@rauh so what do you mean by "It'll try to call the constructor …" ?

dnolen12:06:37

@juhoteperi ok then I don’t see what @rauh means wrt. to this statement

rauh12:06:44

I think that was a mistake. I'm confused now too 😄. I'm checking the generated source rihgt now to make sense of it.

mfikes12:06:02

Maybe that implementation is faster too? Hmm.

juhoteperi12:06:17

Could be, but I'm not sure how often this is really used

juhoteperi12:06:44

The old way to call partial the first time invoke was called was quite... confusing..

dnolen12:06:19

@rauh hrm I’m still confused how it ever worked

dnolen12:06:22

since it only implement arity 2

dnolen12:06:29

and how could you ever get to that?

rauh12:06:41

I think this might be it: Previously it got passed in this as the first argument:

partial_ifn.prototype.apply = (function (self__,args100343){
var self__ = this;
var self____$1 = this;
return self____$1.call.apply(self____$1,[self____$1].concat(cljs.core.aclone.call(null,args100343)));
});

dnolen12:06:16

I still don’t see how you could end up at arity 2

rauh12:06:42

The .call then went to the delegate, wich properly calls the variadic function. But some of the code doesn't use the reassigned this but instead the passed self__ parameter (first param):

var G__100344__delegate = function (self__,a){
var self____$1 = this;
var _ = self____$1;
var or__31642__auto___100345 = self__.p;
if(cljs.core.truth_(or__31642__auto___100345)){
} else {
self__.p = cljs.core.apply.call(null,cljs.core.partial,self__.f,self__.args);
}

rauh12:06:29

Which we can't provide anymore now with the new apply implementation. All this .call and .apply on the prototype is really obsolete nowadays.

rauh12:06:04

It's like 5 functions hops. Super confusing.

rauh12:06:48

FWIW, I don't get a warning when doing this. So it looks like https://dev.clojure.org/jira/browse/CLJS-1445 regressed.

mfikes12:06:53

Is CLJS-1445 only for the defprotocol itself (not its use)?

dnolen12:06:12

yes, in the protocol def

mfikes12:06:38

There is no warning for the Reagent code, AFAICT

dnolen12:06:35

we don’t check implementers because assumed that people would look at the protocol when implementing

mfikes12:06:37

👍 In Clojure, it luckily bottoms out in “Can’t define method not in interfaces”

dnolen12:06:23

@rauh I still don’t see how you can get to that delegate

dnolen12:06:35

there’s arity checking before you call that delegate

dnolen12:06:03

but I think you’re saying since we didn’t prevent & in protocol method implementation

dnolen12:06:10

the compiler would generate a variadic fn

dnolen12:06:35

@juhoteperi so can a Reagent release coincide with 1.9.655 announce?

juhoteperi12:06:10

Sure, just merged and pushed a snapshot

juhoteperi12:06:27

@pesterhazy Can you test 0.7.0-SNAPSHOT

juhoteperi12:06:21

I'll test this myself and hopefully someone else can confirm it works and then I'll do a proper release

dnolen12:06:21

@mfikes I think we should probably call this out in the post?

dnolen12:06:41

it’s not actually a thing that ever worked

mfikes12:06:45

A lot of people are going to hit this issue

dnolen12:06:47

since you could not define such a protocol

dnolen12:06:21

but libs may have done believing it was a thing for IFn specifically since covering all the arities is tedious

dnolen12:06:21

I don’t know why - but pointing out this anti-pattern will not work now that we’ve optimized invocation is important

kommen12:06:46

@juhoteperi i can test it in 15 min

juhoteperi12:06:20

Good, works for me in dev build, with adv build I hit problems with schema-tools and default field in a record

mfikes12:06:16

Drafting something along the lines of

> Note that, owing to optimizations in ClojureScript 1.9.655, incorrect code employing variadic signatures in protocol method implementations may fail. Be sure to update any code or libraries that make use of this construct.

dnolen12:06:19

@juhoteperi you mean Closure warnings for default field?

dnolen12:06:40

ok that’s not an issue, Closure compiler thing

dnolen12:06:48

what about Schema tools?

juhoteperi12:06:07

I mean the record with default field is from Schema-tools

dnolen12:06:11

@mfikes yes looks good

mfikes12:06:51

I revised it slightly to say may fail, instead of will fail.

dnolen12:06:36

@mfikes I think we should add a line that protocol implementations must match some existing signature

dnolen12:06:54

(-foo [a & b]) is impossible because you cannot declare it

mfikes12:06:12

> Note that, owing to optimizations in ClojureScript 1.9.655, incorrect code employing variadic signatures in protocol method implementations may fail. Be sure to update any code or libraries that make use of this construct so that protocol implementations match some existing signature.

dnolen12:06:21

looks good

juhoteperi12:06:19

Should Reagent be mentioned specifically?

rauh12:06:25

I think there MAY be a way to make this actually work if that's wanted.

dnolen12:06:18

I’m actually ok with supported the old busted behavior if it’s not terrible and emitting warning moving forward

dnolen12:06:31

forcing everyone to move lockstep is always a bummer

mfikes12:06:31

(We did that for the recur pattern. 🙂 )

rauh12:06:43

This code is the problem, that's the arity2 protocol, that apply now calls directly, note the .call still works fine (I was wrong entirely earlier):

partial_ifn$$.prototype.$cljs$core$IFn$_invoke$arity$2$ = function() {
 function $G__22894$$($G__22894$$) {
  var $var_args$jscomp$432$$ = null;
  if (0 < arguments.length) {
   for (var $var_args$jscomp$432$$ = 0, $G__22895__a$$ = Array(arguments.length - 0); $var_args$jscomp$432$$ < $G__22895__a$$.length;) {
    $G__22895__a$$[$var_args$jscomp$432$$] = arguments[$var_args$jscomp$432$$ + 0], ++$var_args$jscomp$432$$;
   }
   $var_args$jscomp$432$$ = new $cljs$core$IndexedSeq$$($G__22895__a$$, 0, null);
  }
  return $G__22894__delegate$$.call(this, $var_args$jscomp$432$$);
 }
 function $G__22894__delegate$$($G__22894$$) {
   ///////// PROBLEM HERE: THE __self:
  return $cljs$core$apply$cljs$0core$0IFn$0_invoke$0arity$02$$($cljs$core$apply$cljs$0core$0IFn$0_invoke$0arity$03$$($cljs$core$partial$$, self__.$f$, self__.args), $G__22894$$);
 }
 $G__22894$$.$cljs$lang$maxFixedArity$ = 0;
 $G__22894$$.$cljs$lang$applyTo$ = function($G__22894$$) {
  $G__22894$$ = $cljs$core$seq$$($G__22894$$);
  return $G__22894__delegate$$($G__22894$$);
 };
 $G__22894$$.$cljs$core$IFn$_invoke$arity$variadic$ = $G__22894__delegate$$;
 return $G__22894$$;
}();

dnolen12:06:48

@mfikes that said your note is fine as is whichever way we go

dnolen12:06:36

@rauh so I was right in my analysis 🙂

dnolen12:06:48

we get to arity 2 and the problem is __self ?

mfikes12:06:00

Hah! TCHECK-131 was fixed just now. Perhaps we can re-visit in the future 🙂

dnolen12:06:02

yeah for later

dnolen13:06:06

@juhoteperi if we can make it work for the time being no reason to call out Reagent in this post

dnolen13:06:47

1.9.655 won’t be the version though, if @rauh comes up with something, I want to include the warning patch.

rauh13:06:13

Yes, the problem is the __self, it's not set anywhere. Would mean messing with the compiler, not sure I could be of help on this one. 🙂

rauh13:06:11

@dnolen Inserting this (emitln "var self__ = this;") at line 751 on compiler.cljc should fix it

dnolen13:06:25

@rauh not going to do that 🙂

dnolen13:06:36

this should just work

dnolen13:06:58

yes it should

dnolen13:06:31

@rauh see line 763

dnolen13:06:49

we emit var self__ in the case there is a type

rauh13:06:02

Yeah I uncommented that, but taht didn't seem to fix it. Though I may have missed something.

kommen13:06:54

@juhoteperi looks good here as well

rauh13:06:04

That's the next "delegate" I believe 🙂

dnolen13:06:17

@rauh no everything is a the same level here

dnolen13:06:31

self__ will get hoisted

dnolen13:06:40

so the problem is that type is not set for some reason I think

rauh13:06:22

@dnolen I don't think it's all the same level. There is two functions being generated.

dnolen13:06:38

oh it should be then, I don’t see why the self bit needs to be inside

rauh13:06:44

The type is set for the second function that's generated. There is a self__ in there.

dnolen13:06:00

but I still don’t see how this will fix anything

dnolen13:06:50

in the generated code above you pasted I see no var self__ = ...

dnolen13:06:04

in none of the fns

rauh13:06:21

@dnolen Above was advanced optimized

rauh13:06:33

Sorry, that was confusing.

pesterhazy13:06:03

@juhoteperi trying your snapshot now

rauh13:06:51

I added

(when type
        (emitln "var self__ = this;"))
at line 752

rauh13:06:51

This feels very wrong (.cljs$core$IFn$_invoke$arity$2 (partial-ifn. "a") "b" "c" "d") 😄

rauh13:06:36

FWIW, I think the (when type (emit ...)) on line 765 is useless. It's not needed IMO

dnolen13:06:42

@juhoteperi @pesterhazy et al. please try Reagent with master

pesterhazy13:06:29

@dnolen, using the test case in https://dev.clojure.org/jira/browse/CLJS-2133: works in 1.9.562, fails in 1.9.655 and also fails in 1.9.656 (master)

pesterhazy14:06:08

655 gives Uncaught ReferenceError: self__ is not defined

pesterhazy14:06:41

656 gives

Uncaught TypeError: Cannot read property 'cljs$lang$applyTo' of undefined
    at Function.cljs.core.apply.cljs$core$IFn$_invoke$arity$2 (core.cljs:3800)
    at cljs$core$apply (core.cljs:3797)
    at reagent.impl.util.partial_ifn.G__8391__delegate (util.cljs:65)
    at reagent.impl.util.partial_ifn.G__8391 [as cljs$core$IFn$_invoke$arity$2] (util.cljs:63)
    at Function.cljs.core.apply_to_simple.cljs$core$IFn$_invoke$arity$4 (core.cljs:3781)
    at Function.cljs.core.apply_to_simple.cljs$core$IFn$_invoke$arity$3 (core.cljs:3777)
    at Function.cljs.core.apply_to_simple.cljs$core$IFn$_invoke$arity$2 (core.cljs:3771)
    at Function.cljs.core.apply.cljs$core$IFn$_invoke$arity$2 (core.cljs:3806)
    at cljs$core$apply (core.cljs:3797)
    at main.cljs:6

dnolen14:06:48

@pesterhazy k thanks that’s helpful

pesterhazy14:06:09

@dnolen, test case to reproduce https://github.com/pesterhazy/reagent-655 - you can specify the cljs compiler version like this: CLJS_VERSION=1.9.656 ./build

dnolen14:06:49

@pesterhazy if you can try master please

dnolen14:06:02

the test case passes for me now at REPL & under advanced

pesterhazy14:06:41

yup, 1.9.659 fixes it for me

pesterhazy14:06:05

magical 🙂

dnolen14:06:44

so people will get a warning and that it’ll encourage upgrading

dnolen14:06:48

but it won’t break the world

dnolen14:06:35

@mfikes @anmonteiro if you all have time to check out self host on your end that would be cool

mfikes14:06:55

Yes, Planck is passing its tests… I’ll try to see if the specific variadic thing in Reagent works

pesterhazy14:06:08

@dnolen many thanks for this deus ex machina fix

mfikes14:06:42

Is this a sufficient test?

cljs.user=> (defrecord Foo []
       #_=>  IFn
       #_=>  (-invoke [this & r]
       #_=>   (apply prn this r)))
cljs.user/Foo
cljs.user=> ((->Foo) 1 2 3)
#cljs.user.Foo{} 1 2 3
nil
I’m worried Reagent was doing something more than just variadic. But the above is in Planck.

dnolen14:06:28

@mfikes the fix has a test 🙂

mfikes14:06:42

OK will try that

mfikes14:06:12

@dnolen Looks good. Here is is in Planck build against latest master. https://gist.github.com/mfikes/3256f8a544923e7207349974274b988a Oddly, it also worked when built against 1.9.655. I included that as well in the gist (with full verbose output). I did get a

null is not an object (evaluating 'self__.f')
but only in the interim 1.9.657; ignoring that aspect.

dnolen15:06:02

building 1.9.660

dnolen15:06:31

@mfikes if you want to fix up that post would be nice to just push this out the door now

mfikes15:06:40

OK. working on that post now

rauh15:06:33

@mfikes You'd need to all the arity-2 IFn on the partial-fn object to trigger the error. The new apply will do that for you if you pass 2 arguments.

mfikes15:06:02

> Note that this release introduces a new warning for incorrect code which employs variadic signatures in protocol method implementations. Such code will continue to work with this release. Be sure to update any code or libraries that make use of this construct so that protocol implementations match some existing signature.

rauh15:06:16

It looks like the test in core currently only tests the error that was introduced in the commit like 1h ago.

dnolen15:06:19

@rauh yeah it doesn’t test the Reagent failure but it documents a bunch of things

rauh15:06:35

I predict the 2133 test will actually fail with :fn-direct-invoke.

mfikes15:06:53

@dnolen Post updated.

dnolen15:06:53

yes once you don’t go through .call

rauh15:06:54

The test currently does a .call on the object.

kommen15:06:06

@dnolen i can confirm 1.9.660 works with the reagent 0.6.2 and prints the warning in our nextjournal code base

dnolen15:06:10

anyways, that’s why don’t do stuff like that not behind a compiler flag 🙂

kommen15:06:29

thanks so much for tackling this

rauh15:06:56

So thinking of this, those users that use variadic protocols can't use :fn-direct-invoke.

mfikes15:06:54

Added this to https://github.com/clojure/clojurescript-site/pull/89

`:protocol-impl-with-variadic-method`, protocol impl employs variadic signature

dnolen15:06:05

@rauh yeah but it’s just not a problem for now

rauh15:06:59

@dnolen Agreed. It'll give users time to fix their code and then they can opt-in.

dnolen15:06:39

@mfikes ready when you are

mfikes15:06:52

I think all things are now pushed @dnolen

dnolen15:06:12

ok merged your PR

dnolen15:06:28

putting together ML posts

mfikes15:06:29

There are 2 other PRs that are of lesser importantce, but also related to this release

mfikes15:06:23

@rauh Probably water under the bridge, but here is me trying to get it to fail with 1.9.655: https://gist.github.com/mfikes/0caea23d6a40953201f2917632a1e17c

rauh15:06:24

@mfikes It's still return p.call(null,(2),(3)), you can use apply to call it with a IFn

dnolen15:06:32

@mfikes just waiting for your post to go live now so I can link to it

rauh15:06:48

(apply p [0 1])

mfikes15:06:12

@rauh Thanks. That caused it to fail properly with

Can't find variable: self__
	cljs.core/apply (cljs/core.cljs:3797:1)

mfikes15:06:10

And to be sure, I re-tried the above in Planck against master and it works (while emitting the desired warning when I define the deftype)

mfikes15:06:01

Ahh… now we are on a new day, @dnolen 🙂 The post still says 2017-06-26

mfikes15:06:28

ll put together another quick PR that you can optionally merge

dnolen15:06:34

@mfikes it will change the link so I should wait for that

rauh15:06:16

@dnolen Before building the CLJS site: I also added a PR

rauh15:06:33

We're in sync 🙂

dnolen15:06:44

All merged

mfikes15:06:38

@rauh The name of the option is :fn-invoke-direct (your PR has :fn-direct-invoke)

dnolen15:06:22

fixing here

rauh15:06:12

ok, sorry & thanks

mfikes15:06:30

🙂 I made the same mistake initally when adding it to Planck

dnolen15:06:25

@mfikes a minor thing, we should link to changes.md at the bottom of the post

dnolen15:06:40

It’s nicer to have a blog post as the official announce vs. ML posts

mfikes15:06:33

I can add a link to https://github.com/clojure/clojurescript/blob/master/changes.md if that’s what you are suggesting. Perhaps with some appropriate text.

mfikes15:06:46

For a complete list of updates in ClojureScript 1.9.660 see [Changes]().

mfikes15:06:50

^ something like that

dnolen15:06:02

with the hash it will always jump to the right place

dnolen15:06:18

Thanks everyone! 🎉 cljs

anmonteiro16:06:45

@dnolen made a small tweak to the release post, headings looked huge: https://github.com/clojure/clojurescript-site/pull/97/files

anmonteiro16:06:09

also I’m a bit late to the party but everything looks OK downstream in Lumo

juhoteperi16:06:55

I finally updated this longstanding PR about updating the Babel transform example to use Cljsjs package, and now it also has another section with the long form: https://github.com/clojure/clojurescript-site/pull/54/files

juhoteperi16:06:18

In Boot-cljs I have implemented code (not yet released) to automatically try to require namespaces referred by namespaced :preprocess values, and added a way to provide initialization code to evaluate before running Cljs compiler (as Boot-cljs runs Cljs compiler in separate classloader etc.)

juhoteperi16:06:21

Not sure what is the way to provide js-transforms / require namespace in cljsbuild / figwheel (when using lein plugin, and not from REPL)

dnolen17:06:58

@juhoteperi yeah I don’t have any answer for that other than that probably needs to be provided by a separate jar

dnolen17:06:38

I guess you could have a convention of js-transform.clj or something like that

dnolen17:06:45

and build tooling could load that stuff

dnolen17:06:21

I guess ClojureScript could consider loading these files directly for you a la data literals

juhoteperi17:06:04

Yes, if the convention is "low level enough" (doesn't depend on built tooling) I think it could make sense to implement it in ClojureScript instead of reimplementing it in every tool

juhoteperi17:06:32

I think there is no issue about this yet?

dnolen17:06:45

@juhoteperi there is not, go for it if you want to open it

favila17:06:12

anyone know how to make figwheel ignore an unrecognized config key?

dnolen17:06:17

@favila there’s a way to do that but I can never remember - @bhauman?

bhauman18:06:22

:validate-config :warn-unknown-keys, :ignore-unknown-keys or false

favila18:06:41

ah thank you

moxaj19:06:15

with 1.9.660 + :static-fns + :fn-invoke-direct, I see

(ns foo.bar)

(defn baz []
  ((if true :a :b) {}))
compiled to
// Compiled by ClojureScript 1.9.660 {:static-fns true, :fn-invoke-direct true}
goog.provide('foo.bar');
goog.require('cljs.core');
foo.bar.baz = (function foo$bar$baz(){
return new cljs.core.Keyword(null,"a","a",-2123407586)
(cljs.core.PersistentArrayMap.EMPTY);
});

//# sourceMappingURL=bar.js.map
which then fails at runtime with TypeError: (intermediate value) is not a function

moxaj19:06:05

without :fn-invoke-direct, the keyword is invoked with .call

moxaj19:06:18

@dnolen ^ am I doing something wrong here?

dnolen19:06:44

@moxaj first, it’s an experimental compiler flag

dnolen19:06:55

so come w/ zero expectations and a sense of adventure

moxaj19:06:06

i'm not here to bash anyone, just reporting 🙂

dnolen19:06:27

@moxaj I’m not saying you are - just setting expectations. And I have no idea and I don’t have time to look at.

dnolen19:06:40

I would talk to @rauh and determine if there’s something to file when he has time for it.

rauh19:06:16

Yeah I'm kinda surprised you don't do :optimize-constants true

moxaj19:06:18

I guess I should then 🙂 but the release notes state it builds upon :static-fns

moxaj19:06:28

is :optimize-constants a prerequisite as well?

rauh19:06:00

I'll have to think about it and see what's valid syntax.

moxaj19:06:17

cool, thanks!

dnolen19:06:56

@moxaj on first glance to me it should work

dnolen19:06:07

nothing like that should ever break it doesn’t matter what compiler options you apply

dnolen19:06:10

the generated JS just looks completely wrong to me

thheller19:06:55

@rauh that wouldn’t work with :optimize-constants either

thheller19:06:32

you added a special case for the HO-invoke stuff which checks for keywords but its dynamic in this case so it assumes to always get a function

thheller19:06:39

guess we need .call after all 😕

dnolen19:06:15

@moxaj ok I took some time to look at it here - the code gen here is in fact just wrong

dnolen19:06:23

@rauh :invoke is bad

rauh19:06:25

Yeah it looks like it doesn't even work with :optimize-constants. I think we just need to let the if

dnolen19:06:20

:optimize-constants is not relevant here at all

thheller19:06:22

@rauh not the issue, you didn’t account for (thing {:a 0}) where thing is a keyword (that is unknown to the analyzer, ie. dynamic)

dnolen19:06:24

code gen is just broken

dnolen19:06:43

nothing really to do with keywords either

thheller19:06:32

unless I interpret that wrong it is

rauh19:06:54

Yeah I don't know enough about the analyzier here, but I'm guess that (if x :a :b) will be :tag 'Keyword, hence the binding won't kick in.

rauh19:06:29

Should probably be (keyword? f) instead, ie. a keyword literal

dnolen19:06:57

@rauh don’t think so

dnolen19:06:07

you have a keyword case in emit :invoke

dnolen19:06:10

that should trigger here

thheller19:06:23

but we don’t know that its a keyword?

dnolen19:06:40

because we have a compiler optimization for

thheller19:06:44

((read-string ":foo") ...)

dnolen19:06:45

(if true ...)

dnolen19:06:07

@thheller you need to understand how parse-invoke works in this case (seperate from if optimization)

dnolen19:06:10

it’s subtle

dnolen19:06:05

the problem here is that we know it’s a keyword

dnolen19:06:13

and parse-invoke doesn’t rewrite

dnolen19:06:27

but downstream :invoke emission doesn’t handle this case correctly

thheller19:06:21

uhm .. AFAICT its just a case where this can’t work

thheller19:06:47

since we can’t know the type reliably in this case and the keyword? case never triggers

dnolen19:06:35

we need keep a bunch of issues seperated here 🙂

dnolen19:06:44

there are several problems not one

rauh19:06:45

As I see, there is mismatch, the emitter looks at (and (= (-> f :op) :constant) (keyword? (-> f :form))) whereas the invoke parser looks at :tag. So one of them should change. The emitter can append the IFn protocol when the tag is keyword, or we bind the expression, even if it's a keyword tag. Or am I completely off?

dnolen19:06:45

@rauh one thing, everything aside is that I think your patch is not actually safe

dnolen19:06:10

the final case simply cannot elide .call if we can’t prove it

dnolen19:06:19

but that’s what your patch does, elide it without evidence

rauh19:06:57

@dnolen I think the final case in the emitter will never be used if we properly fix the invoke parser. It'll always go to the second to case in the emitter (checking IFn).

dnolen19:06:18

maybe - but the important thing is this

thheller19:06:35

@rauh ((read-string ":foo") ...) you cannot always know the type, so there must be a fallback

rauh19:06:52

thheller: This would be bound in let by the new HO-invoke? check in the invoke parser

thheller19:06:50

(let [x (read-string ":foo")] (x …)) still falls through to the default :invoke no?

rauh19:06:33

No, that should check for the IFn protocol so it's safe.

dnolen19:06:48

every invocation must be preceded by foo.cljsInvoke ? ...)

dnolen19:06:07

it’s impossible to make naked direct invoke safe if you don’t have information

dnolen19:06:02

which means the final case should never be used

dnolen19:06:05

and we don’t actually need it

dnolen19:06:11

cause we couldn’t ever use it anyway

rauh19:06:15

If we consider (x a0) what can be x? Once we fix the parser (that possibly emits a let) we're can have: fn? js? goog?, variadic-invoke, keyword?, proto? (= (:op f) :var). It can't be an expression anymore so the last case should never happen.

thheller19:06:29

nevermind 😉

rauh19:06:10

@dnolen This: keyword? (= 'cljs.core/Keyword (ana/infer-tag env f)) in the emitter fixes it.

rauh19:06:41

One option... The other is to change the parser and emit a let.

dnolen20:06:02

@rauh yeah that’s what I was suggesting above

dnolen20:06:24

@thheller the read-string case you’re talking about is covered because that gets rewritten into a let assigning the result of the expression to a local thus it becomes a var case which we can handle

favila20:06:58

@rauh if you fix this by changing the emitter, don't forget cljs.core/Symbol (also callable but not a js-fn)

dnolen20:06:47

@favila we don’t need to account for that since we don’t account for it in parse-invoke

dnolen20:06:50

@rauh I applied that fix to master

rauh20:06:07

@dnolen Great thanks!

dnolen20:06:24

@rauh it still seems to me we should remove the final check for *fn-invoke-direct*

dnolen20:06:35

it should never get there and if it does we should emit .call

rauh20:06:14

Or trigger an error, asking people to contact us? We eventually wanna remove all those .call from the prototype IMO.

dnolen20:06:45

no interested in a error

dnolen20:06:51

better to just have a bunch of tests

dnolen20:06:12

removing call from prototype is low priority

dnolen20:06:44

for all we know people might be depending on it

rauh20:06:46

I mean it was a syntax error so it was spotted pretty quickly. IMO not to terrible given we're working with smart developers here.

dnolen20:06:58

some JS code check for call and treat it as a convention for callable

dnolen20:06:21

@rauh one thing that’s really important to understand is that there’s 6 years of ClojureScript out there

dnolen20:06:28

zero interest in breaking stuff

dnolen20:06:53

other projects are OK with that, this one not so much

rauh20:06:24

Yeah understood. Given it's 99% never used in "normal" code, we could centralize the dispatchers (which would be slightly slower) and keep everything backwards compat.

rauh20:06:42

Same for function dispatchers etc.

rauh20:06:16

But that's a pretty big project for the future. But would likely improve code size a bit.

rauh20:06:59

Could even be faster to have centralized dispatchers... Who knows. 🙂

dnolen20:06:23

it’s possible needs testing

rauh20:06:56

Gotta say: Really makes me appreciate all the work that's been put into CLJS compiler by everybody over the years. Thousands of edge cases and the tiniest things can break things at the other end of the code base. Writing a compiler is seems hard. 😆