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 5:53 PM, João Távora wrote:
>
>> I don't understand.  Will _successful_ navigation ever land me on a spot
>> where the identifier I looked for _isn't_ there?
> A similar string in the buffer, yes. But it might be not its definition.
>
> Like in the example I made up, clojure.core/cons -> "(defn cons".

If this backend already exists, then I think it is flawed.  If it
doesn't, then it shouldn't be designed like this.  In a better design
xref-backend-identifier-at-point would return "cons", propertized with
some contextual information (in this case the fact that it belongs to
the "clojure.core/" namespace).

> And I *do* think, when a hint is not found, the method should raise an
> error. This will be helpful for the search-replace functionality.

Then maybe that functionality should bind some global variable to
control this behavior (in CL this would be done more elegantly with
restarts, but we don't have those yet).

>>> 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.
>
> Err, no? In particular, if the new code is in xref-file-location, any
> class inheriting from it could use it automatically. No need to repeat
> it.

We seem to be misunderstanding each other, and perhaps this is not so
important.  I merely meant that if the behaviour is not the default,
then backends will have to repeatedly activate it explicitly.
Simplistically, if the default behaviour were good enough for more than
50% of such backends, we're better off making it the default.

>> So it would be a good idea to have this in grep/xref grep results after
>> all?
>
> A good one, yes, but not so easily-implemented one.

Indeed, I don't fully understand what you have in mind.  But maybe
(actually hopefully) it could be implemented on top of my simplistic
approach.

>>> 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.
>
> It sounds useful, but I'm not sure how useful it's going to be in
> practice. E.g. elisp locations already have their own logic for
> that. Etags does, too.

Yes, and it could/should be refactored to use the new mixin.  Also
you're ignoring stuff outside Emacs.  The main reason I'm interested in
this is, obviously, my own itch.  Which in this case is SLY and Eglot.
Both need locations and both need hinting.  Incidentally, SLY's ancestor
SLIME is where all this xref rubbish was lifted from in the first place
(mighty fine rubbish mind you :-)).

> And if another backend decides not to use xref-file-location, it will
> likely do so for reasons incompatible with this mixin's implementation
> too.

No.  Precisely the contrary.  In this proposal, the two classes are
independent so you needn't scratch one if you just don't like the other.
>
> Anyway, if you'd like to propose a patch, that could be easier to discuss.
>> 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.
>
> I think it should.
>
>   (cl-defmethod xref-location-marker ((l xref-bogus-location))
>     (user-error "%s" (oref l message)))
>
> is the canonical example.

Yes, but we have to agree how/if to unwind.  There are at least three
situations:

a) Error before the file is found (and buffer created)
b) Error after the file is found
c) No error

I think a) should always happen, but b) should only happen if a global
flag is bound (that seems to help your replacer, right?)

> And then xref-query-replace-in-results should catch them. I thought
> it's doing that already. :-(

João