Nodes with same content from different locations considered equal

[apiology]

Parser::AST adds 'location' in Parser::AST::Node on top of its superclass AST::Node, but doesn't overwrite the #hash and #eql?

https://github.com/whitequark/ast/blob/master/lib/ast/node.rb

https://github.com/whitequark/parser/blob/05b88aa0468ce29dfa21e1aff2f0506b1d5d82e1/lib/parser/ast/node.rb

This bit me when I tried to use a node as part of a cache key - is this intended behavior?

See castwide/solargraph#857 for the code in question which triggered this issue

[Earlopain]

Makes sense, I think. A different datapoint from prism: https://github.com/ruby/prism/blob/6ae402ee77248af5c373e84820a3414f99b601cd/test/prism/ruby/parser_test.rb#L47-L55

[apiology]

I see this has been hashed out (so to speak) in #184

Consider this a datapoint to that discussion, but no objections here if folks want to close this issue - https://github.com/castwide/solargraph has a workaround.

[iliabylich]

The way I understand it is that location is a "metadata"-like property of a node, and so it shouldn't impact comparison/hashing.

For example here:

a = b + c
a = b + d

I'd expect a node representing b to be the same in both statements. If I correctly understand your proposal you suggest to make them different (and even more, would there be any two nodes at all that are equal/have the same .hash?).

Also, that's a breaking change (which is probably the strongest counterargument)

[apiology]

I bring no proposal, only the information that this was surprising and caused an issue for us. Do with it what you will - we are fine as-is.

[mbj]

@apiology Depending on identites Hash#compare_on_identity may work for your use case.