Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding defparser #5

Merged
merged 13 commits into from
May 1, 2023
Merged

Adding defparser #5

merged 13 commits into from
May 1, 2023

Conversation

judepayne
Copy link
Contributor

Adding a simple defparser macro that creates a parser in the pod and returns a reference to it, just like parse

@borkdude
Copy link
Contributor

Is it possible to add a test in test.clj for this?

@judepayne
Copy link
Contributor Author

tests added. Let me know if too 'fancy' but the time margin tested in the final test is very safe.
I also tweaked the defparser macro to only deliver a compile-time boost when the grammar is specified as a string rather than (str "first part of grammar" "..."). This brings it into line with how the macro is structured in instaparse itself (and is necessary to avoid the use of eval)

@borkdude
Copy link
Contributor

@judepayne Cool, I'll look at this first thing my morning

@judepayne
Copy link
Contributor Author

hold off pls. Am making further changes

@borkdude
Copy link
Contributor

Alright!

@judepayne
Copy link
Contributor Author

Hi @borkdude I'm trying to get every last bit of instaparse core out across a combination of pod.babashka.instaparse and instaparse-bb. Why should Babashka have less?? :)
I have got most of it. Stumped on one thing in the pod...

instaparse has a function to enable-tracing! and then you can call a parse with :trace true. Instaparse prints parse progress/ diagnostics to standard out (according to the docs here).

In the pod, I have a little macro to capture out

(defmacro capture-out
  "Redirects *out* into baos, a ByteArrayOutputStream, while body is evaluated.
   Any output printed by that evaluation will be collected in baos."
  [baos & body]
  `(with-open [ps#  (PrintWriter. ~baos)]
     (binding [*out* ps#]
       (let [res# ~@body]
         (flush)
         res#))))

and I use it in the -main function like so:

:invoke (let [baos (ByteArrayOutputStream.)]
      (do (try
	    (let [var (-> (get message "var")
			  read-string
			  symbol)
		  args (get message "args")
		  args (read-string args)
		  args (read-transit args)]
	      (if-let [f (lookup var)]
		(let [v  (capture-out baos (apply f args))
		      ;; _ (debug :value v :type (type v))
		      value (write-transit (serialize v))
		      reply {"value" value
			     "id" id
			     "status" ["done"]
			     "out" (.toString baos)}]
		  (write stdout reply))
		(throw (ex-info (str "Var not found: " var) {}))))
	    (catch Throwable e
	      (debug e)
	      (let [reply {"ex-message" (ex-message e)
			   "ex-data" (write-transit
				      (assoc (ex-data e)
					     :type (str (class e))))
			   "id" id
			   "status" ["done" "error"]}]
		(write stdout reply))))
	  (.reset baos)
	  (recur)))

If I modify the parse function in the pod to print something out..

(defn parse [ref & opts]
  (println "parse called")
  (let [id (::id ref)
        p (get @parsers id)]
    (-> (apply insta/parse p opts)
        mark-failure)))

I can see "parse called" in the bb test.clj script, when testing the compiled pod.
However, if I enable-tracing! and do a parse with :trace true I am not seeing the output of instaparse. Not capturing it evidently. Inside, the pod, the capture-out macro will capture the printed diagnostics of a :trace true just fine.
Looking in the instaparse code, it's just using standard println for the diagnostics.

Any tips on what I could be missing here?

@judepayne
Copy link
Contributor Author

Ah I see.
enable-tracing! dynamically reloads the instaparse.gll namespace.

   (defn enable-tracing!
     "Recompiles instaparse with tracing enabled.
      This is called implicitly the first time you invoke a parser with
      `:trace true` so usually you will not need to call this directly."
     []
     (alter-var-root #'gll/TRACE (constantly true))
     (alter-var-root #'gll/PROFILE (constantly true))
     (require 'instaparse.gll :reload))

@judepayne
Copy link
Contributor Author

Closing this while I see whether a PR to instaparse with native-image support for tracing is acceptable. Will revert with a new PR

@judepayne judepayne closed this Apr 29, 2023
@borkdude
Copy link
Contributor

Sure, it might be best to chop up your PRs in smaller parts so we can merge them individually. Thanks for your contributions!

@judepayne
Copy link
Contributor Author

Pleasure! and thanks for the advice

@judepayne judepayne reopened this Apr 30, 2023
@judepayne
Copy link
Contributor Author

Pushed a smaller commit. Feel free to review and merge @borkdude

Additional functionality: defparser macro, parser returns a reified object that implements IFn as well as a Parser protocol so now you can do:

(def p (insta/parser "S = AB*; AB = A B; A = 'a'+; B = 'b'+"))

(p "aaabbbaab")   ;; calling the parser as a function invokes parse (as per instaparse)

Additional tests. Updated Readme

@borkdude
Copy link
Contributor

@judepayne When I run bb test.clj it crashes over here. Could you maybe add the .txt file to e.g. test-resources? I think we need to run these tests in CI as well, but that's different issue/PR.

@judepayne
Copy link
Contributor Author

test-resources done. I'll have a look at circleci for this

@judepayne
Copy link
Contributor Author

Please merge the circleci PR first, then I can re-push this one and see where the problem is with test.clj... as it's working with me.

@borkdude
Copy link
Contributor

borkdude commented May 1, 2023

Done!

@borkdude
Copy link
Contributor

borkdude commented May 1, 2023

Seems to work! I'll merge.

One minor optional follow up after this, there seems to be something odd in the CircleCI build:

Error computing cache key: template: cacheKey:1:19: executing "cacheKey" at <checksum "project.clj">: error calling checksum: open /home/circleci/repo/project.clj: no such file or directory

@borkdude borkdude merged commit 6fc0569 into babashka:main May 1, 2023
@judepayne
Copy link
Contributor Author

test.clj run successfully!

@judepayne
Copy link
Contributor Author

Thanks.

Yes it's from this:

  - save_cache:
      paths:
        - ~/.m2
      key: v1-dependencies-{{ checksum "project.clj" }}

Pretty sure I didn't put that there. Assumed you did? If not, not sure how it got there - I'll remove it and then we're good :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants