Diagnostics: Clearer information style

[muqiuhan]

• Add file content to message.
• Use more symbols and line breaks to separate messages.

Description

In this PR, I tried to make some changes to CompilerDiagnostics.fs to make the compiler print clearer error messages (or warning messages, etc.)

I tried adding a Context field to FormattedDiagnosticDetailedInfo, which reads the corresponding content from the target file according to the information range indicated by the Range type, and then adds it to buf in PhasedDiagnostic.

This is my first time entering the world of the F# compiler, and I know this PR has a lot of issues, but I would like to put forward a small idea about #14832 . ^_^

Checklist

• Test cases added
• Performance benchmarks added in case of performance changes
• Release notes entry updated

[github-actions]

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

[muqiuhan]

@dotnet-policy-service agree

[vzarytovskii]

@DedSec256 had a bunch of changes made to diagnostics context some time ago, maybe he has a couple of thoughts and ideas.

In general - we want to kept few things in mind:

1. We probably want to introduce new style of diagnostics separately instead of changing current one and chose it based on compiler flag and/or current environment (see points below). For example, have a DiagnosticStyle.Rich case and then have some control how to enable it so it's easier to test it and it's not disruptive to current tests and tooling, while we're figuring out details?
2. We need to make sure msbuild/vs/vscode/rider/nvim are rendering new style correctly in their inline diagnostics.
3. We need to make sure that current use environment can correctly render those (for example, terminal supports Unicode, colouring, if we decide to enable it, etc).

In conclusion - thanks for that, we do want to make diagnostics prettier and add more context to them, but I think we want a more systematic approach.

[vzarytovskii]

Wanted to expand of one of the points - a separate diagnostic style - it will certainly help to not change 100s of tests which match directly on the output, and will allow us to test things separately and make sure we didn't accidentally break anything in the process.

[psfinaki]

@muqiuhan thanks for your first PR - welcome to the F# world 😎

We'll indeed need to discuss this one in a broader context since diags are a very complex topic. Meanwhile, if you are interested in it, feel free to address any of these problems we've already agreed on, some of them are great to get hands dirty here and we'll be happy to guide you and help you with them :)

[muqiuhan]

Wanted to expand of one of the points - a separate diagnostic style - it will certainly help to not change 100s of tests which match directly on the output, and will allow us to test things separately and make sure we didn't accidentally break anything in the process.

Like wrapping the output? I once wrote a little tool that reads the output of the F# compiler and reprocesses it and prints it out:

[muqiuhan]

feel free to address any of these problems

Thanks for the tip! I will try to participate in solving these problems ^_^

[vzarytovskii]

Wanted to expand of one of the points - a separate diagnostic style - it will certainly help to not change 100s of tests which match directly on the output, and will allow us to test things separately and make sure we didn't accidentally break anything in the process.

Like wrapping the output? I once wrote a little tool that reads the output of the F# compiler and reprocesses it and prints it out:

No, not wrapping necessary, but as you saw, in FormatDiagnosticLocation function, we have a few separate diagnostic styles, in this PR you have changed DiagnosticStyle.Default's output, I suggest we can add a new one, let's say DiagnosticStyle.Rich where we can make a new output style, with your changes there.

And we can set it to Rich (via tcConfig.diagnosticStyle) when certain compiler flag is passed (for example introduce a new flag - --richdiagnostics+|-), so we can leave current diagnostics unchanged for time being (so we're sure no tooling is broken accidentally), and test the new format (by just passing the flag), to see any problematic corner cases.

Later on we can swap the default to Rich with some fallbacks, like if terminal doesn't support unicode, or is too narrow, we fallback to DiagnosticStyle.Default.

Sorry for a bit hectic description, I hope it makes sense :)

[vzarytovskii]

If you would like to, I can make this change in your branch (maybe later this week), to demonstrate what I mean.

[muqiuhan]

I suggest we can add a new one, let's say DiagnosticStyle.Rich where we can make a new output style, with your changes there.

I understand what you mean, you expressed it very clearly, it's a great idea!

like this:

/// Represents the style being used to format errors
[<RequireQualifiedAccess>]
type DiagnosticStyle =
    | Default
    | Rich
...

Then add it elsewhere:

match tcConfig.diagnosticStyle with
| DiagnosticStyle.Emacs -> ...
| DiagnosticStyle.Default -> ...
| DiagnosticStyle.Rich -> ...
...

[vzarytovskii]

I suggest we can add a new one, let's say DiagnosticStyle.Rich where we can make a new output style, with your changes there.

I understand what you mean, you expressed it very clearly, it's a great idea!

like this:

/// Represents the style being used to format errors
[<RequireQualifiedAccess>]
type DiagnosticStyle =
    | Default
    | Rich
...

Then add it elsewhere:

match tcConfig.diagnosticStyle with
| DiagnosticStyle.Emacs -> ...
| DiagnosticStyle.Default -> ...
| DiagnosticStyle.Rich -> ...
...

Yep, precisely. And then we can add a new compiler flag, which will be selecting DiagnosticStyle.Rich as default one based on flag.

[T-Gro]

@muqiuhan : I have marked the PR as a draft to avoid accidental merging.

Once this is done as a new DiagnosticsStyle (per the suggestion from Vlad above), this will be ready for merge.

[vzarytovskii]

I intend to add a flag tomorrow so we can maybe start testing it sooner? It should take me a couple of hours. I will add a new format for the diagnostic and make tests green (won't be adding any tests now). Let me know if anyone has any objections.

[vzarytovskii]

Ok, I've added new flag (--richerrors), and pushed changes (sorry, couldn't wait, that's legit one of the first things I wanted to do once I got on the team, and could never start):

I propose we push the change to the main, so we can start playing with format and how we output multiple errors, work on some additional things in parallel, etc. I personally think, that "rust-like" format is the ideal one to start with:

error FSXXXX:
1 | let z = x = yyyy
                ^^^^
> Error description...

--------

warning FSXXX:
...
...

We can then try playing with format, characters we use, multi-line errors, etc.

WDYT?

cc @baronfel @T-Gro @dsyme

[baronfel]

(me and @rainersigwald right now)

We're going to be watching what y'all do from the perspective of "what information would a compiler need to give to MSBuild in order for MSBuild to be able to render a message as nicely as this".

[vzarytovskii]

(me and @rainersigwald right now)

We're going to be watching what y'all do from the perspective of "what information would a compiler need to give to MSBuild in order for MSBuild to be able to render a message as nicely as this".

Currently, I'm going to test it with FSI, thought it's applicable to msbuild(fsc) as well.

[edgarfgp]

Great job @muqiuhan and @vzarytovskii. I’m looking forward to see what is next here.

[vzarytovskii]

Happy to merge it if everyone agrees, to start working on some format adjustments

[psfinaki]

These bubbles looks weird to me really - but it's a good move anyway.

[vzarytovskii]

These bubbles looks weird to me really - but it's a good move anyway.

Yea, it's just "make it work" PR

[psfinaki]

Leaving the merge on you here, in case you want anyone else to take a look.