unoplatform / uno

Open-source platform for building cross-platform native Mobile, Web, Desktop and Embedded apps quickly. Create rich, C#/XAML, single-codebase apps from any IDE. Hot Reload included! 90m+ NuGet Downloads!!
https://platform.uno
Apache License 2.0
8.94k stars 724 forks source link

Unexpected `System.Exception` thrown by source generator; reported through telemetry #9018

Open sharwell opened 2 years ago

sharwell commented 2 years ago

One or more of the following lines are throwing System.Exception at a location where exceptions are not expected:

https://github.com/unoplatform/uno/blob/574a441497e0e6faee8d389a3dbac1c40d4d7891/src/SourceGenerators/Uno.UI.SourceGenerators/DependencyObject/DependencyPropertyGenerator.cs#L48-L53

This was identified in error telemetry at Microsoft, and I'm happy to take the time to provide additional details here because you've chosen to make your project open source. 👍

I recommend resolving this issue in one of the following ways:

  1. If this issue only occurs when there are other compiler errors present in the user code, it would be acceptable to silently disable the source generator in this case. The source generator would become active again when the user corrects the code.
  2. If this issue can occur where users might not know how to resolve the issue, rather than throw an exception the generator can use the ReportDiagnostic API to report a user-visible error specific to the scenario.
jeromelaban commented 2 years ago

Thanks Sam! Historically, this code was not running inside the compiler (it was through uno.sourcegenerationtasks) and was not telemetry-worthy :)

We'll be migrating to remove those exceptions when running in the roslyn pipeline, so it'll hopefully cleanup your traces over time and provide a better user experience ;)

Thanks again!

Youssef1313 commented 2 years ago

I think I also introduced a similar issue in https://github.com/unoplatform/uno/pull/6760. I did that because throwing was very common in Uno generators so I was blindly consistent.

I expect too many places to be throwing exceptions in generators, as @jeromelaban said.

anpin commented 2 years ago

I think I'm hitting this too in wild now trying to build a WinUI net6.0 sample with Prism https://github.com/PrismLibrary/Prism/pull/2721

CSC warning CS8785: Generator 'DependencyPropertyGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'Exception' with message 'Unable to find Uno.UI.Xaml.GeneratedDependencyPropertyAttribute' [D:\blav-app\modules\Prism\e2e\Uno.WinUI\src\ModuleA\ModuleA.csproj]

binlog -> msbuild.zip

Youssef1313 commented 2 years ago

@anpin I think you need to file a separate issue with a repro.

Youssef1313 commented 1 year ago

For XamlCodeGenerator, we catch exceptions here:

https://github.com/unoplatform/uno/blob/fb32a4c276ea96d4a0d6d4a7cabb731febd5f563/src/SourceGenerators/Uno.UI.SourceGenerators/XamlGenerator/XamlCodeGeneration.cs#L467-L472

and report them as diagnostics:

https://github.com/unoplatform/uno/blob/fb32a4c276ea96d4a0d6d4a7cabb731febd5f563/src/SourceGenerators/Uno.UI.SourceGenerators/XamlGenerator/XamlCodeGeneration.cs#L520-L528

For DependencyPropertyGenerator, this is now an internal generator and was refactored to IIncrementalGenerator. It no longer throws exceptions with the refactoring, I think.

@sharwell Have you recently seen any errors from Uno generators in telemetry? I think this could be already fixed and can be closed. If you are able to confirm, that would be really great!

sharwell commented 1 year ago

I don't have a good way to reverse the process and find out. I'm a bit surprised these exceptions went to telemetry at all. Normally would just be caught and changed to AD0001 like analyzers.

Youssef1313 commented 1 year ago

AD0001 is only used for DiagnosticAnalyzers as far as I know.