Skip to content

don't use undef_method #108

ArturD opened this Issue Mar 17, 2013 · 5 comments

3 participants

ArturD commented Mar 17, 2013

This is not a good practice:

    undef_method(*( - [:__id__, :__send__, :__class__, :__eval__,
      :object_id, :dup, :instance_eval, :inspect, :to_s,
      :class, :is_a?, :respond_to?, :respond_to_missing?]))

Stop using this practice, or at least add "kind_of?" to exception list.

Ruby RDF member

I can certainly see why adding #kind_of? to the exclusion list would be useful. But in this case (Vocabulary and Solution), we're specifically defining a class and wanting to remove methods that could conflict with variables accessed using method_missing behavior.

This use of undef_method does not seem so obscure, and I wasn't able to come up with any referenced that definitely showed it as being bad practice, do you have some references to cite? Or, are there some other egregious uses you've encountered? Granted, many of the methods defined on Object seem unlikely to be used as variables, certainly anything that includes characters that are not in the range [\w]+, in addition to the .* methods, seem safe to keep, but not trying to anticipate how variables or vocabulary terms might be named is the point of excluding anything we don't think is absolutely necessary for instances of these classes.

Another alternative would be to be explicit about exactly which methods to undefined, although this could cause random behavior if future versions of Ruby introduce new methods that aren't added to the list.

Ruby RDF member

In library code, one can't afford to be too picky about the means used to achieve the desired ends. This code is ugly in more than one sense, certainly; it's also the only way to do what needed to be done on Ruby 1.8 and 1.9 both.

Moving forward and dropping 1.8 compatibility, perhaps we might investigate BasicObject as a superior underlying mechanism here.

ArturD commented Mar 17, 2013

list of undefined methods for on my ruby 1.9.3 looks like this:

[:nil?, :===, :=~, :!~, :eql?, :hash, :<=>, :singleton_class, :clone, :initialize_dup, :initialize_clone, :taint, :tainted?, :untaint, :untrust, :untrusted?, :trust, :freeze, :frozen?, :methods, :singleton_methods, :protected_methods, :private_methods, :public_methods, :instance_variables, :instance_variable_get, :instance_variable_set, :instance_variable_defined?, :instance_of?, :kind_of?, :tap, :send, :public_send, :extend, :display, :method, :public_method, :define_singleton_method, :to_enum, :enum_for, :==, :equal?, :!, :!=, :instance_exec]

Is there any reason to undef methods with '?' or operators?

I feel that explicit list of undefined methods is better way. Ruby libraries tend to assume that those methods are avaliable (this is how it caught my attention). My solution would look like this:

undef_method(*( & [:list, :of, :methods, :to, :undef, :for, :all, :supported, :rubies])
Ruby RDF member

Well, perhaps my original thought of undefing everything that doesn't look like an operator or boolean query (?), other than those specifically enumerated will do.

As 1.9.3 also supports BasicObject, maybe the best is to create 1.8 versions of these classes and otherwise subclass BasicObject. Hopefully, we can drop all 1.8 support later this year, after 2.0 is out and stable. I feel better about being explicit, if it's just for 1.8.x support.

@gkellogg gkellogg was assigned Mar 17, 2013
Ruby RDF member

Changing in both cases to do the following:

# Undefine all superfluous instance methods:
              select {|m| m =~ /^\w+$/}.
              reject {|m| %w(object_id dup instance_eval inspect to_s class).include?(m) || m[0,2] == '__'}.

I did look at using BasicObject, but it seems much more trouble than it's worth right now.

@gkellogg gkellogg closed this Mar 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.