language-python icon indicating copy to clipboard operation
language-python copied to clipboard

combineSrcSpans has a hidden assumption

Open BenjaminCosman opened this issue 4 years ago • 4 comments

combineSrcSpans treats its first argument as the 'start' and second as the 'end', leading to strange behavior like combining (1,1-2) and (1,1) to get (1,1):

>>> x = SpanCoLinear {span_filename = "", span_row = 1, span_start_column = 1, span_end_column = 2}
>>> y = SpanPoint {span_filename = "", span_row = 1, span_column = 1}
>>> combineSrcSpans x y  -- evals to (SpanPoint {span_filename = "", span_row = 1, span_column = 1})

Either the documentation needs to reflect this assumption (it currently incorrectly says "Combines two SrcSpan into one that spans at least all the characters within both spans.") or the implementation needs to be fixed. (I'd be happy to fix it myself if you say which of those two fixes to do.)

BenjaminCosman avatar Dec 12 '19 01:12 BenjaminCosman

Thanks @BenjaminCosman. It is a while since I wrote this code, but I must have assumed something about the ordering.

I don't have a strong feeling for the "right" solution. Do you have a preference? Can you see any pros/cons of different alternatives?

bjpop avatar Dec 14 '19 07:12 bjpop

I was using it to define superspan: x is a superspan of y if (combineSrcSpans x y) == x. So for this purpose, anyway, I'd want the symmetric functionality described by the comment, not the current implementation with assumptions about the order. I can't think right now of cases where I would want those assumptions, so the only reason I can think of to preserve the current implementation would be that for unknown reasons someone else might be relying on it.

BenjaminCosman avatar Dec 14 '19 19:12 BenjaminCosman

Hi @BenjaminCosman. Given that combineSrcSpans is out in the wild and I'm not sure how people might be depending on its behaviour and/or performance it is hard to say whether changing it would affect people.

My proposal is that we add more documentation to combineSrcSpans to characterise its current behaviour and assumptions about ordering of arguments, but leave the implementation as it is. This will mean that you will have to write your own implementation of superspan without using the current combineSrcSpans. However, you might want to write another version, called (say) combineSrcSpansCommute that behaves in your desired way.

I realise that this is probably not ideal from your point of view, but it might be the least breaking change for others.

bjpop avatar Dec 18 '19 03:12 bjpop

I agree, that's the best way to handle it.

BenjaminCosman avatar Dec 20 '19 19:12 BenjaminCosman