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

Ruby 3.1 : fix Bigdecimal #7747

Merged
merged 22 commits into from Apr 12, 2023
Merged

Ruby 3.1 : fix Bigdecimal #7747

merged 22 commits into from Apr 12, 2023

Conversation

k77ch7
Copy link
Contributor

@k77ch7 k77ch7 commented Apr 6, 2023

This fixes some BigDecimal failures. I believe this PR will make it more compatible against MRI, though not completely.

The following error may be caused by JSON gem
https://github.com/jruby/jruby/actions/runs/4638638467/jobs/8208637958#step:8:56

When this PR is merged, I will make a PR against JSON gem.

diff --git a/java/src/json/ext/Parser.java b/java/src/json/ext/Parser.java
index ef28393..0a0995b 100644
--- a/java/src/json/ext/Parser.java
+++ b/java/src/json/ext/Parser.java
@@ -1195,7 +1195,7 @@ case 5:
             Ruby runtime = getRuntime();
             ByteList num = absSubSequence(p, new_p);
             IRubyObject numString = runtime.newString(num.toString());
-            return parser.decimalClass.callMethod(context, "new", numString);
+            return runtime.getKernel().callMethod(context, parser.decimalClass.getName(), numString);
         }

@enebo enebo added this to the JRuby 9.4.3.0 milestone Apr 6, 2023
@enebo
Copy link
Member

enebo commented Apr 6, 2023

@k77ch7 could you audit where we still use Bigdecimal.new in our internal suites. This looks like lots of failures but I think it is the same test(s) failing in each run. This is amazing too. This fixed a lot of stuff!

Copy link
Member

@kares kares left a comment

Choose a reason for hiding this comment

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

👍 nice to see so many tags now passing - great work.

@k77ch7
Copy link
Contributor Author

k77ch7 commented Apr 7, 2023

@enebo i will work to audit and fix failures.

@k77ch7 k77ch7 marked this pull request as ready for review April 7, 2023 14:43
Copy link
Member

@kares kares left a comment

Choose a reason for hiding this comment

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

👍 should be down to one failure - due the JSON ext

but I believe this will need a PR on JSON gem (and a potential gem release) before this is merged because we would not want JRuby to ship with a broken JSON (default) gem.

the decimal_class logic should try matching the C logic better and either be backwards compatible with older JRuby or the required_ruby_version should be bumped ...

@enebo
Copy link
Member

enebo commented Apr 11, 2023

@kares I wonder if we could leave Bigdecimal#new until we fix JSON with an issue open to remove it once we get that addressed? I am open to either path as I don't know how much effort it will be to get that JSON fix and a release. Leaving new in for a while longer wouldn't break us any more than we are now.

@kares
Copy link
Member

kares commented Apr 12, 2023

I wonder if we could leave Bigdecimal#new until we fix JSON with an issue open to remove it once we get that addressed? I am open to either path as I don't know how much effort it will be to get that JSON fix and a release. Leaving new in for a while longer wouldn't break us any more than we are now

that would work as well, kind of depends on what JSON accepts and how long it takes (we had some issues accepting JRuby specific changes which ended up taking way too long to even merge, that shouldn't be the case these days) ...

I think on the JSON side having the Java ext sync-up with the C logic but in a way that keeps compatibility would be best, if possible.


@k77ch7 let us know if you want to look into the JSON gem side, if not I will take a look at some point.
and if you do feel free to tag me up for review (as well).

@k77ch7
Copy link
Contributor Author

k77ch7 commented Apr 12, 2023

I agree with @enebo because of there are a lot of exclude BigDecimal tags.
so I revert commits to leave Bigdecimal#new.
I'm Interested in improving BigDecimal compatibility.
@kares I would like you to look into JSON gem.

@enebo
Copy link
Member

enebo commented Apr 12, 2023

Created #7752 to track what needs to happen with JSON+JRuby.

@enebo enebo merged commit 952b5fe into jruby:master Apr 12, 2023
42 of 47 checks passed
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.

None yet

3 participants