Hyperlink support

[scrabsha]

Most recent terminals now support hyperlink thanks to OSC 8. Embedding hyperlinks in compiler error messages would allows users to quickly jump back to their code and fix the error they've just read in their terminal.

Some terminals/text editors (at least Zed and VSCode) already support this in a hack-ish way by trying to guess what could be a path and opening a file with the same name when the path is clicked. Embedding hyperlinks in annotate-snippet output would make this much more reliable and make it available to other terminals/text editors as well!

---

Hyperlinks probably need to be opt-in, as I'm pretty sure there are some situations in which it is not desirable to embed hyperlinks :D

We also need to make sure we embed the right URIs, as different applications expect different URI format. rg embeds a few presets (source):

Alias	Expands to
default	Platform-aware file:// scheme (see below)
file	file://{host}{path}
vscode	vscode://file/{path}:{line}:{column}
vscode-insiders	vscode-insiders://file/{path}:{line}:{column}
vscodium	vscodium://file/{path}:{line}:{column}
cursor	cursor://file/{path}:{line}:{column}
macvim	mvim://open?url=file://{path}&line={line}&column={column}
textmate	txmt://open?url=file://{path}&line={line}&column={column}
kitty	file://{host}{path}#{line}
grep+	grep+://{path}:{line}
none	Explicitly disable hyperlinks

I think it'd be reasonable to do the same thing and let the program who uses annotate-snippets pick which URI scheme should be used. In the future we may want to let the user supply custom URI formats, but the list above is good enough as a first start.

In order to generate a valid URI, we need a few things:

• (1) The hostname of the machine the file originates from.
• (2) The absolute path to the file the diagnostic originates from.
• (3) The line/column an annotation refers to.

For (1), we have a bunch of default values:

• "" (the empty string) and "localhost", which would break when the diagnostic is displayed on a device that is not the device the file originates to (compiler run over an SSH connexion)
• The value returned by gethostname() (the POSIX C function). This would require FFI and unsafe code, which I don't really want to find in a diagnostic rendering library.
• The value returned by std::net::hostname, which is nightly-only (tracking issue), which requires bumping the MSRV from 1.66 to nightly which is a big no no.
• Reading the content of the HOSTNAME environment variable. This environment variable is not mandated by POSIX, and apparently not set in sh (source).
• The output of the hostname command. Running it takes around 30ms on my machine but I guess it's ok to be slow when rendering diagnostics?

I think spawning hostname and reading its output is the most robust thing to do and would prefer doing this. In any case, we should encourage annotate-snippets users to provide a hostname if there's an easy way to get it and use this value over any of the default value listed above.

For (2), we could call Path::canonicalize on the snippet path and use the returned path. This has some downsides (some are listed in the rg code):

• Symlinks are removed.
• This touches the filesystem quite a lot, but once again I think we could allow ourselves to be slow when rendering diagnostics.

Here again, we should allow annotate-snippet users to provide their own absolute path and call Path::canonicalize only when we don't have anything else.

For (3), we could use the line/column we compute from the annotation span and not include any line/column when the annotation is not spanned.

We should also be treat absolute paths and hostnames as untrusted inputs and perform all the normalizations needed before turning them into hyperlinks. I hope it is reasonable to expect that no sane computer user uses special characters in their hostname/file paths.

Finally, we should stay as conservative as possible and fallback to non-hyperlink rendering when there's not enough information available to do it.

---

Given all the requirements and ideas listed above, hyperlinks could be added to the example displayed in the README with something like:

fn main() {
    let source = r#"                annotations: vec![SourceAnnotation {
                label: "expected struct `annotate_snippets::snippet::Slice`, found reference"
                    ,
                range: <22, 25>,"#;
    let report = &[Level::ERROR
        .primary_title("expected type, found `22`")
        .element(
            Snippet::source(source)
                .line_start(26)
                .path("examples/footer.rs")
                .full_path(
                    "/home/scrabsha/git/github/scrabsha/annotate-snippets-rs/examples/footer.rs",
                )
                .annotation(
                    AnnotationKind::Primary.span(193..195).label(
                        "expected struct `annotate_snippets::snippet::Slice`, found reference",
                    ),
                )
                .annotation(
                    AnnotationKind::Context
                        .span(34..50)
                        .label("while parsing this struct"),
                ),
        )];

    let renderer = Renderer::styled()
        .decor_style(DecorStyle::Unicode)
        .hyperlink_format(HyperlinkFormat::Kitty)
        .hostname("localhost");
    anstream::println!("{}", renderer.render(report));
}

This would require adding the following APIs:

