Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XAML warnings/diagnostics support #13447

Merged
merged 27 commits into from
Nov 17, 2023
Merged

XAML warnings/diagnostics support #13447

merged 27 commits into from
Nov 17, 2023

Conversation

maxkatz6
Copy link
Member

@maxkatz6 maxkatz6 commented Oct 31, 2023

What does the pull request do?

  1. Support for multiple reported errors. Previously single binding/markupextension/other error would stop whole build at once. Note, XML parsing errors are still fatal for the build. As well as Emit errors. It's planned to support multiple XML parsing errors though, in the future.
  2. It is possible to report warnings that can be collected by the IDE or runtime xaml loader (via RuntimeXamlLoaderConfiguration.DiagnosticHandler API).
  3. It is possible to override diagnostic severity using TreatWarningsAsErrors, WarningsAsErrors,
    WarningsNotAsErrors, NoWarn, EditorConfig files (limited). Or in case of runtime loader - using the same RuntimeXamlLoaderConfiguration.DiagnosticHandler. Note, each diagnostic has default and min severity - you can't mark any Error as a Warning, but you can do in the opposite way.
  4. Special handling for Bindings, Selectors/Setters and intrinsic (both XAML standard like x:Static and Avalonia parsers).
  5. Added Obsolete warnings and errors support.
  6. Standartized avalonia diagnostic codes for a bit.
  7. Added "AvaloniaXamlVerboseExceptions" msbuild property. Previously any unhandled exception would log full exception info with stracktrace. Now it's disabled by default.

What is the current behavior?

Poor.

