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

Requiring visibility=PRIVATE for native initialize is error-prone #8210

Open
headius opened this issue Apr 24, 2024 · 0 comments · May be fixed by #8213
Open

Requiring visibility=PRIVATE for native initialize is error-prone #8210

headius opened this issue Apr 24, 2024 · 0 comments · May be fixed by #8213

Comments

@headius
Copy link
Member

headius commented Apr 24, 2024

In #8146 we see a side effect of not properly setting up a native #initialize method as private: DelegateClass will attempt to also wrap #initialize, breaking the delegator's constructor path and leading to ArgumentError: not delegated errors.

The issue in that case is that a third-party JRuby extension defines #initialize without any change to visibility, which effectively results in it being public. The immediate fix would be to add visibility = Visibility.PRIVATE to the @JRubyMethod annotation, but this is obviously going to be missed by people unfamiliar with the requirement that #initialize be private. I am opening this bug to consider ways to prevent that from happening in the future.

A few ideas:

  • Warn at runtime if a native #initialize is defined as public. It would be noise on startup, but immediately apparent to any extension writers what they need to do.
  • Always defined #initialize as private, regardless of the annotation.
    • We could possibly allow an exception for when it is explicitly defined as public... but I'm not sure how to detect that. The default in the annotation is already public, so we have no way to know whether it was left unspecified or explicitly set to public.
  • Enhance DelegateClass to always ignore initialize, since delegating it will almost always be wrong and broken. This would make it possible to fix immediately (with a gem release) and dodge the issue of how to handle users that make public #initialize methods.

The workaround is to manually update the visibility of any affected #initialize back to private, as shown in #8146.

@headius headius added this to the JRuby 9.4.8.0 milestone Apr 24, 2024
headius added a commit to headius/jruby that referenced this issue Apr 25, 2024
In jruby#8146 we saw that DelegateClass can be broken by
JRuby extensions that do not explicitly set their initialize
methods to Visibility.PRIVATE. This is an error-prone requirement
as detailed in jruby#8210, since there's no indication that
initialize is special and should always be private.

This patch introduces a "DEFAULT" visibility to be the default for
the annotation. When binding methods using annotation processing,
the DEFAULT visibility will translate itself to PRIVATE for the
initialize method and PUBLIC for other cases. This ensures that
initialize is always set to PRIVATE if no other visibility is
specified, avoiding the bug. If initialize is set to some other
visibility, it will keep that visibility.

Fixes jruby#8210
@headius headius modified the milestones: JRuby 9.4.8.0, JRuby 10.0.0.0 Apr 25, 2024
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 a pull request may close this issue.

1 participant