Fix Parsers.Parser.||| (by adding equality comparison for Position objects)

[peteraldous]

Parsers.Parser.||| was not working as expected. I looked into it and discovered that Position objects were using pointer equality, not structural equality. I added this equality function and it fixed the issue.

I won't be offended if there are changes that need to be made to make this more idiomatic or whatever.

[SethTisue]

hi! welcome to the repo

Parsers.Parser.||| was not working as expected

could you add a test case to demonstrate the issue that is now fixed, and to prevent it from regressing in the future?

[SethTisue]

It's a bit puzzling to me that this would have fixed anything, given that the only two subtypes of Position that I see are NoPosition and OffsetPosition, and the former is a singleton for which reference equality should be fine, and the latter is a case class which should have an automatically generated equals. Though in the latter it's comparing offset instead of line and column, as in your change — is the latter somehow more correct?

[peteraldous]

The issue is from an assumption in this pattern: https://github.com/scala/scala-parser-combinators/blob/main/shared/src/main/scala/scala/util/parsing/combinator/Parsers.scala#L422 - it seems obvious that if next2.pos < next1.pos and next2.pos == next1.pos are both false that the only remaining possibility is that next2.pos > next1.pos. However, next2.pos == next1.pos seems to have been checking for pointer equality. As such, the parser acts as if q matched more of the input than p did whenever they match the same amount of the input. Adding an equals method fixes this logic.

[peteraldous]

I'm not seeing the same behavior when I run sbt test on my machine. Is there a way I can configure my machine so it produces the same error as the workflow above?

[Philippus]

I'm not seeing the same behavior when I run sbt test on my machine. Is there a way I can configure my machine so it produces the same error as the workflow above?

I'm also having trouble with triggering the same error locally.

[peteraldous]

The puzzle might be explained by the fact that https://github.com/scala/scala-parser-combinators/blob/main/shared/src/main/scala/scala/util/parsing/input/StreamReader.scala#L69 creates an anonymous subclass of Position.

[SethTisue]

Ah, that seems to makes sense, though to be really sure, I'd have to dig deeper than I have time to do.

We are... understaffed in this repo for knowledgeable PR review.

As a result, I'm not sure how to proceed; optimistically merge, or ask you to further defend the change. wdyt? How sure are you this is now right?

(Anyone else watching the repo available to weigh in?)

[peteraldous]

Thanks for the response. I have plenty on my plate, as well, so I understand.

I am happy to further defend the change but thought I'd answered all of your questions: the tests I added demonstrate correct behavior and if you comment out the equals method I added, the last of the three breaks. I also added a comment with a pointer to code that makes an anonymous subclass of Position that doesn't have an equals method.

I would much rather that you wait until you're comfortable merging. Is there anything else I can answer or tweak? (Happy to have comments from others on the repo, too.)

[SethTisue]

Thank you!