make it harder to emit any ANSI colors when color is set to off

[nevakrien]

as discussed in #160 adding a switch specifically for ansi colors is not ideal as it adds a dependency and slows down the usual path.
what this PR does is verify that its impossible to emit ansi colors under any config with color=false without manually inserting the ansi codes into the strings.

this is because:

1. regardless of what ReportStyle gives as the color we filter it
2. all XXX_color() methods filter
3. all label colors are filtered at write time rather than when we add them

if I am reading correctly the old add_labels code this PR does introduce a breaking API change.
since previously setting the config after adding labels would make for those labels being colored.
I suspect this is not intuitive or desired behavior so I am considering this a bug fix.

I think this is much nicer since now users can be confident that setting color to off before printing would always generate output without ansi signs.

[zesterer]

This doesn't actually solve the same problem as the original solution. The reason that toggling colour is hard is because users will insert ANSI escape codes into things like error messages and label text, because that's what the crate encourages them to do. This change doesn't strip ANSI codes from the labels, and so most users will still see unintuitive results.

This is why I think that a proper solution requires work at the level of the formatting API as mentioned in #160.

[nevakrien]

I think if we go with what we taljked about in #160 the issue of inline ANSI codes would still persists it would just be an anti pattern at that case. like other than manually parsing there is simply nothing we can do to avoid a user putting an ANSI charchter into the stream.

but other than that edge case I think the crate should not be adding them. so basically we would require users to not manually insert ansi colors ever. basically it seems there are 2 independent issues

1. the crate sometimes adds colors to the given text even when color=false
2. there are thigns that arent achivble without manually inserting ansi colors.

but if the hope is to solve both on the same PR i can do that as well. i had some ideas for styling

[zesterer]

I think if we go with what we taljked about in #160 the issue of inline ANSI codes would still persists it would just be an anti pattern at that case. like other than manually parsing there is simply nothing we can do to avoid a user putting an ANSI charchter into the stream.

Yes, but I don't think that's a problem. If the user is explicitly choosing to shoot themselves in the foot, there's not all that much that we can do about it, especially if we're providing steel-capped boots.

but if the hope is to solve both on the same PR i can do that as well. i had some ideas for styling

Yep, that's the goal. And I think it's possible!

Consider:

struct Styled<T> { inner: T, style: &'static str }

// Arbitrary, extremely unlikely to appear in any real strings
const STYLE_START: &'static str = "\x1B\x0E";
const ELEM_START: &'static str = "\x1B\x17";
const STYLE_END &'static str = "\x1B\x0F"; 

impl<T: fmt::Display> fmt::Display for Styled<T> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "{STYLE_START}{}{ELEM_START}{}{STYLE_END}", self.style, self.inner)
    }
}

When the user does, say, format!("hello {}!", "world".style("error")) the resulting string is:

hello \x1B\x0Eerror\x1B\x17world\x1B\x0F!

When rendering a diagnostic, Ariadne will search for these escape sequences. When it finds one, it looks up the style name in the 'stylesheet' provided for the relevant backend (like the ANSI CLI one) and apply the relevant ANSI escape sequences to the text.

This has a few advantages:

1. The escape sequence is easier to detect than ANSI ones, so we don't need to pull in heavyweight crates like vte to detect them

2. This scales beyond the ANSI CLI output too. We can use styles for alternative backends that provide their own styling options. Here are some alternative backends I've been thinking about:

