Addresses should not equal things that are not addresses.

Review Request #3791 — Created April 29, 2016 and submitted

nhoward_tw
pants
3306
pants-reviews
benjyw, jsirois, molsen

Currently addresses return NotImplemented when other is not an instance of Address. That's fine, but __ne__ calls __eq__ without checking for NotImplemented, which is truthy, so address != not_an_address will return False.

This changes __eq__ to return False instead, and adds a regression assertion. It also updates __ne__ so that it uses the operator so that it can't propagate NotImplemented even if it might otherwise have been propagated.

Ran address tests locally, CI away in PR.

MO
  1. Ship It!
  2. 
      
JS
  1. 
      
  2. 
      
ST
  1. I believe that this is correct. See:

    https://docs.python.org/2/reference/datamodel.html#object.eq

    But it's possible that only __eq__ on BuildFileAddress should be returning NotImplemented in order to delegate to Address's comparator.

    1. Example:

      >>> Address.parse('blah:blah') == 1
      False
      >>> 1 == Address.parse('blah:blah')
      False
      >>> "blah:blah" == Address.parse('blah:blah')
      False
      >>> Address.parse('blah:blah') == "blah:blah"
      False
      
    2. I wonder if the original intent was to raise NotImplemented() not return NotImplemented()

    3. My link was broken. See:

      https://docs.python.org/2/reference/datamodel.html#object.__eq__
      
    4. TIL!  I think this is exotic enough that it deserves a comment with Stu's pointer if you otherwise ~revert.
    5. The bug isn't in __eq__, it's in __ne__. not x.__<rich-op>__(y) is something you should never do because calling __<rich-op>__ may return something that bool() will coerce to True, but using the operator will coerce to False.

  2. 
      
NH
JS
  1. 
      
  2. src/python/pants/build_graph/address.py (Diff revision 2)
     
     
    Erm - ok.  Has it been determined if we actually want the nuance that NotImplemented provides?  This is our 1 use in the non-exp codebase.  It was an incorrect use (for `__ne__`), and ~no-one understands its use.
    
    If it really must be used, I think a pointer to docs is warranted.
  3. 
      
NH
JS
  1. Yay!
  2. 
      
ST
  1. Ship It!
  2. 
      
NH
NH
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as https://github.com/pantsbuild/pants/commit/0ea93e789eeb87c3bcee2dc0097e2a1d191a9057
Loading...