bug#32034: 26.1; [PACTH] better xref-location-marker for imperfect file locations

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 7/4/18 4:37 PM, João Távora wrote:
>
>> Still better than failing randomly.  Especially if you hint that the
>> match was approximate (much like the way diff tell you about "fuzz").
>
> The problem is it will "break" correct navigations sometimes.

I don't understand.  Will _successful_ navigation ever land me on a spot
where the identifier I looked for _isn't_ there?

>> Anyway, those totally hypothetic future users of the class could well
>> and polish the default behaviour if they decided it's worth it.
>
> Suppose we use a new class. If the hint argument is mandatory, then
> there should be no problem: the backend explicitly asked for this
> behavior.

Yes, we agree.

> Similarly if xref-file-location grows a new optional field which
> defaults to nil.

Only in this case it's more code in the backend, and repeated across
backends.

>>> It can certainly become outdated if the user has added or deleted a
>>> couple of lines there.
>>
>> But then, in that case, isn't it much better to find an actual match
>> (and perhaps warn) than to land the user in nowhereland?
>
> Yes. And in Grep results, it might be beneficial to use the new
> behavior. The code creating the locations will pass line's contents as
> the "hint" (and maybe we should make the hint a regexp, to be able to
> use the line-beginning and line-ending anchors). In that use case,
> though, it would be better to error out if the hint is not found.
>
> I'm somewhat worried about what will happen if a hint misdetects a
> match anyway, and we're in many-automatic-edits land (such as
> xref-query-replace-in-results), but on the balance, it'll probably be
> better than the current behavior anyway.
>
> Except for, err... lines with several matches on them. Not sure what
> to do about those. A special class, probably.

So it would be a good idea to have this in grep/xref grep results after
all?

>> Because I want something with some default behaviour.  Behaviour that is
>> admittely half-decent, but nevertheless better than current behaviour.
>> But since you showed that xref-location-marker is used by your grep
>> substitute, I don't want to cause any regressions in its existing,
>> broken behaviour, whatever that may be exactly.
>>
>>> It might be shorter implementation-wise, too.
>>
>> This is how I imagine the implementation:
>>
>>     (defclass xref-hinted-location (xref-file-location)
>>       ((hint :initarg :hint))))
>>          (cl-defmethod xref-location-marker :around ((l
>> xref-hinted-location))
>>        <...code to search around...>)
>>
>> Compared to the "add optional field" approach, there would be about 1 extra 
>> line,
>> the defclass one.
>
> If we use an optional field, we could call ignore-errors around
> forward-char if that field is present (your proposal number 1).

I don't fully understand this, but I just remembered that it's better to
have a separate class for another, probably more interesting reason.  We
should just make it a mix-in: that way, we can give "hinted" semantics
to existing location classes, not just xref-file-location.

Regarding ignore-errors, we should (quite independently of the remaining
discussion) first agree if xref-location-marker should be allowed to
error at all, and what should happen if it does.