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

[Bugfix] Fix method visibility in some corner cases #3143

Closed

Conversation

itarato
Copy link
Collaborator

@itarato itarato commented Jun 30, 2023

We've been seeing some inconsistencies between truffle/cruby on how added method visibility is derived: #3134

Related Ruby issue: https://bugs.ruby-lang.org/issues/19749

Expectation

  • if define target == current callFrame self -> use scope visibility
  • use public visibility

This PR is fixing these cases:

Redefine private method but outside of its context (should be public)

class Foo10
	private

	def bar
		"public"
	end
end

foo10 = Foo10.new
foo10.define_singleton_method(:bar, foo10.method(:bar))

foo10.bar # No error in CRuby but fails in TruffleRuby

Redefine public method in the same context as private (should be private)

class Foo11
	def bar; end
end

foo11 = Foo11.new

class << foo11
	private
	
	define_method(:bar, Foo11.new.method(:bar))
end

foo11.bar # Fails in CRuby but no errors in TruffleRuby

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jun 30, 2023
@itarato itarato marked this pull request as ready for review July 16, 2023 10:39
@eregon eregon self-requested a review August 14, 2023 15:00
@eregon
Copy link
Member

eregon commented Aug 16, 2023

@andrykonchin had a very related fix in #3187 for issue #3181.
@andrykonchin Could you rebase this? I think the code changes can be dropped because they seem to do the same and then it's just extra specs, which is valuable.

end

it "defines the new method according to the scope when the definition context is the same" do
FooWithOneBar = Class.new do
Copy link
Member

Choose a reason for hiding this comment

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

Let's use a local variable here, I don't think this class needs to be named.
@andrykonchin Could you do that?

@andrykonchin andrykonchin self-assigned this Aug 16, 2023
@andrykonchin andrykonchin added the in-ci The PR is being tested in CI. Do not push new commits. label Aug 16, 2023
@andrykonchin
Copy link
Member

Merged in f5c5c19

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