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

[Truffle] remove_const doesn't completely clear a module's name #3858

Closed
nirvdrum opened this issue May 6, 2016 · 7 comments
Closed

[Truffle] remove_const doesn't completely clear a module's name #3858

nirvdrum opened this issue May 6, 2016 · 7 comments
Milestone

Comments

@nirvdrum
Copy link
Contributor

nirvdrum commented May 6, 2016

rspec-support has a JRuby-specific fix that exposes a flaw in our remove_const implementation. It basically replaces a class with a subclass by using remove_const to remove the previous definition. Normally, a class cannot subclass itself, but remove_const should make the previous definition anonymous, at which point it can be subclassed.

A reduced example is:

module RSpec
  module Support
    class MethodSignature
    end

    class MethodSignature < remove_const(:MethodSignature)
      def x
        puts "Called x"
      end
    end
  end
end

RSpec::Support::MethodSignature.new.x

In MRI, this prints "Called X". In JRuby+Truffle, this results in a TypeError with a message of ``Support': superclass mismatch for class RSpec::Support::MethodSignature`.

This is currently a blocker for many cases where rspec is used.

@nirvdrum
Copy link
Contributor Author

nirvdrum commented May 6, 2016

Just as a general note, the JRuby bug that rspec-support is attempting to workaround is #2816, which similarly affects JRuby+Truffle (probably due to shared code).

@chrisseaton
Copy link
Contributor

I think this is fixed by 066f5c2, 7a224a5, but the latter might be too specific and not really fix the problem.

@eregon
Copy link
Member

eregon commented May 7, 2016

but remove_const should make the previous definition anonymous, at which point it can be subclassed.
It does not AFAIK, but it does remove the constant so that it's not found when looking up if the class already exist or not.

@eregon
Copy link
Member

eregon commented May 7, 2016

This workaround is very confusing though as it produces twice 2 classes with the same name in the chain:
[A, A, Object, PP::ObjectMixin, Kernel, BasicObject].
There is already specs for arity and parameters of attr_ method AFAIK, we should fix those.

@eregon eregon reopened this May 7, 2016
@eregon
Copy link
Member

eregon commented May 7, 2016

The first fix of executing superclass first should be enough.
@nirvdrum Could you confirm this work, even with 7a224a5 reverted?

@nirvdrum
Copy link
Contributor Author

nirvdrum commented May 9, 2016

While having two classes with the same name in the ancestry chain is confusing, this is also what MRI does. My phrasing around the term "anonymous" was ambiguous. Benoit's follow-up is accurate.

I tried reverting 7a224a5 and the sample code outputs the same result as with the commit present. It appears safe to revert.

eregon added a commit that referenced this issue May 10, 2016
* See #3858 for background.
* Fix WeakRef superclass.
@eregon
Copy link
Member

eregon commented May 10, 2016

Closed by 7ed9a36

@eregon eregon closed this as completed May 10, 2016
@enebo enebo added this to the JRuby 9.1.1.0 milestone May 19, 2016
@eregon eregon modified the milestones: truffle-dev, JRuby 9.1.1.0 May 19, 2016
@enebo enebo added this to the Non-Release milestone Dec 7, 2017
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

No branches or pull requests

4 participants