• HTML+CSS (style gets translates to an inline DOM element with a CSS style tag)
• LSP JSON (the style gets translated to a clickable link to, say, a location in the source code or some documentation)
• Markdown (style gets translated into markdown **bold**/*italics*)

[nevakrien]

@zesterer I think this would be best implemented in a way that doesn’t clash with Display. We really don’t want to accidentally create structs that render in a broken way when someone tries to call println! on them — that would be very confusing for users of this library or any derived library.

It would also be nice if ReportStyle and Style were actually the same thing, because they appear to be trying to achieve the same goal. With that in mind, it may be simpler if we hold style information next to the text itself.

So instead of just having one big chunk of text, we would have something like:

    Vec<(Str: Display, S:ReportStyle)>

Then later, somewhere in our code, we could do something like:

    for (text, style) in message {
        let color = style
            .get_color(&self.config)
            .filter(|_| self.color);
        write!("{}", text.fg(color))?;
    }

We would simply iterate and apply the styles. This is probably a performance regression compared to what currently exists, but I honestly can’t find a way to support styles that doesn’t require at least some amount of extra work.

There is also a nice trick we can do here for multiple backends.
Because not all backends support all methods it would be a good idea to have that represented in types.
For example markdown does not support colors the same way ANSI and HTML do.

So we could have AnsiStyle, HtmlStyle, MarkdownStyle, etc., to capture this fact.
Then RedStyle wont implement MarkdownStyle but would implement AnsiStyle and HtmlStyle.
so when you try and render a red error message in markdown you would get a compile time error.

ofc ErrorStyle would be implemented for markdown with something like bold.

[zesterer]

I can see your reasons, but I don't think it's the way to go for a few reasons:

1. We have to make some fairly vast assumptions about what styles backends could support, and a new backend that breaks those assumptions might end up causing breaking changes to the existing API.
2. It's not clear to users which backends support which styles, so it's going to open the floodgates to a whole load of "why doesn't my text appear red/bold/etc." issues
3. It doesn't play nicely with the existing Rust formatting infrastructure
4. Because the style gets stored with the report, it means that users can't extend the design later. This is the exact same issue as ReportStyle today.

I think this would be best implemented in a way that doesn’t clash with Display. We really don’t want to accidentally create structs that render in a broken way when someone tries to call println! on them

There's no reason for this to be the case! Let's imagine a slight modification to the API:

macro_rules! text {
  ($($toks:tt)*) => { Text(format_args!($($toks)*)) };
}

pub struct Text(fmt::Arguments);

impl Text {
    fn fmt_inner(&self, f: &mut fmt::Formatter, with_style: bool) -> fmt::Result {
        if with_style {
            // Styles are included in-band, used for diagnostic rendering
        } else {
            // Styles are stripped from the output
        }
    }
}

impl fmt::Display for Text {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        // The `Display` impl doesn't include styling information
        self.fmt_inner(f, false)
    }
}

// Usage

Label::new(span)
    .with_msg(text!("Hello, {}!", "world".style("error")))

Here, the Display impl will not contain any unexpected styling annotations.

[nevakrien]

Yes so the second method was more.in the direction of what I was thinking.

It might even be MORE performent than existing things because it does not force rendering into a string like we previously did.

There might be some middle ground here so basically imagine text! Would be using closures of some sort thus

  ($($toks:tt)*) => { Text(|config| format_args!($($toks.apply(config)*)))};

struct Text(FnOnce(config)->Arguments)

Its the same idea as earlier but we have it delay evaluation of that format until the config is actually applied.

In terms of the api its exactly the same but we don't need to parse the text for special bytes now.

This solves the issue of accidentaly adding config options before the message while keeping things mostly the same.

There is like the additional heap allocation that may need to go there which kinda sucks. So its not a perfect solution.

[nevakrien]

@zesterer
so i made an intial sketch with traits that also supports formating with text_format as well as custom styling.
I went with traits because I really wanted to avoid the perf cost of parsing text when its not needed.

fairly happy with how styling came out like it would be nice for either aproch.
for example you can mark text as never colored with "text".with_style(None) or as red with "text".with_style(Color::Red).
if we go with traits you could have cascading styles similar to CSS because the trait takes color in as an argument.

I also got it to stay backwards compatible with the same trick of default value and after some tinkering none of the tests needed to be rewritten for the new style.

its not done yet the 3 things that are needed are:

1. feature flag colors properly and potentially move some of the code to draw.rs
2. add a general use enum type for the trait. similar to what we did for ReportStyle
3. fix the docs to explain the trait better

but thought I would ask for feedback now since the general shape of the code is fairly clear already

[zesterer]

Unfortunately, I think that the changes are extremely far from what I'm proposing and still largely exhibit all of the disadvantages that I listed before.

I'm going to sketch something up tonight.