Compare SearchResult values instead of hashes in eql?

[OskarEichler]

Summary

Compare ordered array contents and modseq in SearchResult#eql? instead of treating matching hash values as proof of equality. Also reject a non-nil modseq when the receiver has no modseq. The existing order-insensitive == and hash calculation are unchanged.

Reproduction

require 'net/imap'
a = Net::IMAP::SearchResult[1, modseq: nil]
b = Net::IMAP::SearchResult[1, modseq: 2]
p [a.eql?(b), b.eql?(a)] # before: [true, false]; after: [false, false]
colliding = Class.new(Net::IMAP::SearchResult) { def hash = 7 }
p({colliding[1, modseq: 2] => :a, colliding[9, modseq: 3] => :b}.size)
# before: 1; after: 2

Verification

• 400 focused checks; 73 failing expectations before, zero afterward. They cover nil/non-nil modseq, ordered contents, plain arrays, subclasses, deliberately colliding hashes and Hash lookup. Related merged 🐛 Fix SearchResult#== for LHS with no modseq #514 fixed ==, not eql?.
• Existing rake test on this isolated branch: 1726 tests, 12588 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications, Ruby 4.0.6 via rbenv. The baseline also passes all 1,726 tests; assertion counts vary slightly across runs.
• Supplemental RuboCop Lint retains the same 48 existing findings. Ruby syntax and git diff --check pass. No repository tests, dependencies or workflows were added or modified; focused reproductions live outside the repository under the consumer's no-new-tests policy.
• Independent branch based on 6d2ef7a636a1e2449187a83b06ac7a5baa54ead2; the runtime diff from released 0.6.6 is documentation-only before this patch.

Compatibility and limits

No API, dependency or Ruby-minimum change. Distinct values no longer collapse into one Hash key. Nil-modseq Array compatibility and the non-nil same-class restriction remain. Plain Array receivers still use Array equality; this does not promise cross-class symmetry for arbitrary Array subclasses. Other Ruby/OS versions were not run locally. No production or external IMAP service was used. Local verification does not imply upstream CI approval or comprehensive behavioral coverage.

[nevans]

I remember finding #514. It's too bad I didn't spot this at the same time!

Can you add a few tests please?

[nevans]

return false unless super is good, except that checking modseq could be much faster than checking Array#eql?, and it's a good idea to short-circuit when possible. So the modseq should be checked first, and call super last:

Suggested change

	return false unless super
	if modseq.nil?
	!other.respond_to?(:modseq) || other.modseq.nil?
	else
	self.class == other.class && modseq == other.modseq
	end
	if modseq.nil?
	!other.respond_to?(:modseq) || other.modseq.nil?
	else
	self.class == other.class && modseq == other.modseq
	end &&
	super

I think we should use the same type comparison on both sides of the conditional, either other.respond_to?(:modseq) or self.class == other.class. I'd also be okay with self.class === other. But I lean against using respond_to? in eql?.

I'll note that SearchResult#== also uses respond_to?(:modseq), but it's normal for #eql? to be stricter than #==.

Suggested change

	return false unless super
	if modseq.nil?
	!other.respond_to?(:modseq) || other.modseq.nil?
	else
	self.class == other.class && modseq == other.modseq
	end
	if modseq.nil?
	!(self.class === other) || other.modseq.nil?
	else
	self.class === other && modseq == other.modseq
	end &&
	super

That can be further simplifed as

Suggested change

	return false unless super
	if modseq.nil?
	!other.respond_to?(:modseq) || other.modseq.nil?
	else
	self.class == other.class && modseq == other.modseq
	end
	(self.class === other ? modseq == other.modseq : modseq.nil?) &&
	super

Looking at SearchResult#==, it has an even nicer way to compare modseq:

Suggested change

	return false unless super
	if modseq.nil?
	!other.respond_to?(:modseq) || other.modseq.nil?
	else
	self.class == other.class && modseq == other.modseq
	end
	modseq == (other.modseq if self.class === other) &&
	super

As a bugfix, I'd be happy with that. But, as a backward incompatible change (for 0.7.0), I'd prefer to go further:

Suggested change

	return false unless super
	if modseq.nil?
	!other.respond_to?(:modseq) || other.modseq.nil?
	else
	self.class == other.class && modseq == other.modseq
	end
	self.class === other &&
	modseq == other.modseq &&
	super

[nevans]

Related: SearchResult#hash should always incorporate modseq into its hash, not only when it's nil. While that wouldn't 100% fix the issues you identified, it would make them far less likely.

def hash = [super, self.class, modseq].hash