Loading ...
Sorry, an error occurred while loading the content.

Patch: Poor use of hasOwnProperty in JSLint

Expand Messages
  • Daniel Cassidy
    Hi List, I was surprised to run JSLint on some of my code today only to see the following message: hasOwnProperty is a really bad name. Stopping, unable to
    Message 1 of 6 , Oct 20, 2008
    View Source
    • 0 Attachment
      Hi List,

      I was surprised to run JSLint on some of my code today only to see the
      following message:

      'hasOwnProperty' is a really bad name.
      Stopping, unable to continue. (26% scanned).


      While I agree that hasOwnProperty is, in many ways, a uniquely bad
      choice of name in JavaScript, it's not a terrible choice for the
      hasOwnProperty function itself, which is what I was (re-)implementing.

      Looking through the JSLint source, the reason for the message became
      immediately apparent:

      [ ...]
      function addlabel(t, type) {

      if (t === 'hasOwnProperty') {
      error("'hasOwnProperty' is a really bad name.");
      }
      [ ... ]

      // Define t in the current function in the current scope.

      if (funct.hasOwnProperty(t)) {
      [ ... ]
      }
      [ ... ]


      Clearly, "hasOwnProperty" is banned as a name solely because
      funct.hasOwnProperty(t) will fail with an exception in that case,
      hence the fact that JSLint dies with "unable to continue".

      Use of hasOwnProperty in the form obj.hasOwnProperty(p) is a bad
      practice. While apparently correct, it will fail to operate as
      expected if obj has its own property which is itself called
      "hasOwnProperty". Indeed, when using an object to map arbitrary keys
      to values, one should never rely on that object inheriting any
      methods, since the possibility of a clash between a key name and a
      method name is the very definition of "arbitrary".

      A much safer practice is this:

      {}.hasOwnProperty.call(obj, p);

      This is guaranteed to work unless some nefarious person has overridden
      Object.prototype.hasOwnProperty with something broken (in which case
      you are well and truly stuffed :)).


      I have enclosed a patch which changes all use of hasOwnProperty in
      JSLint to the safer alternative. It also removes the error message
      such that hasOwnProperty will be an acceptable choice of name.

      (This is the first time I've used Yahoo Groups and I'm not sure if
      attachments are allowed. If not, I will upload the patch somewhere
      when I get home).

      Thanks,
      Dan.


      [Non-text portions of this message have been removed]
    • Daniel Cassidy
      Hi List, On Mon, Oct 20, 2008 at 6:50 PM, Daniel Cassidy ... Patch here: http://www.danielcassidy.me.uk/misc/fulljslint-hasOwnProperty.patch Thanks, Dan.
      Message 2 of 6 , Oct 20, 2008
      View Source
      • 0 Attachment
        Hi List,

        On Mon, Oct 20, 2008 at 6:50 PM, Daniel Cassidy
        <mail@...> wrote:
        > (This is the first time I've used Yahoo Groups and I'm not sure if
        > attachments are allowed. If not, I will upload the patch somewhere
        > when I get home).

        Patch here: http://www.danielcassidy.me.uk/misc/fulljslint-hasOwnProperty.patch

        Thanks,
        Dan.
      • Douglas Crockford
        ... Why are you doing that?
        Message 3 of 6 , Oct 23, 2008
        View Source
        • 0 Attachment
          --- In jslint_com@yahoogroups.com, "Daniel Cassidy" <mail@...> wrote:
          >
          > Hi List,
          >
          > I was surprised to run JSLint on some of my code today only to see the
          > following message:
          >
          > 'hasOwnProperty' is a really bad name.
          > Stopping, unable to continue. (26% scanned).
          >
          >
          > While I agree that hasOwnProperty is, in many ways, a uniquely bad
          > choice of name in JavaScript, it's not a terrible choice for the
          > hasOwnProperty function itself, which is what I was (re-)implementing.

          Why are you doing that?
        • Daniel Cassidy
          Hi Douglas, On Thu, Oct 23, 2008 at 11:25 PM, Douglas Crockford ... Two reasons. The first reason is that I keep seeing very subtle bugs being introduced as a
          Message 4 of 6 , Oct 23, 2008
          View Source
          • 0 Attachment
            Hi Douglas,

            On Thu, Oct 23, 2008 at 11:25 PM, Douglas Crockford
            <douglas@...> wrote:
            > --- In jslint_com@yahoogroups.com, "Daniel Cassidy" <mail@...> wrote:
            >> While I agree that hasOwnProperty is, in many ways, a uniquely bad
            >> choice of name in JavaScript, it's not a terrible choice for the
            >> hasOwnProperty function itself, which is what I was (re-)implementing.
            >
            > Why are you doing that?

            Two reasons.


            The first reason is that I keep seeing very subtle bugs being
            introduced as a result of hasOwnProperty being used in the form
            obj.hasOwnProperty(p).

            Imagine that some user input is being used as a key in obj. Although
            in most scenarios it is unlikely that the user will enter
            "hasOwnProperty", it is possible, and it is absurd that this input
            should break an otherwise robust program.

            In other scenarios, the problem is worse because it is quite likely
            that the user will enter "hasOwnProperty". For example this is the
            case if the user's input is a JavaScript program. Two examples of
            JavaScript programs that have this problem in practice are JsDoc
            Toolkit and JSLint (although the latter handles the problem with more
            grace than the former, which simply crashes with an unhelpful error
            message).

            To discourage my colleagues from making this error, I wrote a helper
            function roughly as follows:

            (function () {
            var hasOwnProperty; // JSLint chokes on this line
            if (Object.prototype.hasOwnProperty === undefined) {
            hasOwnProperty = function (object, key) {
            return object[key] !== undefined &&
            object[key] !== object.prototype[key];
            };
            } else {
            hasOwnProperty = function (object, key) {
            return {}.hasOwnProperty.call(object, key);
            };
            }
            COMPANY_NAME.utils.hasOwnProperty = hasOwnProperty;
            })();

            By simply instructing my colleagues to call
            COMPANY_NAME.utils.hasOwnProperty(obj, key) instead of
            obj.hasOwnProperty(key), we avoid instances of this bug in our own
            code.

            JSLint is rejecting the above snippet because of the variable named
            "hasOwnProperty". I don't think this is because of any bad practice in
            my code. Rather, JSLint is rejecting it in order to hide a bad
            practice in its own code -- ironically an instance of the very same
            bad practice I was trying to avoid in the first place.


            The second reason is evidenced in the code snippet above. As you know,
            some browsers don't provide a hasOwnProperty function so it's wise to
            implement your own.


            Thanks,
            Dan.
          • Douglas Crockford
            ... I don t want to modify JSLint to satisfy a single, unnecessary, dubious use. I recommend that you change your variable name. And if you are trying to
            Message 5 of 6 , Oct 23, 2008
            View Source
            • 0 Attachment
              --- In jslint_com@yahoogroups.com, "Daniel Cassidy" <mail@...> wrote:
              > Two reasons.
              >
              >
              > The first reason is that I keep seeing very subtle bugs being
              > introduced as a result of hasOwnProperty being used in the form
              > obj.hasOwnProperty(p).
              >
              > Imagine that some user input is being used as a key in obj. Although
              > in most scenarios it is unlikely that the user will enter
              > "hasOwnProperty", it is possible, and it is absurd that this input
              > should break an otherwise robust program.
              >
              > In other scenarios, the problem is worse because it is quite likely
              > that the user will enter "hasOwnProperty". For example this is the
              > case if the user's input is a JavaScript program. Two examples of
              > JavaScript programs that have this problem in practice are JsDoc
              > Toolkit and JSLint (although the latter handles the problem with more
              > grace than the former, which simply crashes with an unhelpful error
              > message).
              >
              > To discourage my colleagues from making this error, I wrote a helper
              > function roughly as follows:
              >
              > (function () {
              > var hasOwnProperty; // JSLint chokes on this line
              > if (Object.prototype.hasOwnProperty === undefined) {
              > hasOwnProperty = function (object, key) {
              > return object[key] !== undefined &&
              > object[key] !== object.prototype[key];
              > };
              > } else {
              > hasOwnProperty = function (object, key) {
              > return {}.hasOwnProperty.call(object, key);
              > };
              > }
              > COMPANY_NAME.utils.hasOwnProperty = hasOwnProperty;
              > })();
              >
              > By simply instructing my colleagues to call
              > COMPANY_NAME.utils.hasOwnProperty(obj, key) instead of
              > obj.hasOwnProperty(key), we avoid instances of this bug in our own
              > code.
              >
              > JSLint is rejecting the above snippet because of the variable named
              > "hasOwnProperty". I don't think this is because of any bad practice in
              > my code. Rather, JSLint is rejecting it in order to hide a bad
              > practice in its own code -- ironically an instance of the very same
              > bad practice I was trying to avoid in the first place.
              >
              >
              > The second reason is evidenced in the code snippet above. As you know,
              > some browsers don't provide a hasOwnProperty function so it's wise to
              > implement your own.


              I don't want to modify JSLint to satisfy a single, unnecessary,
              dubious use. I recommend that you change your variable name. And if
              you are trying to implement hasOwnProperty, I think you intended
              object.constructor.prototype[key]. But I don't think it works for
              mischievous objects that have altered or inherited constructor
              properties.
            • Daniel Cassidy
              On Fri, Oct 24, 2008 at 12:40 AM, Douglas Crockford ... I agree. But you misunderstand my complaint. It is trivial to change my program to work with JSLint,
              Message 6 of 6 , Oct 23, 2008
              View Source
              • 0 Attachment
                On Fri, Oct 24, 2008 at 12:40 AM, Douglas Crockford
                <douglas@...> wrote:
                > I don't want to modify JSLint to satisfy a single, unnecessary,
                > dubious use. I recommend that you change your variable name.

                I agree. But you misunderstand my complaint.

                It is trivial to change my program to work with JSLint, and indeed I
                have already done so.

                However, I believe that calling hasOwnProperty in the form
                obj.hasOwnProperty(p) is a bad practice. The reason it is bad is that
                naive, lazy or tired developers will assume that obj is a generic
                container which can contain any key/value pair, whereas in fact it is
                unable to safely contain the key "hasOwnProperty". I have seen this
                exact problem several times in the wild.

                JSLint itself does not make this assumption, so this is not really a
                bug. But I believe that bad practices are best avoided even when the
                developer knows they are safe -- unless there is no alternative.

                My complaint is that there is a trivial, safe alternative to the bad
                practice and that JSLint does not use it. That is all.


                > And if
                > you are trying to implement hasOwnProperty, I think you intended
                > object.constructor.prototype[key].

                You're right. I was retyping from memory -- it's a typo and not
                present in the original code, honest :).


                > But I don't think it works for
                > mischievous objects that have altered or inherited constructor
                > properties.

                I agree, it's impossible to correctly reimplement hasOwnProperty on
                implementations where it is not present, which is a shame. That's why
                my snippet defers to the built-in hasOwnProperty whenever possible.

                That said, you have raised the rather important point that, on
                platforms with no hasOwnProperty, I'm exchanging one problem for
                another in that now objects which are assumed to be generic containers
                will be able to contain members named "hasOwnProperty", but unable to
                contain members named "constructor". I have no idea how I missed that
                -- thanks for the observation.

                That isn't true on modern JavaScript platforms, though, and my dodgy
                attempt at reimplementing hasOwnProperty is very much irrelevant to
                the point that I was trying to raise :).

                Thanks,
                Dan.
              Your message has been successfully submitted and would be delivered to recipients shortly.