Convert SourceLocation to Range<GraphQLLocation>, not just starting line/column

[dylan-apollo]

Needed for diagnostics in https://github.com/apollographql/router/pull/5247 .

Seems like maybe we want to have a single struct somewhere instead which returns start and end to save a bit of extra work in calling both methods? But not sure how to align that with the need of GraphQLLocation to be the spec's execution error location.

[goto-bus-stop]

Maybe instead of get_line_column_start and get_line_column_end, we could have a method that returns a (GraphQLLocation, GraphQLLocation) tuple? This would address the double-lookup and I think it would be extremely rare to only need the end but not the start.

The methods could be called get_line_column and get_line_column_range for getting the start only or the start+end range respectively, addressing the two common cases.

[SimonSapin]

+1 for get_line_column and get_line_column_range. The latter could even return std::ops::Range<GraphQLLocation> (or Option of)

[dylan-apollo]

+1 for get_line_column and get_line_column_range. The latter could even return std::ops::Range<GraphQLLocation> (or Option of)

I like having a struct that encodes the meaning, rather than a tuple (which is clear to me but not necessarily to everyone). std::ops::Range would work, but it specifies that the end is exclusive. Is that true of end_offset()? Feels weird for line/col, since it might wrap to the next line just to be exclusive.

[trevor-scheer]

Having the range available would be a nice convenience for the language server. The type that's most interesting for my use case is the lsp_types::Range which says 0 indexed with an exclusive end, so that sounds compatible with the suggested Range<GraphQLLocation> approach. I am indifferent to tuple or struct, I think in this case the ordering of the tuple is very clear. A struct would match the lsp_types::Range type more closely, but in either case I will be mapping the type.

[dylan-apollo]

I've reworked this PR to a completely new proposal that uses new structs for representing both line/col, decoupled from the GraphQL error representation.

However, given that we're going to need an exclusive end & zero-indexing for LSP (and therefore composition), this may need further tweaking to get everything just right, maybe with Range<LineColumn> instead of the new dedicated struct. Interested to hear any ideas on how to best handle exclusive ends.

[dylan-apollo]

On closer inspection, TextRange is already using exclusive ends. So I updated to Range<LineColumn> and made it 0-indexed. I then switch to 1-indexing only when converting to a GraphQLError. I don't know how all the pub interfaces are being consumed elsewhere, so please scrutinize to see if some of those types need to be swapped around (e.g., if it's important to transform Diagnostics to GraphQLError outside of json conversion).

[dylan-apollo]

Also, a quick prototype of these changes in router: https://github.com/apollographql/router/pull/5263