(my browser farted the half finished report into existence :P )

The __eq__() implementation of the path-based loaders in importlib is just the stock one that compares object identity.  So two that are effectively the same compare unequal.  This has a material impact on ModuleSpec.  ModuleSpec.__eq__() does a comparision of its various attributes, one of them being the loader.  Thus most specs will compare unequal even though they are effectively equal.

I recommend that we provide a better implementation for SourceFileLoader and friends.

Larry: would such a feature addition be okay?

Here's a patch.

There can be some interesting backwards compatibility consequences when adding an __eq__ implementation to a class that was previously using the default ID based __eq__:

- it becomes unhashable (unless you also add a suitable __hash__ definition)

- subclasses with additional significant state may start comparing equal, even though their additional state is not taken into account by the new __eq__ function.

For the latter problem, you can alleviate it by comparing the instance dictionaries rather than specific attributes:

>>> class Example:
...     def __eq__(self, other):
...         return self.__class__ == other.__class__ and self.__dict__ == other.__dict__
... 
>>> a = Example()
>>> b = Example()
>>> a == b
True
>>> a.foo = 1
>>> a == b
False
>>> b.foo = 1
>>> a == b
True

(technically this can still change subclass behaviour if they're messing about with slots, but there *is* such a thing as being *too* paranoid about backwards compatibility)

The hashability problem is easy enough to handle just by mixing together the hashes of the attributes of most interest:

    def __hash__(self):
        return hash(self.name) ^ hash(self.path)

Brett, could you weigh in please?

I'm fine with the suggestions Nick made. While loaders are not technically immutable (and thus technically probably shouldn't define __hash__), they have not been defined to be mutable and mucked with anyway, so I have no issue if someone breaks the hash of a loader by changing an attribute post-hash.

Good point, Nick.  Here's a patch that follows your recommendation on both points.

Unless there are objections, I'll commit this in the next day or two.

That's not how this works, Eric.  I have to give you permission to add a new feature, which I remind you I have yet to do.

My bad, Larry.  I guess I was reading between the lines too much. :)

That reminds me: I ended up working around this in the runpy tests by only
checking the loader type was correct in the module specs. With an improved
definition of equality for loaders, the runpy tests could be both
simplified *and* made more rigorous at the same time (by simply comparing
specs for equality, the same as the other module level attributes).

Yeah, it was while writing tests that I ran into this.

So can you tell me how this will make users' lives easier?  I don't really understand the issues involved.  But the only concrete thing I've seen mentioned is making testing easier, and that's not worth breaking feature freeze over.

Yeah, I think we can safely leave this to 3.5.

Right now say you have 2 module specs that are the same.  The only difference is that the 2 loaders are not the same instance (they were created separately with the same arguments, ergo equal).  The two specs will not compare as equal even though they are equal.

I expect users will find it surprising if they compare module.__spec__ to another spec that is basically the same (as described above) and it resolve to not equal.  I can see this as particularly vexing for importer writers that are switching over to the new spec-based APIs.

In my mind, the benefit of removing that unexpected (and aggravating) behavior outweighs the risk that someone is depending on identity-only comparision for the two loader types that are impacted by this change (which were both just added in 3.3).

Importer writers are already used to __loader__ being annoying, and comparting specs for equality is unlikely to be a common thing (and easily worked around by comparing spec.origin instead)

1. Is this patch going to change best practice for working with ModuleSpec?

2. If we delayed it to 3.5, will users have to ignore it to work around the deficiencies of the ModuleSpec implementation in 3.4?

I'm guessing the answer to both of these is "well, no, not really" simply because comparing ModuleSpec objects is not expected to be a regular operation.

Yes, I think it will just make the third party idiom for testing that the
right module was imported to be to check spec.origin rather than comparing
specs directly. It's a nice-to-have, rather than something essential that
justifies breaking feature freeze.

That is, I think the answer to both your questions is actually "Yes, but it
doesn't really matter due to the obscurity of the use case".

I'm fine with this.  Thanks, Larry, for your attentiveness and diligence.

So, not to yank your chain, but... I'm okay with checking this in.  Yes, we're already in beta, but ModuleSpec is brand new, and the sense I get is that this use case is obscure even for ModuleSpec.  The only installed base is beta 1 users, and given that this is new functionality...

Anyway.  I expect to tag late tonight, say in twelve hours.  Can you check it in before then?

I'll commit it in a little while.  Thanks.

New changeset a72a0e4dad20 by Eric Snow in branch 'default':
Issue #19927: Add __eq__ to path-based loaders in importlib.
http://hg.python.org/cpython/rev/a72a0e4dad20

You broke buildbots.  Please fix.

http://buildbot.python.org/all/builders/AMD64%20FreeBSD%2010.0%203.x/builds/1389

Hmm, hard to see how you caused that with the path loader change.  Still please take a quick look.

I fired off another build to see if it was a transient error, but that'll take a while.

I'll take a look.  It could be something with #19713 or #19708 that also failed there.

The other failing buildbot for those 3 changesets is http://buildbot.python.org/all/builders/x86%20Windows7%203.x/builds/7800.

The windows buildbot failure looks like a race condition in a test unrelated to my changes (see issue #20127).  I'm looking at the FreeBSD failure now.

Which passed on the subsequent run...

The FreeBSD failure happened in test_threading (apparently), where it was the last test to "finish".  In the passing run it finished 339/388 -- the seed was different (1253928 vs. 5389019).

This does not seem to be related to my 3 changesets.  I'm guessing it's one of those lingering race conditions we have sprinkled here and there.