Relay Compiler outputted file links - bad format for VSCode

[alex-statsig]

When running the relay compiler with errors, theres a few problems with the outputted erroring filepaths. They are not critical or "wrong", but cause inconvenience when attempting to open the problematic line (especially in vscode). Here's an example output:

[ERROR] Compilation failed.
[ERROR] Unable to run relay compiler. Error details: 
Failed to build:
 - Validation errors:
 - The type `Company` has no field `metric`. Did you mean `metrics`?:client/components/console/metrics_catalog/MetricPageInsightsCard.tsx:495:501

1. Theres no space between "Did you mean?" and the file link. This causes vs-code to think the filepath starts with the "?", so cmd-clicking the file in terminal fails
2. Theres a colon before the path; I'm not sure why this is here (to indicate its relative to the root?), but again it is included in the path opened by VSCode upon cmd-clicking, causing it to fail to find any files
3. The numbers after the path appear to be character indices within the file, whereas vscode expects the format <path>:<linenumber>:<character_index_within_line>. I'm not sure how standard this format is, but it would be very convenient within vscode at least to adopt this format.

[captbaritone]

This is great feedback thank you! I think the relevant code is here: https://github.com/facebook/relay/blob/main/compiler/crates/graphql-cli/src/diagnostic_printer.rs#L89

Would you be interested in opening a pull request?

[alex-statsig]

It looks like this actually coming from somewhere else:
https://github.com/facebook/relay/blob/main/compiler/crates/common/src/diagnostic.rs#L226-L232

which calls into Location's toString: https://github.com/facebook/relay/blob/main/compiler/crates/common/src/location.rs#L86C3-L86C3

I'll put up a PR to fix the missing space, but converting the character span to line numbers seems a bit more involved, as I'm not sure the right information is being passed around to this Diagnostics object from a ton of callsites (in the code you linked, it actually is handled correctly by source.to_span_range(location.span()), but the source is not available here).

I'm still open to helping contribute that part, but it seems so much more involved that I would want a sanity check on the approach (adding location_range to DiagnosticsData)

[captbaritone]

I see. This is just the location information at the bottom of the CLI output which summarizes the errors emitted above? Do you have nicely formatted diagnostics above with the correct line numbers?

[alex-statsig]

I see. This is just the location information at the bottom of the CLI output which summarizes the errors emitted above? Do you have nicely formatted diagnostics above with the correct line numbers?

Ah yeah I do, and that seems to show the correct ranges / format. In my case I had ~20 validation errors and missed the section above. Is there a reason to output the individual errors again at the bottom with worse formatting? Are some errors only listed in the bottom summary? Perhaps that section should just say N Validation errors encountered above or something.

Thanks a ton for the quick responses and help tracking this down btw!

[captbaritone]

I think the report at the bottom serves two purposes. It serves as a terser summary of all the encountered errors, and it also includes non-diagnostic errors such as config errors, etc..

I think replacing the diagnostics that appear here with a message that just includes the count does seem more reasonable.

[captbaritone]

I think the two places we build those lists are:

• relay/compiler/crates/relay-compiler/src/errors.rs

  Line 41 in 19c66f7

  .map(|err| format!("\n - {}", err.print_without_source()))

• relay/compiler/crates/relay-compiler/src/errors.rs

  Line 235 in 19c66f7

  .map(|err| format!("\n - {}", err.print_without_source()))

[captbaritone]

Additionally, I'd be open to a PR that just removes the location information when printing without source. I think it's strictly worse to include misleading information than no information.

[alex-statsig]

#4573 and #4574 both addressed pieces of this, seems resolved now