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

@mfikes ok thanks

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

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

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

maybe Monday

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

yep

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

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

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

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

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

https://dev.clojure.org/jira/browse/CLJS-2109

https://dev.clojure.org/jira/browse/CLJS-2112

https://dev.clojure.org/jira/browse/CLJS-2094

that’s probably it

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

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

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

@alexmiller ^ ?

i.e. `foo.core

referring to the behavior of syntax-quote here

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

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

@alexmiller yes I know that

but I’m talking about dotted symbol

you can’t resolve that

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

oh if there is a dotted symbol def

ohhhh, this has different rules due to Java classes

actually this doesn’t work

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

in Clojure

(defn foo.core [])

`foo.core -> ‘foo.core

in REPL

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

so I’m correct in my analysis then

dotted symbols are just returned from syntax-quote

or the semantic equivalent

this is on the reader reference page I believe

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

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

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

seems like that matches behavior in Clojure

dotted unqualified symbol that is

exactly

although I guess qualified symbols are also returned as is

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

no you can

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

actually you can

so yeah

only return dotted symbol w/o namespace unchanged

yeah

thanks

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

CI spinning away successfully: https://github.com/mfikes/clojurescript/commits/master

Cool. CI is working. The error in https://travis-ci.org/mfikes/clojurescript/builds/246270269 is fixed with the patch in https://dev.clojure.org/jira/browse/CLJS-2116

@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

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

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

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

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

I don't think it's fixable really

Sounds like the ^:const change might break existing code

I don't know how likely this combination is

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

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

fix was just to remove ^:const

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.

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

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

for us it was documentation

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

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

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

but I’m not super comfortable with that anymore 😛

Yeah, for caching?

yes

Sure, I can dig into it

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

Will do

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

(just following your repro instructions in 1989)

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

@mfikes https://github.com/clojure/clojurescript/commit/f119f6596cb136ddcb4d35e76156124a331a4e8b

Will take a gander

@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

ok

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

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

to avoid the need for dynamic require

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

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

OK, that would do it

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

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

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.

ok!

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.

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

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.

makes sense

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

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

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

k cool

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

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

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.

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

there’s no munging here on the name

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

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

ah

(Only a bootstrap thing, FWIW)

just leave a comment for that then

OK

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

Yeah 🙂 Had to reuse that @@. Sweet

lol

thanks much!

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

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

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

Not important

Unused

Cool.

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

but let’s deal with that after your patch

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