Skip to content

Add tests for eval#868

Merged
jeaye merged 10 commits intojank-lang:mainfrom
dgr:dgr-eval
Apr 20, 2026
Merged

Add tests for eval#868
jeaye merged 10 commits intojank-lang:mainfrom
dgr:dgr-eval

Conversation

@dgr
Copy link
Copy Markdown
Collaborator

@dgr dgr commented Apr 18, 2026

Closes #259

@dgr
Copy link
Copy Markdown
Collaborator Author

dgr commented Apr 18, 2026

Note that this does not test ClojureScript eval, but I need some feedback on the way that I handled the reader conditionalization. Specifically, ClojureScript still includes an eval symbol, so the when-var-exists macro will find it and not print SKIP - eval. Thus, the tests, if any, in the namespace will be executed. I could have conditionalized the whole deftest form but if I had done that, then there would have been no test reported in the ClojureScript output. So, instead, I conditionalized each of the testing blocks inside the deftest form. Thus, ClojureScript sees a deftest that contains no assertions. @jeaye, if you want me to do it another way, I'm happy to adjust it.

@jeaye
Copy link
Copy Markdown
Member

jeaye commented Apr 18, 2026

Thus, ClojureScript sees a deftest that contains no assertions. @jeaye, if you want me to do it another way, I'm happy to adjust it.

This is how I'd do it, too. All good.

@jeaye jeaye requested a review from E-A-Griffin April 18, 2026 20:02
@jeaye
Copy link
Copy Markdown
Member

jeaye commented Apr 18, 2026

Oh, actually, can we just have one reader conditional, as a top-level do which is nil for CLJS?

Copy link
Copy Markdown
Member

@jeaye jeaye left a comment

Choose a reason for hiding this comment

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

Thanks, Dave!

Comment thread test/clojure/core_test/eval.cljc Outdated
Comment on lines +51 to +52
(is (= 43 (eval '(let [y 43] (or false y)))))
(is (= 43 (eval '(loop [y 0] (if (= y 43) y (recur (inc y)))))))))))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note that both of these are macros and not special forms. That's not to say this is wrong, it's just not exactly testing what's commented. let* and loop* are the special forms, which would work just as well here. The macros just add destructuring support.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hm. I relied on https://clojure.org/reference/special_forms for the list of special forms. And the doc strings for both let and loop say they are special forms. For example:

user> (doc loop)
-------------------------
clojure.core/loop
  (loop [bindings*] exprs*)
([bindings & body])
Special Form
  Evaluates the exprs in a lexical context in which the symbols in
  the binding-forms are bound to their respective init-exprs or parts
  therein. Acts as a recur target.

  Please see http://clojure.org/special_forms#loop

and

user> (doc let)
-------------------------
clojure.core/let
  (let [bindings*] exprs*)
([bindings & body])
Special Form
  binding => binding-form init-expr
  binding-form => name, or destructuring-form
  destructuring-form => map-destructure-form, or seq-destructure-form

  Evaluates the exprs in a lexical context in which the symbols in
  the binding-forms are bound to their respective init-exprs or parts
  therein.

  See https://clojure.org/reference/special_forms#binding-forms for
  more information about destructuring.

  Please see http://clojure.org/special_forms#let
Spec
  args: (cat :bindings :clojure.core.specs.alpha/bindings :body (* any?))
  ret: any?

Comment thread test/clojure/core_test/eval.cljc Outdated
Comment thread test/clojure/core_test/eval.cljc Outdated
#?(:cljs nil
:default (testing "Lists, function application, and macros"
;; empty list evaluates to itself
(is (= '() (eval '())))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about an infinite seq?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

What are you trying to test with that? That it doesn't get realized?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

So, eval'ing an infinite range returns a clojure.lang.Iterate object. I'm not even sure what that is.

user> (class (eval '(range)))
clojure.lang.Iterate

What are you wanting to test with that?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Interestingly, eval'ing a fixed range returns a clojure.lang.LongRange:

user> (class (eval '(range 10)))
clojure.lang.LongRange

Copy link
Copy Markdown
Member

@jeaye jeaye Apr 20, 2026

Choose a reason for hiding this comment

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

What are you trying to test with that? That it doesn't get realized?

Yep.

So, eval'ing an infinite range returns a clojure.lang.Iterate object. I'm not even sure what that is.

That's the type of (range).

Interestingly, eval'ing a fixed range returns a clojure.lang.LongRange:

LongRange was the type that went in. The Long is because each value is a Long (i.e. 10 is a Long).

So, eval clearly doesn't realize the seq, from the behavior. That's a great test case to have.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

So, how do I test for not being realized, exactly? Is it just that I get something back at all, because if it tried to realize the infinite sequence it would wait until the full sequence was realized and finally throw and OOM exception?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Exactly. If we get it back and continue running the test, we can know that it wasn't realized.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

OK, I'll add this.

@dgr dgr marked this pull request as draft April 20, 2026 19:36
@dgr
Copy link
Copy Markdown
Collaborator Author

dgr commented Apr 20, 2026

I think this is good to go now.

@dgr dgr marked this pull request as ready for review April 20, 2026 20:09
@jeaye jeaye merged commit c55d585 into jank-lang:main Apr 20, 2026
6 checks passed
@jeaye
Copy link
Copy Markdown
Member

jeaye commented Apr 20, 2026

Great work, Dave!

@dgr dgr deleted the dgr-eval branch April 20, 2026 22:51
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.

clojure.core/eval

2 participants