Fix Symbol.__eq__ to return false when comparing with None

[weaversam8]

Fixes #1476. Symbol.__eq__ now returns False when comparing with an object that is not an instance of the Symbol class.

[MegaIng]

No, the class should not just silently return False, since this is an internal class and comparing it would other stuff indicates an internal bug in our library - Making the check an assert instead would be fine, and @erezsh said that specifically comparing with None returning False would also be fine.

In what context are you encountering this error message? What are you trying to do?

[weaversam8]

So, technically a client could encounter the Symbol class thru the term_subs option of the experimental Reconstructor class, though that's probably the only API surface that exposes this class.

I'm encountering this not thru that API, but because I had to get pretty deep into the internals of the reconstruction process, and reimplemented both the Reconstructor and the WriteTokensTransformer classes in my PR to python-hcl2: amplify-education/python-hcl2#177. (Be warned, this is a bit hacky, and far off the beaten path for how users should integrate with Lark.)

Has the assert used here helped catch a bug before? I opened #1476 in the first place because I figured that overrides of __eq__ would typically want to extend its existing compatibility rather than reduce it. Since the default behavior for Python supports comparing any two objects (regardless of if they're instances of the same type) I assumed that you would want to preserve that behavior here. If that's not the case, that's fine.

[MegaIng]

So, technically a client could encounter the Symbol class thru the term_subs option of the experimental Reconstructor class, though that's probably the only API surface that exposes this class.

Aha, yeah, that is clearly public API, even if experimental. We could either change the API to only use str (since we don't actually need to extra info stored in the Symbol instance here) or update Symbol.__eq__ to not raise an error and instead return NotImplemented (like a good class should do) - we definitely shouldn't keep the current state.

(Be warned, this is a bit hacky, and far off the beaten path for how users should integrate with Lark.)

If your changes are usable outside of your specific grammar, contributing them back would most likely be much appreciated - the Reconstructor is experimental and we would like it to be useful - Ideally you don't have to know anything about it's internals to use it, although that is probably slightly too idealistic thinking.

[erezsh]

I'm okay with returning NotImplemented. Whatever bugs it may catch, they would probably trigger the tests anyway.

[weaversam8]

OK, I updated this PR to return False for None and to raise a NotImplementedError for other types.

If your changes are usable outside of your specific grammar, contributing them back would most likely be much appreciated

@MegaIng - if I get some more bandwidth soon maybe I'll make a PR. The gist of the limitation I encountered was this: the insert_spaces option for Reconstructor was not flexible enough for my use case. I needed to make decisions about where to insert whitespace while the Reconstructor was visiting each token of the AST. To accomplish this, I ended up:

• creating my own class that extended Transformer_InPlace to replace the WriteTokensTransformer implementation
• creating my own class that extended Reconstructor to use my custom transformer implementation

I think the most general way to allow this type of customization would be to allow the users to provide their own write_tokens argument to Reconstructor to adjust how it outputs tokens as it traverses the AST.

[MegaIng]

OK, I updated this PR to return False for None and to raise a NotImplementedError for other types.

No, you should be returning NotImplemented (a distinct thing from NotImplementedError), not raise the exception yourself - The python interpreter will then do the right thing (ask the other type if it can handle this comparison, and if the answer is still NotImplemented, compare by identity)

[weaversam8]

Ah, TIL. Thank you for the correction. See latest commit.

[erezsh]

Thanks for contributing!