impl<'a, T: Clone> Snippet<'a, T> {
    pub fn full_path(self, full_path: impl Into<OptionCow<'a>>) -> Self { ... }
}

impl<'a> Origin<'a> {
    pub fn full_path(mut self, uri: impl Into<Cow<'a, str>>) -> Self { ... }
}

impl<'a> Renderer<'a> {
    pub const fn hyperlink_format(mut self, hyperlink_format: HyperlinkFormat) -> Self { ... }
    pub const fn hostname(mut self, hostname: &'a str) -> Self { ... }
}

pub enum HyperlinkFormat {
    Default,
    File,
    VsCode,
    Kitty,
    // ...
}

[scrabsha]

Mhm. Apparently I managed to research and implement most of this locally without ever seeing Title::id_url 🤦‍♀️. Anyways this is about embedding an URL in the file path displayed in the snippets, not in the error title. I'll update the issue body to reflect this better.

[epage]

We lean towards Origin::path_url and Snippet::path_url, leaving the policy around supported hyperlink formats and hostnames to the caller. This also allows for other kinds of "paths" like errors related to URLs.

[scrabsha]

mhm. i sort of wished we could provide some reasonable defaults so that users wouldn't have to generate URLs over and over. your point on non-file URLs is valid though.

i'll submit a PR adding Origin::path_url and Snippet::path_url in the next few days (i have most of the code for this ready already, it just need some cleanings).

[epage]

One problem with the approach I proposed and is being implemented #386 is that usually the line and column is being dynamically determined which means the caller does not have them for putting in the URL.

[scrabsha]

One problem with the approach I proposed and is being implemented #386 is that usually the line and column is being dynamically determined which means the caller does not have them for putting in the URL.

what do you think of having something like:

pub struct Snippet<'a, T, F = ()> {
    pub(crate) path: Option<Cow<'a, str>>,
    pub(crate) line_start: usize,
    pub(crate) source: Cow<'a, str>,
    pub(crate) markers: Vec<T>,
    pub(crate) url: Url<F>,
    pub(crate) fold: bool,
}

enum Url<'a, F> {
    Eager(Cow<'a str>),
    Lazy(F),
}

impl<'a, T> Snippet<'a, T> {
    pub fn source(source: impl Into<Cow<'a, str>>) -> Self { ... }
}

impl<'a, T, F> Snippet<'a, T, F> {
    pub fn path_url(self, impl Into<Cow<'a, str>>) -> Snippet<'a, T> { ... }

    pub fn path_url_lazy_line_column<G, D>(self, f: G) -> Snippet<'a, T, G>
    where
        G: FnOnce(usize, usize) -> Option<D>,
        D: Display,
    { /* ... */ }
}

(same for Origin)

being added later, after #386 is merged?

it would call the passed closure with the (line, byte column) pair once both have been computed and use the returned object as a URL for the hyperlink. it is not clear to me whether we should pass a char column or a byte column to the user-submitted function. worst case we can pass both (maybe with the char column being wrapped in an option) to the caller and let it decide.

(i quickly looked at the code, and it looks like we can always compute at least the line and the byte column. however, i wouldn't be surprised if there was something subtle i missed - please tell me if i am wrong!)

i am also not sure what are the implications of adding a second generic type parameter to Snippet with regards to SemVer. i think it is ok given that we are pre-1.0.0 so we can get away with this by bumping the minor version number. i am also not happy with the fact this "cascades" to most of the publicly-available types, but i think defaulting it to () makes it a bit easier to work with, the same way as Vec<T> is actually a Vec<T, A>.

at first i was very hesitant on adding a second generic type parameter to Snippet, as users may want to mix different FnOnces and different Displays in the same report, but we could totally let users pass a Box<dyn FnOnce(usize, usize) -> Box<dyn Display>>, which would make it work. it would require allocations and dynamic dispatch, but we already do both internally, so i would dare to claim that it is ok?

bikeshedding is welcome for the following:

• a good name for path_url_lazy_line_column
• whether the closure passed by the user should take usizes or a struct Position { ... }

---

finally, and this is i think the most important: i do think we should refrain from adding this until someone who would actively use it clearly states their requirements. i am convinced this problem can be solved with an approach that can cleanly coexist with Snippet::path_url and that we should not block #386 over a limitation that does not impact it. would it make sense to add something along of the lines of

if you are bothered that you have to compute the line-column for your URL by yourself, please comment at #366.

in the documentation of Snippet::path_url of #386?

[epage]

I would lean towards changing

pub fn path_url(mut self, url: impl Into<OptionCow<'a>>) -> Self

to

pub fn path_url(mut self, url_formatter: impl FnMut(&str, usize, usize) -> String) -> Self {
    self.url = Some(Box::new(url_formatter));
    self
}

The extra allocations per snippet per rendered diagnostic shouldn't be too bad compared to all of the logic from the renderer. The indirection shouldn't be too bad, especially compared to an enum and everything else that is done.

Using an FnMut means we can put it in a Box<dyn FnMut>.

Accepting (&str, usize, usize) makes it so someone can create a formatter independent of the specific diagnostics being rendered and pass that in trivially.

I'm always split about whether to use FnMut or define a custom trait as a custom trait is more flexible and is easier to document but is more API surface.

[scrabsha]

on always requiring a closure: it specializes for callers that don't keep track of the line/column themselves. this makes the API super awkward for compilers that already have their own source map or any other mechanism to keep track of the line/column and/or don't care about some of these (for instance, GitHub permalinks and "classic" file:// URLs). we should give the caller an easy way to pass a pre-computed URL without forcing them to pass it into a closure that ignores its arguments (|_path, _line, _col| url).

on passing a path to the user (and expecting them to use it): this gives to the user the idea that only the path that is passed to Snippet::path/Origin::path is the only path that is needed to generate the URL. in most cases, the path passed to these functions is a relative path, which is not sufficient to create a file:// URL. sometimes the passed path does not even exist (--remap-path-prefix), making this essentially useless. i hear your argument that it'd allow users to create a formatter independent of diagnostic, and honestly i wish it was possible, but frankly looks more like a footgun than anything else.

I'm always split about whether to use FnMut or define a custom trait as a custom trait is more flexible and is easier to document but is more API surface.

i clearly prefer the trait approach, as it would allow passing closures as well as pre-computed URLs, which avoids the questionable gymnastic i showed earlier. something like:

pub fn path_url<'url>(mut self, url: impl UrlGenerator + 'url) -> Snippet<'a, 'url> {
    self.url = Some(Box::new(url));
    self
}

pub trait UrlGenerator {
    fn generate(&mut self, line: usize, byte_col: usize) -> String;
}

impl UrlGenerator for &str { /* ... */ }

impl UrlGenerator for String { /* ... */ }

impl<T> UrlGenerator for T
where
    T: FnMut(usize, usize) -> String,
{ /* ... */ }

it would even allow users to make some (albeit limited) reusable formatters through currification:

let hostname: String = /* ... */;

let url = |absolute_path| {
    |line, _col| {
        format!("file://{hostname}{path}:{line}")
    }
}

Origin::path("relative/path.rs").path_url(url("absolute/path.rs"))

(also, thank you for mentioning FnMut, i completely forgot that rendering a report does not consume it)

[epage]

Right, I forgot about the fact that the path to display may not be the path to render though having that as a parameter doesn't preclude resolving that, one way or the other.

I wouldn't add another lifetime but reuse 'a.

I suspect we'll need to accept an Fn instead of an FnMut because we probably can't call self.url.generate() when generate takes &mut self. Even if it can be made to work out, if we can reserve the implementation flexibility to not require the mutability, that seems better. It should be unlikely someone needs a mutable reference inside of the closure.

[scrabsha]

I suspect we'll need to accept an Fn instead of an FnMut because we probably can't call self.url.generate() when generate takes &mut self. Even if it can be made to work out, if we can reserve the implementation flexibility to not require the mutability, that seems better. It should be unlikely someone needs a mutable reference inside of the closure.

i discovered this like 10 minutes ago --'

i suspect there is a solution involving storing formatters in Arc<Mutex<Box<dyn FnMut ...>>>. i have not investigated this because i was not sure it's worth it. it'd be a pain to write, review and maintain. and as you said, it's quite unlikely that someone requires it.

I wouldn't add another lifetime but reuse 'a.

agreed. it much clearer with just 'a.

---

@epage something that we did not talk about and i thought while working on this: do we want to allow URL formatters to be fallible? in other words, should the closure the user passes require returning String or Option<String>.

on the one hand, i think generating an URL should not be more complicated than some kind of concatenation of the actual file name with the line and column we computed internally. in an ideal world in which const closures exist (and allow for concatenation), i would require URL formatters to be const closures.

on the other hand, i am pretty sure someone is going to come up with a situation in which they need fallible URL generators and i do not want them to be tempted to fallback to an empty string.

we need to decide this before accepting URL formatters, as the compiler reports conflicting implementation if the trait is implemented for two Fn, even if they have a different prototype (playground link).

i don't really have an opinion on this, do you?

[epage]

The question with fallibility is whether the cases where it happens can be handled silently or should they be handled more directly.

If you put the lookup for the hostname in the function and it fails, likely can render without it.

Should we support the user parsing a URL format string in the function? In that case, returning None or and Err is dependent on their specific use case.

If we want to support checking for hyperlink support in the function, then we should support None. Purely on its own, we have insufficient information for checking hyperlink support since we don't know what we will be written to but all of annotate-snippets is designed around assuming the target supports ansi escape codes. The caller can either use Renderer::plain (and not set a URL) or use anstream. So that isn't a problem.

So a lot of this comes to:

• For the most part, we don't know
• We could have support checking for hyperlinks in the function but should we?

We will make breaking changes in the future. We could settle this by going the more restrictive route and seeing what feedback that leads to. Over-generalizing means people are unlikely to tell us something is important or not.

[epage]

Also, I suspect we should be more specific than UrlGenerator. Maybe PathUrlFormatter or FormatPathUrl.