How was the solution implemented (if it's not obvious)?

There are a couple of important moments:

  1. Compiler and transformation context now have a diagnostics handler built in, which can be used to manually report specific errors/warnings and recover gracefully.
  2. Throwing an exception from the transformer is still supported and is still used almost everywhere. When any transformer fails, visitor will return ISkipXamlAstNode, marking the node to be ignored from further processing, so we won't multiply errors count.
  3. Compiler tries to avoid unnecessary errors caused by not crashing early, but there more work needs to be done to perfect this handling, so it is still possible to have a butterfly effect of unhandled errors.
  4. In MSBuild Task there is a limit of 100 diagnostics of any type per file.

Checklist

Breaking changes

Different diagnostic codes, but it's hardly a breaking change, as it wasn't really structured.

Fixed issues

Fixes #5842
Fixes #7461
Re-fixes #9612 (as a warning instead of an error)
Partially #4466

@maxkatz6
Copy link
Member Author

Depends on kekekeks/XamlX#99


namespace Avalonia.Markup.Xaml.XamlIl.CompilerExtensions;

internal static class AvaloniaXamlDiagnosticCodes
Copy link
Member Author

@maxkatz6 maxkatz6 Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to have unique diagnostic codes per each error? Currently, it's grouped by area and ability to lower severity, i.e. two different selector parse errors will have the same code - AVLN2201 - right now.

&& clrProperty.DeclaringType == context.GetAvaloniaTypes().ResourceDictionary)
{
var nodeName = on.Type.GetClrType().Name;
context.ReportDiagnostic(new XamlDiagnostic(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example of Avalonia-specific XAML warning.

@maxkatz6 maxkatz6 force-pushed the xaml-warnings-support branch from 5d7707a to d052b91 Compare November 2, 2023 05:22
@maxkatz6 maxkatz6 force-pushed the xaml-warnings-support branch from 0b30812 to 92b3b05 Compare November 2, 2023 05:38
@maxkatz6 maxkatz6 force-pushed the xaml-warnings-support branch from 7e3cc0a to dba33dd Compare November 2, 2023 05:52
@maxkatz6 maxkatz6 force-pushed the xaml-warnings-support branch from d62961f to 51680e4 Compare November 2, 2023 06:23
@maxkatz6 maxkatz6 marked this pull request as ready for review November 2, 2023 06:31
@maxkatz6
Copy link
Member Author

maxkatz6 commented Nov 2, 2023

If anybody has some time to test this PR, it would help. Basically, it should be able to report most XAML errors, while not regressing anything.

@maxkatz6 maxkatz6 requested review from kekekeks and grokys November 2, 2023 06:33
@MrJul
Copy link
Member

MrJul commented Nov 6, 2023

I tested this for a while and can see multiple errors being properly reported and MSBuild error properties being respected.
This seems to work nicely!

Here's a few things I've noticed:

Missing error location

The file name and position where an error occurs don't seem to be properly reported anymore:

Before this PR:

1>C:\Dev\Avalonia\samples\Sandbox\MainWindow.axaml(8,6,8,6): Avalonia error AVLN: 0004: XamlX.XamlParseException: Duplicate setter encountered for property 'Height' Line 8, position 6. [C:\Dev\Avalonia\samples\Sandbox\Sandbox.csproj]
1>C:\Dev\Avalonia\samples\Sandbox\MainWindow.axaml(8,6,8,6): Avalonia error AVLN: 0004: at Avalonia.Markup.Xaml.XamlIl.CompilerExtensions.Transformers.AvaloniaXamlIlDuplicateSettersChecker.Transform(AstTransformationContext context, IXamlAstNode node) in C:\Dev\Avalonia\src\Markup\Avalonia.Markup.Xaml.Loader\CompilerExtensions\Transformers\AvaloniaXamlIlDuplicateSettersChecker.cs:line 39 [C:\Dev\Avalonia\samples\Sandbox\Sandbox.csproj]
1>C:\Dev\Avalonia\samples\Sandbox\MainWindow.axaml(8,6,8,6): Avalonia error AVLN: 0004: at XamlX.Transform.AstTransformationContext.Visitor.Visit(IXamlAstNode node) in C:\Dev\Avalonia\src\Markup\Avalonia.Markup.Xaml.Loader\xamlil.github\src\XamlX\Transform\AstTransformationContext.cs:line 58 [C:\Dev\Avalonia\samples\Sandbox\Sandbox.csproj]
[...]

With Rider/VS properly taking me to the error location.

After this PR:

28>MSBUILD : Avalonia error AVLN2203: Duplicate setter encountered for property 'Height' [C:\Dev\Avalonia\samples\Sandbox\Sandbox.csproj]

Notice the lack of MainWindow.axaml(8,6,8,6)

Multiple obsolete diagnostics

An obsolete usage is reported as two warnings/errors (probably because it's two lines):

23>MSBUILD: Error AVLN5001 Avalonia: 'SomeObsoleteControl' is obsolete [C:\Dev\Avalonia\samples\Sandbox\Sandbox.csproj]
23>MSBUILD: Error AVLN5001 Avalonia: don't use me! [C:\Dev\Avalonia\samples\Sandbox\Sandbox.csproj]

In contrast, the C# compiler reports:

Warning CS0618 : 'SomeObsoleteControl' is obsolete: 'don't use me!'

Duplicate unexpected errors

Note that while known errors don't have a strack trace anymore, unexpected ones still do, effectively reporting each line as a different error.
For example:

21>MSBUILD: Error AVLN0000 Avalonia: The input string 'mm' was not in a correct format. [C:\Dev\Avalonia\samples\Sandbox\Sandbox.csproj]
21>MSBUILD: Error AVLN0000 Avalonia: System.FormatException: The input string 'mm' was not in a correct format. [C:\Dev\Avalonia\samples\Sandbox\Sandbox.csproj]
21>MSBUILD: Error AVLN0000 Avalonia: at System.Number.ThrowOverflowOrFormatException(ParsingStatus status, ReadOnlySpan`1 value, TypeCode type) [C:\Dev\Avalonia\samples\Sandbox\Sandbox.csproj]
21>MSBUILD: Error AVLN0000 Avalonia: at System.Double.Parse(String s, IFormatProvider provider) [C:\Dev\Avalonia\samples\Sandbox\Sandbox.csproj]
[...]

However that was already the case before this PR, so it' just a thing to note for later fixing.

EditorConfig handling

As noted in the code, EditorConfig parsing is very basic, using regexes, so specified file globs are ignored.
Could we use https://github.com/editorconfig/editorconfig-core-net/ instead to properly parse them, and match the globs?

Note that this library targets .NET Standard 1.3, doesn't have any dependency, and is MIT-licensed, so we should be able to ILMerge into Avalonia.Build.Tasks without issues.

@maxkatz6
Copy link
Member Author

maxkatz6 commented Nov 7, 2023

Missing error location

Interesting. Will take a look. It should be reported correctly.
Upd: fixed

Multiple obsolete diagnostics

Looks like MSBuild doesn't handle multiline errors as an error with description. Instead it splits it into multiple errors/warnings. Need to research on how to correctly add description to the error/warning.
Upd: fixed

Duplicate unexpected errors

In general, we can't completely avoid "butterfly effect" of one error producing more errors. But in your example I see something that should be avoided (basically the same parsing error reported multiple time) + AVLN0000 code is wrong there. Will check it.
Upd: fixed, added AvaloniaXamlVerboseExceptions msbuild property

EditorConfig handling

Yes, I will try to ilmerge it.
Upd: it seems EditorConfig library is pretty...unusable. It tried to read file system folders by itself, and doesn't provide any way to pass our own editor config files. And we need it with MSBuild, as it generates some editorconfig files as well.

@workgroupengineering
Copy link
Contributor

Great work.

A feature I would like to have would be the ability to write an analyzer like the Roslyn analyzers, which can be plugged into the compilation pipeline via nuget.

Everyone could write their own rules based on company policy.

@maxkatz6
Copy link
Member Author

maxkatz6 commented Nov 7, 2023

@workgroupengineering I don't think we will have plugin/analyzer infrastructure soon. We don't even have an immutable nodes tree that we can safely send to the third-party analyzer. So, for now, the best bet would be to implement these analyzers as a part of the compiler.

@maxkatz6
Copy link
Member Author

maxkatz6 commented Nov 8, 2023

@MrJul I am going to leave EditorConfig improvements out of the scope of this PR for now. Other issues should be solved.

@MrJul
Copy link
Member

MrJul commented Nov 8, 2023

Tested again, everything works fine (with AvaloniaXamlVerboseExceptions=false for the last issue).

Let's get back to the EditorConfig issue later, once your PR there is hopefully merged :)

@jmacato jmacato enabled auto-merge November 17, 2023 07:19
@jmacato jmacato added this pull request to the merge queue Nov 17, 2023
Merged via the queue into master with commit ea64505 Nov 17, 2023
@jmacato jmacato deleted the xaml-warnings-support branch November 17, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants