Skip to content

Add tests for <#820

Merged
jeaye merged 2 commits intojank-lang:mainfrom
dgr:dgr-lt
Nov 24, 2025
Merged

Add tests for <#820
jeaye merged 2 commits intojank-lang:mainfrom
dgr:dgr-lt

Conversation

@dgr
Copy link
Contributor

@dgr dgr commented Nov 23, 2025

Closes #119

@dgr
Copy link
Contributor Author

dgr commented Nov 23, 2025

Another CLR failure:

FAIL in (test-<) (:0)
negative tests
expected: (thrown? Exception (< "1" "2"))
  actual: nil

@E-A-Griffin, thoughts?

@E-A-Griffin
Copy link
Collaborator

ClojureCLR seems to convert strings and chars to longs for inequality operators. I peeked at the ClojureCLR source code and messed around in the repl for a bit but I think this is worth tagging @dmiller

$ cljr
Clojure core loaded in 176 milliseconds.
Clojure 1.12.2
user=> (< "1" "2")
true
user=> (< "2" "1")
false
user=> (< \1 \2)
true
user=> (< \2 \1)
false
user=> (< #"1" #"2")
Execution error (InvalidCastException) at System.Runtime.CompilerServices.CastHelpers/ChkCast_Helper (NO_FILE:0).
Unable to cast object of type 'System.Text.RegularExpressions.Regex' to type 'System.IConvertible'.
user=> (. clojure.lang.Numbers (lt "1" "2"))
true

@dmiller
Copy link
Contributor

dmiller commented Nov 24, 2025

This is an interesting problem. The root cause is that Java has a Number class and the JVM does not. I had to do a bunch of workarounds in clojure.lang.Numbers to deal with the mismatch. And this results in an overgeneralization. Where you have parameters of type Number on the JVM, I have to type them as Object. That allows many things to slip into the internal machinery. At some point they will get converted. True numeric primitives are dealt with in their own manner. For numeric types that are not primitive -- think of BigDecimal -- I call methods on the Convert class, such Convert.ToInt64. For this, the value being converted must support the System.IConvertible interface. BigDecimal does. Unfortunately, so does System.String. So if the string argument can be converted to a long, it will fly. (The actual conversion are encoded in methods such as clojure.lang.Util.ConvertoLong.)

I coded this way because (1) I didn't have the Number type to help out, (2) it gave me the extension to BigInteger and BigDecimal (both of which were written by me), and didn't tie me to a fixed set of types -- I had no idea what else might get added in. Unfortunately, it does over-generalize.

The only solution to this over-generalization is to restrict the set of types considered. This could be done by using more restricted versions of Util.ConvertToLong and kin to use in Numbers. This could also be done by directly modifying the definitions of Util.Convert.ToLong and kin. Util.ConvertToLong has something like 46 references. And some of those are to functions used in core. Either is a potentially breaking change, though to a coding practice that should not have been relied on.

What's the right solution? Live with over-generalization? Potentially break code?

@dgr
Copy link
Contributor Author

dgr commented Nov 24, 2025

@dmiller , I can't advise you one way or another as to the "right solution." My role is just to highlight where I find differences between implementations and then highlight those to the various implementers. This is an area where there are some definite differences across multiple implementations.

The CLJS guys, for instance, delegate a lot of functionality straight to the JS runtime and JS does some weird things with some parameters. When I ask questions of the CLJS devs of the form, "Do you want it to work like this?" often the answer is "We delegate to the runtime for speed and that's how JS behaves." Since these oddities are often for cases that are "odd" to begin with (e.g., negative testing with strange parameters that wouldn't otherwise make sense), we let them go as they are. And then sometimes I find something and folks say, "Oh! Yea. That's a bug." I think you have more flexibility than they do, because the CLR is not an insane runtime like the JS runtime is.

So, the call is yours to make. Just let me know and I can either conditionalize the test in the test suite to CLJR's behavior and make it pass, or I can leave it as is and you can update your side.

I would note that since this is in an area where Clojure JVM throws, it's unlikely that you have too many users that are relying on the CLJR behavior. And it goes without saying that if you are going to make a change, sooner is better than later.

@dmiller
Copy link
Contributor

dmiller commented Nov 24, 2025 via email

@dgr
Copy link
Contributor Author

dgr commented Nov 24, 2025

OK, noted.

@dgr
Copy link
Contributor Author

dgr commented Nov 24, 2025

Done.

Side question: what's the best way to set up a CLJR environment for local testing on a Mac? Ideally, I could catch these CLR differences before I create PR and let CI run.

@E-A-Griffin
Copy link
Collaborator

I'm on a mac, I'd suggest just installing dotnet via brew then installing and using cljr via dotnet tool install cljr, iirc

@jeaye
Copy link
Member

jeaye commented Nov 24, 2025

Agreed with, you, David. This feels like something which is worth documenting, but not worth breaking.

Thanks for the collaboration, folks. 🙂

@jeaye jeaye merged commit 58bc8cf into jank-lang:main Nov 24, 2025
2 checks passed
@dmiller
Copy link
Contributor

dmiller commented Nov 24, 2025 via email

@dgr dgr deleted the dgr-lt branch November 24, 2025 19:48
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/<

4 participants