Fix bad line number assertion in ScriptInfo.positionToLineOffset

[elibarzilay]

This is the line number side of ecddf84 (from #21924, fixing #21818).
But the code is slightly improved for both cases: instead of testing
that leaf is defined, check whether lineCount is zero, and if it is,
return 〈1,0〉 for the one-based line and zero-based column numbers.
(The requirement of lineCount > 0 is also seen in the fact
that lineNumberToInfo expects a "OneBasedLine" argument.)

I've stared at this code way too much, since I think that there is
something more fundamentally wrong here. E.g., EditWalker only
pushes to startPath but never pops even a children-less node that
is left after deleting the whole contents. But I can't figure out the
overall structure, which is also why the test that I added is not
great (see the comment there; also, #21924 is dealing with the same
problem and didn't add a test).

Fixes #44518.

Also, fix a random redundantly-nested if that I ran into in this file.

[elibarzilay]

@amcasey: I think that this is fine as is given the precedent of #21924, but I'd much rather add some tests and/or some explanations in the file.

[elibarzilay]

@amcasey: I'm not sure what should be in the tree or what it should represent. I also guessed that it should never return the 0 line-count, since it should be equivalent to an empty node -- not only is this a vague guess, there seem to be very few tests for all of this, which is why I don't want to add things that might break in unexpected ways.

(Yeah, the GH UI was confusing.)

[amcasey]

@elibarzilay But this change already takes a dependency on the relationship between leaf and lineCount. It seems like the minimal hack would be oneBasedLine: this.lineCount() || 1 (or max, if you prefer). In my view, that more transparently hacks around the assert.

Having said that, unless this concretely improves the user experience (and I'm not sure having fewer exceptions in the log counts) or reduces problematic noise in one of our processes, my inclination would be to do nothing until we understand it well enough to make a correct fix (which might be far in the future).

[elibarzilay]

Using max(_, 0) will resolve it, but will swallow potential bugs that
may return a negative result for whatever reason, therefore _ || 0 is
better.

And by the same token, the fix here is better than _ || 0, since it is
more specific.

[elibarzilay]

@amcasey, so after even more quality time spent trying to get a test that works, I figured that the best approach is not to assert the value of this.lineCount(), but instead use it as a test, since if it's zero then lineNumberToInfo will fail to get a leaf, so this is now an improvement over the #21924 fix. (My best guess is that these editor locations suffer from the common confusion of having a zero line count means that the position is on line 1.)

In any case, I did this improvement and in addition I also added a Debug.checkDefined around the leaf expression.

(And BTW, just to be clear, this is the exact same issue that #21924 resolved partially -- see the stack traces of #21818 and #44518: both are coming not from the edit operation itself but from the getNavigationTree that happens later.)

The test is not great -- see the comments in the test for that. But it will still be useful to see if there's a deeper underlying problem here if there's a decision to invest time on that (but I'm more confident now that this is the right fix).

[amcasey]

Thanks!