Skip to content

Categorize more frames as :tooling #11

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Aug 20, 2023
Merged

Categorize more frames as :tooling #11

merged 6 commits into from
Aug 20, 2023

Conversation

vemv
Copy link
Member

@vemv vemv commented Aug 19, 2023

  • :tooling now intends to more broadly hide things that are commonly Clojure-internal / irrelevant to the application programmer.
    • New exhaustive list:
      • cider.*
      • clojure.core/apply
      • clojure.core/binding-conveyor-fn
      • clojure.core/eval
      • clojure.core/with-bindings
      • clojure.lang.Compiler
      • clojure.lang.RT
      • clojure.main/repl
      • nrepl.*
      • java.lang.Thread/run (if it's the root element of the stacktrace)

With this, tooling will be able to more effectively hide internals in the following UI:

image

...none of the other filters are as adequate.

Care has been taken to not hide stuff that can be considered relevant. For instance, for (/ 2 0), clojure.lang.Numbers.java is the code that ultimately runs that code, so it's good to show.

Which is to say, clojure.lang classes cannot be hidden altogether. clojure.lang.Compiler and clojure.lang.RT yes. And also a few select clojure.core functions, related to eval, fn invocation, and lazy sequence internals.

Predictably, our criterion will be refined over time, but this seems a good fresh start for now.

Cheers - V

@vemv vemv requested review from r0man and bbatsov August 19, 2023 19:14
vemv
vemv commented Aug 19, 2023
View reviewed changes
src/haystack/analyzer.clj Outdated
@@ -120,17 +120,26 @@
(flag-frame frame :repl)
frame))

