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

Fix arity exception msgs for Clojure 1.10, 1.11 #793

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

lread
Copy link
Contributor

@lread lread commented Aug 22, 2022

We were almost taking into account the sci.impl. fn name prefix that
shows up in Clojure 1.10+, but not quite.
Now also only applying arity conversion code to arity exception messages.

We were previously accidentally only testing against Clojure 1.9.0, so
these test failures went unnoticed.

Fixes #791

Please answer the following questions and leave the below in as part of your PR.

We were almost taking into account the `sci.impl.` fn name prefix that
shows up in Clojure 1.10+, but not quite.
Now also only applying arity conversion code to arity exception messages.

We were previously accidentally only testing against Clojure 1.9.0, so
these test failures went unnoticed.

Fixes babashka#791
@lread
Copy link
Contributor Author

lread commented Aug 22, 2022

@borkdude, I'm now curious as to why the native tests worked prior to my fix.
I think the Graal native-image build uses Clojure 1.10.3 so they should have failed?
I'll take a peek tomorrow to answer this question.

@lread lread marked this pull request as draft August 22, 2022 04:05
@lread
Copy link
Contributor Author

lread commented Aug 22, 2022

Ah. The failing tests are not run under native-image:

sci/test/sci/core_test.cljc

Lines 406 to 407 in 4981762

(deftest error-location-test
(when-not tu/native?

This is probably just due to the technically complexity of verifying them under native-image.

If I temporarily enable the tests for native-image they won't pass because they aren't verified correctly, but the behaviour is correct.

Take this test:

sci/test/sci/core_test.cljc

Lines 414 to 419 in 4981762

(tu/assert-submap {:type :sci/error, :line 1, :column 15,
:message #"Wrong number of args \(1\) passed to: user/foo"}
(try (eval* "(defn foo []) (foo 1)")
(catch #?(:clj clojure.lang.ExceptionInfo :cljs ExceptionInfo) ex
(let [d (ex-data ex)]
d))))

Before this PR, :message is generic: Wrong number of args (1) passed to: function of arity 0
After this PR, :message is, as expected, specific: Wrong number of args (1) passed to: user/foo

@lread lread marked this pull request as ready for review August 22, 2022 14:06
@borkdude borkdude merged commit f1811a6 into babashka:master Aug 22, 2022
@borkdude
Copy link
Collaborator

Thanks!

lread added a commit to lread/sci that referenced this pull request Aug 24, 2022
* master:
  Fix babashka#791: arity exception msgs for Clojure 1.10, 1.11 (babashka#793)
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.

Some arity exception message conversion tests failing under Clojure 1.10, 1.11
2 participants