Fork me on GitHub
#cljs-dev
<
2015-08-07
>
crisptrutski09:08:47

@dolen created my first two clojurescript issues, unsure of whether the format is acceptable

crisptrutski09:08:02

also happy to tackle either if they’re beginner friendly (don’t want to make the mistake of jumping into a deeper hole than I expected though)

dnolen11:08:39

@crisptrutski: very wary of 1399, that ticket will need more justification. 1400 needs a ClojureScript only reproducer.

crisptrutski11:08:30

ok thanks, i’ll update the tickets

crisptrutski11:08:55

for 1399 - think rejecting #”…” contents that are not valid “…” expressions is a fine outcome - just current behaviour was very confusing

dnolen11:08:54

@crisptrutski: 1399, but it’s open set, the ticket needs to describe how this can be accomplished reasonably.

dnolen12:08:03

@crisptrutski: please add reproducer inline in the ticket thanks

crisptrutski12:08:32

i’ve linked to the repo in the ticket - is there a better way?

dnolen12:08:02

@crisptrutski: just put it inline in the ticket, it’s going to be small

dnolen12:08:13

you can do code blocks with {code} … {code}

dnolen12:08:21

to be clear, in a new comment is fine

crisptrutski12:08:42

whoops, too late updated again

crisptrutski12:08:55

will use comments in future to decrease noise, sure

dnolen12:08:59

@crisptrutski: in this case fine simple_smile

dnolen12:08:33

trying that

dnolen12:08:13

@crisptrutski: 1400 vetted & assigned to you

crisptrutski12:08:15

any tips for digging in? very green to the compiler

dnolen12:08:42

@crisptrutski: ticket updated with guidance about where to add the test case

dnolen12:08:59

@crisptrutski: look at doseq in src/main/clojure/cljs/core.cljc

dnolen12:08:03

that’s the macros file

crisptrutski12:08:19

ok great - yeah looked at the macroexpansion already

dnolen12:08:27

I suspect something simple around bootstrapping

dnolen12:08:40

notice that macros there have to deal with circularity

dnolen12:08:58

Clojure macros gets redefined

dnolen12:08:17

because of redefinition we have to be a bit careful when we need the “host” macro functionality

dnolen12:08:23

or fn functionality

dnolen12:08:40

thus core/str because we’ve redefined str

dnolen12:08:16

but this gets a little more mind-bending because we do the same thing for bootstrapped ClojureScript

dnolen12:08:47

where core/str now refers to ClojureScript macros/fns because we redefining them again so that they can be compiled to JavaScript

dnolen12:08:47

we’ve had a few bugs due to incorrect usage of core/foo I would look here first.

dnolen12:08:17

the next thing that your test cases demonstrates, it appears to depend on the :context

dnolen12:08:32

every ClojureScript expression gets compiled to a AST with :context

dnolen12:08:44

this can be :expr, :statement or :return

crisptrutski12:08:25

thanks for taking the time to explain this - looks like i’m about to learn a lot

crisptrutski12:08:06

have an idea for 1399, will try put a patch together for that also

dnolen12:08:09

I’ll be working on various ClojureScript things today so feel free to ping me w/ more questions

crisptrutski12:08:16

great, thanks!

dnolen12:08:10

@crisptrutski: better to explain what you’re going to do before starting on patch so you don’t get disappointed if it gets rejected after you work on it simple_smile

crisptrutski12:08:12

i’ve only got an hour for this, so will probably get over the hump during the weekend

crisptrutski12:08:46

changing the behaviour of the regex reader macro to validate the string the same way the string form does

dnolen12:08:49

@crisptrutski: the reader isn’t a part of ClojureScript though, that’s tools.reader

crisptrutski12:08:05

ok, i was looking at cljs.reader

crisptrutski12:08:18

the macros function - is that code deprecated?

dnolen12:08:22

cljs.reader is just for EDN

crisptrutski12:08:38

ok, guess this is losing legs then

crisptrutski12:08:47

#”\/“ is perfectly legal in regular clojure

crisptrutski12:08:13

so suggestion then would just be to treat escaped slashes as regular slashes in the regex generation, like clojure does

dnolen12:08:29

@crisptrutski: ok seems reasonable, but definitely a tools.reader thing

dnolen12:08:04

but getting fixes into tools.reader is not hard

crisptrutski12:08:28

thanks for heading off my wrong turn - interested to poke at the project too so it’s a plus

dnolen12:08:25

@crisptrutski: if that’s the approach you want to take I would close 1399 with comment then open tools.reader issue and cross reference.

crisptrutski12:08:36

just to node, the behaviour of (cljs.reader/read-string "#\"\/\””) is exactly to throw on the string form

crisptrutski12:08:09

just to double check my understanding before closing 1399

crisptrutski12:08:42

clojure itself is not using tools.reader? trying to understand how only cljs is getting poisoned

crisptrutski12:08:09

guessing “\/“ is read as “\\/“ by tools.reader, but doubt clojure would demunge that

andrewmcveigh12:08:00

as far as I remember, #”\/“ => “\\/“ which is correct for clojure

crisptrutski12:08:24

well (re-find #"\/" "/“) => “/"

andrewmcveigh12:08:43

I’d guess that works due to the behaviour of java.util.regex.Pattern

andrewmcveigh12:08:07

Just a guess though

crisptrutski12:08:09

@dnodel - just tried (clojure.tools.reader/read-string "#\"\/\””), and that does throw - so the issue must be on the cljs side still

crisptrutski12:08:37

@andrewmcveigh: (re-find #”\\/" "/“) => nil

andrewmcveigh12:08:27

@crisptrutski: but your first example is not the same

andrewmcveigh12:08:47

You need to escape that backslash… (clojure.tools.reader/read-string "#\"\\/\”")

crisptrutski12:08:08

what a silly mistake - thanks for spotting it

andrewmcveigh12:08:02

The second example is looking for a literal backslash, which is obviously not there

crisptrutski12:08:25

sure - understand the behaviour of the valid regexes, just trying to see where js and jvm routes diverge for the badly formed string

crisptrutski12:08:50

so seems the regex form isn’t read by tools.reader for clj

andrewmcveigh12:08:10

Clojure proper has it’s own reader written in java

crisptrutski12:08:17

and still confused at how “`#”\\/“`" via #“\/“ results in an unescaped / in the js regex, but #”/“ emits an escaped one

crisptrutski12:08:38

sure people are tired of this noise for such a trivial edge case though - i’m shutting up simple_smile

dnolen12:08:38

@crisptrutski: look at the emission for Pattern and RegExp in cljs/compiler.cljc we do some escaping there

dnolen12:08:56

round line 230

andrewmcveigh12:08:02

Yep, was just looking at that.

crisptrutski13:08:32

ah, ok so this double escaping of slashes caught me out

crisptrutski13:08:16

#”\/“, (str (r/read-string "#\"\\/\"”)) => "\\/“ #”\\/“, (str (tools.reader/read-string "#\"\\\\/\"”)) => "\\\\/“

crisptrutski13:08:30

so that’s a smoking gun for tools.reader

crisptrutski13:08:46

if “\\\\” is one literal “\”, then what is “\\/“ - junk

crisptrutski13:08:12

and the clojurescript emitter was not written defensively around invalid regex asts