(defn- tool? [frame-name last?]
(boolean (or (re-find #"^clojure\.lang\.AFn|^clojure\.lang\.RestFn|^clojure\.lang\.RT|clojure\.lang\.Compiler|^nrepl\.|^cider\.|^clojure\.core/eval|^clojure\.core/apply|^clojure\.core/with-bindings|^clojure\.core/binding-conveyor-fn|^clojure\.main/repl"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this and it will need a slight tweak because real frame-names are munged:

["clojure.main$repl$read_eval_print__9234$fn__9235/invoke"
   "clojure.main$repl$read_eval_print__9234/invoke"
   "clojure.main$repl$fn__9243/invoke"
   "clojure.main$repl/invokeStatic"
   "clojure.main$repl/doInvoke"
   "clojure.lang.RestFn/invoke"
   "nrepl.middleware.interruptible_eval$evaluate/invokeStatic"
   "nrepl.middleware.interruptible_eval$evaluate/invoke"
   "nrepl.middleware.interruptible_eval$interruptible_eval$fn__384954$fn__384969/invoke"
   "clojure.lang.AFn/run"
   "nrepl.middleware.session$session_exec$main_loop__385762$fn__385779/invoke"
   "nrepl.middleware.session$session_exec$main_loop__385762/invoke"
   "clojure.lang.AFn/run"
   "java.lang.Thread/run"
   "clojure.lang.LispReader/read"
   "clojure.lang.LispReader/read"
   "clojure.lang.LispReader/read"
   "clojure.core$read/invokeStatic"
   "clojure.core$read/invoke"
   "nrepl.middleware.interruptible_eval$evaluate$fn__384007/invoke"
   "clojure.main$repl$read_eval_print__9234$fn__9235/invoke"
   "clojure.main$repl$read_eval_print__9234/invoke"
   "clojure.main$repl$fn__9243/invoke"
   "clojure.main$repl/invokeStatic"
   "clojure.main$repl/doInvoke"
   "clojure.lang.RestFn/invoke"
   "nrepl.middleware.interruptible_eval$evaluate/invokeStatic"
   "nrepl.middleware.interruptible_eval$evaluate/invoke"
   "nrepl.middleware.interruptible_eval$interruptible_eval$fn__384954$fn__384969/invoke"
   "clojure.lang.AFn/run"
   "nrepl.middleware.session$session_exec$main_loop__385762$fn__385779/invoke"
   "nrepl.middleware.session$session_exec$main_loop__385762/invoke"
   "clojure.lang.AFn/run"
   "java.lang.Thread/run"
   "clojure.lang.Util/runtimeException"
   "clojure.lang.LispReader/interpretToken"
   "clojure.lang.LispReader/read"
   "clojure.lang.LispReader/read"
   "clojure.lang.LispReader/read"
   "clojure.core$read/invokeStatic"
   "clojure.core$read/invoke"
   "nrepl.middleware.interruptible_eval$evaluate$fn__384007/invoke"
   "clojure.main$repl$read_eval_print__9234$fn__9235/invoke"
   "clojure.main$repl$read_eval_print__9234/invoke"
   "clojure.main$repl$fn__9243/invoke"
   "clojure.main$repl/invokeStatic"
   "clojure.main$repl/doInvoke"
   "clojure.lang.RestFn/invoke"
   "nrepl.middleware.interruptible_eval$evaluate/invokeStatic"
   "nrepl.middleware.interruptible_eval$evaluate/invoke"
   "nrepl.middleware.interruptible_eval$interruptible_eval$fn__384954$fn__384969/invoke"
   "clojure.lang.AFn/run"
   "nrepl.middleware.session$session_exec$main_loop__385762$fn__385779/invoke"
   "nrepl.middleware.session$session_exec$main_loop__385762/invoke"
   "clojure.lang.AFn/run"
   "java.lang.Thread/run"
   "clojure.lang.Numbers/divide"
   "clojure.lang.Numbers/divide"
   "cider.nrepl.inlined.deps.haystack.v0v1v0.haystack.analyzer$eval1308394/invokeStatic"
   "cider.nrepl.inlined.deps.haystack.v0v1v0.haystack.analyzer$eval1308394/invoke"
   "clojure.lang.Compiler/eval"
   "clojure.lang.Compiler/eval"
   "clojure.core$eval/invokeStatic"
   "clojure.core$eval/invoke"
   "nrepl.middleware.interruptible_eval$evaluate$fn__383933$fn__383946/invoke"
   "clojure.lang.AFn/applyToHelper"
   "clojure.lang.AFn/applyTo"
   "clojure.core$apply/invokeStatic"
   "clojure.core$with_bindings_STAR_/invokeStatic"
   "clojure.core$with_bindings_STAR_/doInvoke"
   "clojure.lang.RestFn/invoke"
   "nrepl.middleware.interruptible_eval$evaluate$fn__383933/invoke"
   "clojure.main$repl$read_eval_print__9234$fn__9237/invoke"
   "clojure.main$repl$read_eval_print__9234/invoke"
   "clojure.main$repl$fn__9243/invoke"
   "clojure.main$repl/invokeStatic"
   "clojure.main$repl/doInvoke"
   "clojure.lang.RestFn/invoke"
   "nrepl.middleware.interruptible_eval$evaluate/invokeStatic"
   "nrepl.middleware.interruptible_eval$evaluate/invoke"
   "nrepl.middleware.interruptible_eval$interruptible_eval$fn__384954$fn__384969/invoke"
   "clojure.lang.AFn/run"
   "nrepl.middleware.session$session_exec$main_loop__385762$fn__385779/invoke"
   "nrepl.middleware.session$session_exec$main_loop__385762/invoke"
   "clojure.lang.AFn/run"
   "java.lang.Thread/run"]

@vemv vemv marked this pull request as draft August 20, 2023 00:22
@vemv vemv force-pushed the expand-tooling-criterion branch from e534769 to cd6e9a3 Compare August 20, 2023 00:58
@vemv
Copy link
Member Author

vemv commented Aug 20, 2023

Ready again, tested locally.

This is gloriously compact stacktrace for (/ 2 0):

image

I also enabled the cljs test suite in CI. For that I adapted a few cljs tests. I can't make much sense of them so I would appreciate a review when you have the chance.

@vemv vemv marked this pull request as ready for review August 20, 2023 01:09
bbatsov
bbatsov reviewed Aug 20, 2023
View reviewed changes
src/haystack/analyzer.clj Outdated
@@ -120,17 +120,27 @@
(flag-frame frame :repl)
frame))

(defn- tool? [frame-name last?]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not tooling-frame??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also - it might be good to explain in the docstring the current criteria for a tooling frame.

bbatsov
bbatsov reviewed Aug 20, 2023
View reviewed changes
src/haystack/analyzer.clj Outdated
@@ -120,17 +120,27 @@
(flag-frame frame :repl)
frame))

(defn- tool? [frame-name last?]
(let [demunged (repl/demunge frame-name)]
(boolean (or (re-find #"^clojure\.lang\.AFn|^clojure\.lang\.RestFn|^clojure\.lang\.RT|clojure\.lang\.Compiler|^nrepl\.|^cider\.|^clojure\.core/eval|^clojure\.core/apply|^clojure\.core/with-bindings|^clojure\.core/binding-conveyor-fn|^clojure\.main/repl"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this huge regexp can be make a defvar to reduce future diffs in the function and to make it possible for clients to add/change it.

@bbatsov
Copy link
Member

bbatsov commented Aug 20, 2023

Looks good overall, apart from my small remarks.

@vemv vemv force-pushed the expand-tooling-criterion branch from cd6e9a3 to d93e177 Compare August 20, 2023 11:13
@vemv vemv merged commit 2e864d0 into master Aug 20, 2023
@vemv vemv deleted the expand-tooling-criterion branch August 20, 2023 11:23
@vemv
Copy link
Member Author

vemv commented Aug 20, 2023

Thanks for the review!

An async eyeing from @r0man would be most welcome as well.

@vemv vemv mentioned this pull request Aug 21, 2023
Merged
@r0man
Copy link
Contributor

r0man commented Aug 22, 2023

@vemv LGTM. Thanks for this! I might be a bit slow with responding in the foreseeable future, since I'm taking holidays and traveling. But feel free to ping me. I take a look when I can.

@vemv
Copy link
Member Author

vemv commented Aug 22, 2023

Thank you, and safe travels!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbatsov bbatsov bbatsov left review comments

@r0man r0man Awaiting requested review from r0man

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vemv @bbatsov @r0man