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

[Compatibility] Add Refinement#refined_class #3094

Closed

Conversation

itarato
Copy link
Collaborator

@itarato itarato commented Jun 5, 2023

Note: Depends on #3093 (this PR requires the Module#refinements method to exist from that PR).

Source: #3039

Refinement#refined_class has been added. [Feature #12737]

@itarato itarato self-assigned this Jun 5, 2023
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jun 5, 2023
@itarato itarato added the shopify label Jun 5, 2023
@itarato itarato force-pushed the feature/PA-3039-Refinement-refined_class branch from 33e40e6 to 77e1363 Compare June 5, 2023 20:13
@@ -2584,4 +2584,13 @@ protected Object doClass(RubyClass rubyClass) {
return rubyClass.isSingleton;
}
}

@Primitive(name = "refined_class")
Copy link
Member

Choose a reason for hiding this comment

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

The Primitive name should reflect in which module/class it's defined, so it should be refinement_refined_class.
Ideally it'd be a CoreMethod though, for that we need to add RefinementNodes, which I think makes sense.
We should then move the refinement_import_methods Primitive there.

@itarato itarato force-pushed the feature/PA-3039-Refinement-refined_class branch from 16dbda8 to 4ea20cb Compare June 7, 2023 00:24
@itarato itarato marked this pull request as ready for review June 7, 2023 01:09
@itarato itarato requested a review from eregon June 7, 2023 01:09
src/main/ruby/truffleruby/core/refinement.rb Outdated Show resolved Hide resolved
test/mri/tests/ruby/test_refinement.rb Show resolved Hide resolved

return clonedRootNode.getCallTarget();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: it makes sense to have moving the class into a separate file committed separately.

@itarato itarato force-pushed the feature/PA-3039-Refinement-refined_class branch from 4ea20cb to e585022 Compare June 7, 2023 18:07
@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Jun 8, 2023
@eregon
Copy link
Member

eregon commented Jun 12, 2023

MRI tests fail with:

  1) Error:
TestRefinement#test_refined_class:
NoMethodError: undefined method `refinements' for #<Module:0x16da3f8>
    /b/b/e/main/test/mri/tests/ruby/test_refinement.rb:1839:in `test_refined_class'

@itarato
Copy link
Collaborator Author

itarato commented Jun 12, 2023

MRI tests fail with:

  1) Error:
TestRefinement#test_refined_class:
NoMethodError: undefined method `refinements' for #<Module:0x16da3f8>
    /b/b/e/main/test/mri/tests/ruby/test_refinement.rb:1839:in `test_refined_class'

@eregon Added a note to the description - this PR depends on #3093

@itarato itarato force-pushed the feature/PA-3039-Refinement-refined_class branch from e585022 to 4a53f81 Compare June 12, 2023 14:44
@eregon eregon assigned eregon and unassigned itarato Jun 21, 2023
@eregon
Copy link
Member

eregon commented Jul 3, 2023

I have rebased and pushed that to https://github.com/oracle/truffleruby/tree/feature/PA-3039-Refinement-refined_class
You can update this PR to that commit, so then when it merges it's marked as Merged and not Closed (I cannot push to Shopify/truffleruby).

@eregon
Copy link
Member

eregon commented Jul 3, 2023

The MRI test fails transiently:

  1) Failure:
TestRefinement#test_refined_class [/Users/graal/slave/e/main/test/mri/tests/ruby/test_refinement.rb:1863]:
<Integer> expected but was
<String>.

Because the order is unspecified.
TBH MRI tests are such a pain to deal with, let's focus on specs. I'll just tag this test.

graalvmbot pushed a commit that referenced this pull request Jul 3, 2023
@eregon
Copy link
Member

eregon commented Jul 4, 2023

Merged in ec4bbc5

@eregon eregon closed this Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. OCA Verified All contributors have signed the Oracle Contributor Agreement. shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants