Fork me on GitHub
#cljs-dev
<
2017-06-23
>
mfikes11:06:14

TCHECK-131 is not resolved, so I’ve attached a patch that reverts CLJS-2082 (dealing with minor conflicts) to https://dev.clojure.org/jira/browse/CLJS-2110

mfikes14:06:55

@dnolen Are you considering working towards a ClojureScript release today?

dnolen14:06:49

yes the spec bug is a problem but if we’re going to do that I want to fix the spec caching issues

dnolen14:06:57

so I don’t know if we’ll actually cut a release today

dnolen14:06:03

maybe Monday

mfikes14:06:23

OK. The spec caching issues is this one? https://dev.clojure.org/jira/browse/CLJS-1989

dnolen14:06:56

we need to write out spec registry info into the cache and load it when the ns loads

dnolen14:06:47

and probably want to this at least a slightly generic way so we can reuse it for something else later

mfikes14:06:51

Ahh. Clever solution. That one hadn’t crossed my mind. Otherwise it is a tough problem. 🙂

dnolen14:06:24

deeper spec integration is on the road map - so this is inline anyhow

dnolen15:06:16

tickets I’m looking at for today besides CLJS-1989

dnolen15:06:55

that’s probably it

dnolen15:06:19

this release will already be something of a whopper in terms of enhancements/fixes/changes

mfikes15:06:50

Will have to spread the word for people to pre-test 🙂

dnolen15:06:27

as far as I can tell symbol resolution for dotted symbols is special case where the symbol is just returned?

dnolen15:06:38

i.e. `foo.core

dnolen15:06:02

referring to the behavior of syntax-quote here

Alex Miller (Clojure team)15:06:48

no, syntax quote fully resolves symbols in the form based on current namespace

Alex Miller (Clojure team)15:06:06

so `foo should always yield a fully-qualified symbol, which is what fdef expects

dnolen15:06:17

but I’m talking about dotted symbol

dnolen15:06:25

you can’t resolve that

Alex Miller (Clojure team)15:06:27

`foo.core will resolve to user/foo.core or whatever

dnolen15:06:58

oh if there is a dotted symbol def

Alex Miller (Clojure team)15:06:03

ohhhh, this has different rules due to Java classes

dnolen15:06:12

actually this doesn’t work

Alex Miller (Clojure team)15:06:13

that’s treated as a Java class named foo.core

dnolen15:06:21

(defn foo.core [])

dnolen15:06:27

`foo.core -> ‘foo.core

Alex Miller (Clojure team)15:06:38

.‘s are not allowed in symbol name parts (for this reason)

dnolen15:06:46

so I’m correct in my analysis then

dnolen15:06:56

dotted symbols are just returned from syntax-quote

dnolen15:06:59

or the semantic equivalent

Alex Miller (Clojure team)15:06:00

this is on the reader reference page I believe

Alex Miller (Clojure team)15:06:40

so, is there still a question now? sorry, no context

dnolen15:06:02

yes I want to change how we interact with the reader to fix a bug

dnolen15:06:17

my assumption is that it is always safe to just return a dotted symbol w/o resolving it because of what you just said

Alex Miller (Clojure team)15:06:42

seems like that matches behavior in Clojure

Alex Miller (Clojure team)15:06:52

dotted unqualified symbol that is

Alex Miller (Clojure team)15:06:34

although I guess qualified symbols are also returned as is

dnolen15:06:02

you can’t have a dot in an alias right?

Alex Miller (Clojure team)15:06:49

