Track end line numbers and column information

[viluon]

This change aims to improve source location tracking for purposes of error reporting. I deliberately maintained backwards compatibility, however the (original) choice of textOffset for determining the line number is questionable, as it typically points to the node's identifier rather than to the actual start of the node in the source code.

For example, in the test for f1, the reported FileLocation points to the start of the identifier f1 rather than to the keyword void.

@Location
void f1(@Location int p2) {

}

I would argue a more correct implementation would use startOffset rather than textOffset, perhaps exposing textOffset via another property of FileLocation. I'm happy to make changes here but went with the backwards-compatible default.

One more thing that occurred to me is that these line & column coordinates could be tracked in smaller classes still and exposed as two (or more) pairs in FileLocation, e.g. something like

data class FileLocation(
    val filePath: String,
    val start: SourceCoordinates, // based on startOffset
    val end: SourceCoordinates,
    val text: SourceCoordinates, // an extension based on textOffset
) : Location()

might be a better model. Such a backwards-incompatible API change might be rejected outright or take too long to incorporate and I'd personally prefer some column information rather than none. I'm happy to propose the necessary changes if maintainers are willing.

[viluon]

Are there any maintainers available for review? Perhaps @hfmehmed or @ting-yuan?

[usharik]

We really need that. Please review

[viluon]

Thank you for the review @hfmehmed. As I don't have write access to this repository, could you please merge this PR as well?

[hfmehmed]

Apologies for the delay. The PR looked good to me but I will check with @ting-yuan as well about it before merging. Bear with us on this one. Thank you

[viluon]

Thank you @hfmehmed. This hopefully shouldn't preclude merging this change, but I would appreciate it if you and @ting-yuan could share your thoughts on the textOffset vs startOffset issue and related compatibility concerns I outlined in the PR description.

[viluon]

@hfmehmed / @ting-yuan any updates?

[viluon]

FWIW, I resolved the conflicts (removal of support for KSP 1). I had build failures locally in tests related to Android, none of them seem related to my changes, however.

[ting-yuan]

Thanks a lot for the patch and opening the discussion!

Unfortunately, as you already mentioned, this patch would break the binary compatibility, meaning that processors would be required to be recompiled and re-released. I'm sorry that it was actually a bad design choice back in the early days of KSP. Location shouldn't be sealed, and FileLocation shouldn't be data class in the beginning.

Instead of changing the FileLocation, I'd recommend introducing KSNode.detailedLocation and DetailedLocation, and welcome proposals as long as compatibility is maintained.

[viluon]

@ting-yuan thanks for the response! Back when I was making this change, I assumed that FileLocation was only ever constructed inside ksp. Of course, that's generally not true, since the constructor is public. However, a quick skim of code on GitHub suggests that this assumption holds, at least for open source.

I'm not familiar with the binary compatibility guarantees/requirements of ksp. Would you consider this PR breaking even if downstream users only read FileLocations and never constructed them? (It introduces new properties but doesn't change existing ones.)

[viluon]

@ting-yuan could you advise on which direction to take? Whether to change FileLocation or design something like DetailedLocation instead.

[viluon]

@ting-yuan or @hfmehmed, any updates?

[viluon]

[viluon]

@ting-yuan @jaschdoc I've rebased onto main. Could you please take a look at my questions above?

[jaschdoc]

Hi @viluon, thank you for pinging us. I think Ting-Yuan's comment still applies. Adding an extension function and DetailedLocation is the best way to go.

Ideally, we change the FileLocation type to an interface and have detailed source coordinates but that's a breaking change and just changing that part of the API does not warrant a major release. This is, however, definitely something that would be nice change for a major release.

[jaschdoc]

See this comment #2499 (comment)