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

Comments

Projects
None yet
4 participants
@nirvdrum
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 nirvdrum added the truffle label May 6, 2016

@nirvdrum

This comment has been minimized.

Show comment
Hide comment
@nirvdrum

nirvdrum May 6, 2016

Contributor

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).

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@chrisseaton

chrisseaton May 6, 2016

Contributor

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

Contributor

chrisseaton commented May 6, 2016

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

@chrisseaton chrisseaton closed this May 6, 2016

@eregon

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon May 7, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon May 7, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon May 7, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@nirvdrum

nirvdrum May 9, 2016

Contributor

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.

Contributor

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

[Truffle] Greatly simplify the superclass mismatch checking.
* See #3858 for background.
* Fix WeakRef superclass.
@eregon

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon May 10, 2016

Member

Closed by 7ed9a36

Member

eregon commented May 10, 2016

Closed by 7ed9a36

@eregon eregon closed this 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