user=> (alias 'a.b 'clojure.string)
nil
user=> `a.b/join
clojure.string/join

dnolen15:06:52

actually you can

dnolen15:06:10

only return dotted symbol w/o namespace unchanged

dnolen16:06:38

alright only CLJS-1989 left, might want to build & test with master now 🙂

favila16:06:45

@mfikes We've been using in production for the past week a prelease CLJS from right after the two patches I submitted, no problems. Great perf enhancements

favila16:06:04

we're not on master so there have been some commits since then we aren't testing

favila16:06:28

A gotcha we encountered is that const and goog.define are incompatible

favila16:06:10

if you use a goog define var, better not use const too because the GCC will never see the var name to replace it

mfikes16:06:42

Is that worth a JIRA, or is it just a corner case?

favila16:06:27

I don't think it's fixable really

mfikes16:06:28

Sounds like the ^:const change might break existing code

favila16:06:08

I don't know how likely this combination is

favila16:06:08

It was something like (def ^:const MY-COMPILE-TIME-CONSTANT "@define {string}" "default-value")

favila16:06:40

our build tooling would adjust MY-COMPILE-TIME-CONSTANT via :closure-defines

favila16:06:50

fix was just to remove ^:const

favila16:06:46

I don't know if this situation is even fixable, given the nature of each feature. You just need to be aware that they interact.

dnolen17:06:44

@favila yeah I’m inclined to say not actually problem

dnolen17:06:08

I don’t know why you would use these two features together - but we’ll see when it goes out

favila17:06:05

for us it was documentation

dnolen17:06:07

@favila glad to hear you’re not seeing other issues with all these changes

dnolen17:06:17

@favila yes we’ll call it out in the changes

dnolen19:06:09

@mfikes so doing this fdef thing will likely need a bootstrap change too

dnolen19:06:26

but I’m not super comfortable with that anymore 😛

mfikes19:06:28

Yeah, for caching?

mfikes19:06:35

Sure, I can dig into it

dnolen19:06:43

once I get this working I’ll push it and you can take a look

dnolen19:06:15

@mfikes while peak perf in Nashorn still leaves something to be desired, start has improved a lot

dnolen19:06:29

(just following your repro instructions in 1989)

mfikes19:06:16

I see, you are seeing the general effects of caching on Nashorn

mfikes20:06:08

Will take a gander

dnolen20:06:59

@mfikes it’s a bit tricky since this code requires cljs.spec.alpha if not already loaded if we encounter this in the analysis cache

dnolen20:06:17

@mfikes it may make sense to just make the dep explicit in bootstrapped

mfikes20:06:24

Cool, I see you tucked it in :cljs.spec/speced-vars

dnolen20:06:24

to avoid the need for dynamic require

dnolen20:06:52

that is make analyzer explicitly depend on cljs.spec.alpha in bootstrapped

dnolen20:06:14

@mfikes yeah the approach is almost the same as ::constants

mfikes20:06:18

OK, that would do it

dnolen22:06:48

@mfikes no rush if you can’t get to this today or even this weekend or whatever

dnolen22:06:56

I’ll wait for your patch so we can have a coordinated release

mfikes22:06:02

Thanks. I actually have a patch that might be very close. Hitting some strangeness when trying to use it with Planck. I’ll attach it to the ticket so you and @anmonteiro can see what I’m testing.

mfikes22:06:20

Attached it to https://dev.clojure.org/jira/browse/CLJS-2117 The strangeness I’m working through is what appears to be infinite recursion when Planck attempts to AOT compile the cljs.spec.alpha namespace. I think I’ll comment that AOT compilation out and see how the revised code behaves anyway.

mfikes22:06:53

I’m commenting out pieces of that patch to attempt to isolate what is causing the infinite recursion (which happens whenever loading cljs.spec.alpha, not just for Placnk’s AOT use case): Here is an example: https://gist.github.com/mfikes/bf030bf47b265325e95a53290558c04a

mfikes22:06:41

Aha! cljs.analyzer can’t explicitly depend on cljs.spec.alpha because that then depends on cljs.analyzer. Trying a variant that just moves that explicit dependency into cljs.js itself.

dnolen23:06:40

makes sense

dnolen23:06:22

@mfikes and then resolve should work w/o requiring in analyzer

mfikes23:06:04

Yeah. Actually making use of the exact same technique used to grab the macroexpand var

mfikes23:06:23

(We went through a few iterations until that was found to be the most robust way, for some reason.)

mfikes23:06:07

Things appear to be working. Attached a v2 patch. Only funny looking thing in the patch is that the symbol is '-speced-vars

mfikes23:06:29

(At least the symbol held in the map accessed by ns-interns*)

mfikes23:06:59

I’m going to re-test the Clojure cases, and also remove some workarounds I had in Planck for this defect and ensure the workarounds are no longer needed.

dnolen23:06:20

@mfikes why - in the actual source it is in fact _speced_vars

dnolen23:06:27

there’s no munging here on the name

mfikes23:06:10

Inside ns-interns* there is a call to demunge that I bet explains it

mfikes23:06:57

Yeah, it is because it is taking js-keys and trying to reverse them back to Clojure symbols

mfikes23:06:10

(Only a bootstrap thing, FWIW)

dnolen23:06:20

just leave a comment for that then

dnolen23:06:38

otherwise patch looks good, nice refactor to reuse most of the logic

mfikes23:06:52

Yeah 🙂 Had to reuse that @@. Sweet

dnolen23:06:06

thanks much!

mfikes23:06:17

You know things are getting very meta when you see @@

dnolen23:06:56

@mfikes heh yeah - even 2 years later bootstrapped doesn’t cease feeling a bit magical 🙂

mfikes23:06:20

@dnolen There is a comment made by Heller in CLJS-1989 regarding another atom that might be holding side effects of s/defcljs.spec/registry-ref. I wonder if that is covered by these changes or is a separate chunk of important state

dnolen23:06:41

Not important

dnolen23:06:27

@mfikes actually not right about registry-ref I thought we didn’t use it but we do for cljs.spec.test.alpha

dnolen23:06:00

but let’s deal with that after your patch

mfikes23:06:13

OK. My patch is nearly done. Running the self-host tests now.