`removeRange`, `replaceRangeBy` with negative range duplicates that range

[lue-bird]

Describe the bug When the start Location is later in the document than end,

• removeRange duplicates the source code in the range with start and end inverted
  • example removing in inverted remove range: test remove test → test removeremove test
• replaceRangeBy ... "A" is like removeRange with "A"` directly between the duplicated sources
  • example replacing by "A" in inverted replace range: test replace test → test replaceAreplace test

SSCCE (Short Self-Contained Correct Example) https://github.com/lue-bird/elm-review-sscce-negative-range

Expected behavior On the one hand it's good that you will notice that you've switched up start and end by having it duplicated; this exact behavior seems unintentional, though.

Possible alternatives are

• whenever there's a negative range, fail fixing with a nice message
• provide a fix which in the inverted range says something like "elm-review rule ERROR: start Location is later in the document than end"
• silently do nothing in the fix

Additional context Found this bug by accidentally messing up start and end locations.

[jfmengels]

I've noticed this problem as well before, and agree that this should not be the behavior. It's pretty cool to be able to duplicate code but I think it's rarely going to be useful the way it is now :sweat_smile:

Whenever we try to apply fixes, it is either going to be successful or create a Problem, and that is our way of reporting issues. And I agree that that should be the way forward. Adding a new variant there would be a semver breaking change unfortunately (which I don't want to happen right now). So either we find a workaround, or we create a ProblemV2 and a FixResultV2 types.

Maybe I should have a | OtherProblem String variant in order to be able to create future kinds of problems without a braking change.

We could also ignore this fix altogether, but I think that is going to be confusing. I think we should probably go for the duplicate types for ProblemV2.

[lue-bird]

Yeah I'm sure we can find more edge-cases like this one in the future, with negative locations or non-existent locations etc, so adding an | OtherProblem String variant as well seems future-proof. When some day v3 would come, all the found cases can then be explicitly added if it makes sense.