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

Refactor how diagnostics work in the interop generators #87700

Merged
merged 10 commits into from
Jun 22, 2023

Conversation

jkoritzinsky
Copy link
Member

Update the diagnostics system to have the following characteristics:

  • Well known categories of diagnostics, such as "Not supported" or "Unnecessary data" are represented by a discriminated union.
  • Each generator provides diagnostic descriptors that fit specific formatting rules for each check (which all of our generators follow)
  • Slightly abstract out the concept of "diagnostic" locations so it can be shared better.
  • Split "create diagnostic" from "report diagnostic" to make it easier to share code.
  • Remove our exception type and move to a "generator and a collection of possibly fatal diagnostics" model to enable representing diagnostics that shouldn't stop source-generated interop codegen.

Update the diagnostics system to have the following characteristics:

- Well known categories of diagnostics, such as "Not supported" or "Unnecessary data" are represented by a discriminated union.
- Each generator provides diagnostic descriptors that fit specific formatting rules for each check (which all of our generators follow)
- Slightly abstract out the concept of "diagnostic" locations so it can be shared better.
- Split "create diagnostic" from "report diagnostic" to make it easier to share code.
- Remove our exception type and move to a "generator and a collection of possibly fatal diagnostics" model to enable representing diagnostics that shouldn't stop source-generated interop codegen.
@jkoritzinsky jkoritzinsky added area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature labels Jun 16, 2023
@jkoritzinsky jkoritzinsky added this to the 8.0.0 milestone Jun 16, 2023
@ghost ghost assigned jkoritzinsky Jun 16, 2023
@ghost
Copy link

ghost commented Jun 16, 2023

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

Update the diagnostics system to have the following characteristics:

  • Well known categories of diagnostics, such as "Not supported" or "Unnecessary data" are represented by a discriminated union.
  • Each generator provides diagnostic descriptors that fit specific formatting rules for each check (which all of our generators follow)
  • Slightly abstract out the concept of "diagnostic" locations so it can be shared better.
  • Split "create diagnostic" from "report diagnostic" to make it easier to share code.
  • Remove our exception type and move to a "generator and a collection of possibly fatal diagnostics" model to enable representing diagnostics that shouldn't stop source-generated interop codegen.
Author: jkoritzinsky
Assignees: -
Labels:

area-System.Runtime.InteropServices, source-generator

Milestone: 8.0.0

@jkoritzinsky
Copy link
Member Author

Guess it's time to solve the dependency issue (consuming older generators with some live built componenets also included)

@jtschuster
Copy link
Member

Guess it's time to solve the dependency issue (consuming older generators with some live built componenets also included)

If we don't pass any types from Microsoft.Interop.SourceGeneration between LibraryImportGenerator and ComInterfaceGenerator, we might be able to make the Microsoft.Interop.SourceGeneration project a shared project? Though that would make each assembly much larger.

@jkoritzinsky
Copy link
Member Author

We want to eventually ship Microsoft.Interop.SourceGeneration as its own assembly/NuGet package, so we don't want to include it as source in the dependent projects.

Also, we should solve these conflict issues anyway instead of continuing to punt them. If any of the task assemblies were actually using the generators, we would have pretty rough build breaks as both copies of the generator would try to generate code.

…e condition, as this best describes what we actually want to check.
@jkoritzinsky jkoritzinsky merged commit 73f96e6 into dotnet:main Jun 22, 2023
111 checks passed
@jkoritzinsky jkoritzinsky deleted the diagnostic-refactor branch June 22, 2023 23:02
@am11
Copy link
Member

am11 commented Jun 24, 2023

@jkoritzinsky, FYI, noticed a failure in a mono outerloop test:

$ ./build.sh mono+libs -keepnativesymbols true
$ pushd src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests
$ ~/projects/runtime/dotnet.sh build -t:Test -p:OuterLoop=true \
    -p:XunitMethodName=LibraryImportGenerator.UnitTests.AttributeForwarding.InOutAttributes_Forwarded_To_ForwardedParameter
...
    Discovering: LibraryImportGenerator.Unit.Tests (method display = ClassAndMethod, method display options = None)
    Discovered:  LibraryImportGenerator.Unit.Tests (found 1 of 187 test case)
    Starting:    LibraryImportGenerator.Unit.Tests (parallel test collections = on, max threads = 10)
      LibraryImportGenerator.UnitTests.AttributeForwarding.InOutAttributes_Forwarded_To_ForwardedParameter [FAIL]
        Assert.Collection() Failure
        Collection: [int __a_native]
        Error during comparison of item at index 0
        Inner exception: Assert.Collection() Failure
                Collection: []
                Expected item count: 2
                Actual item count:   0
        Stack Trace:
          /Users/am11/projects/runtime/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/AttributeForwarding.cs(359,0): at LibraryImportGenerator.UnitTests.AttributeForwarding.<>c__DisplayClass6_0.<InOutAttributes_Forwarded_To_ForwardedParameter>b__1(IParameterSymbol param)
          /Users/am11/projects/runtime/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/AttributeForwarding.cs(358,0): at LibraryImportGenerator.UnitTests.AttributeForwarding.<>c.<InOutAttributes_Forwarded_To_ForwardedParameter>b__6_0(IMethodSymbol targetMethod, Compilation newComp)
          /Users/am11/projects/runtime/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/AttributeForwarding.cs(445,0): at LibraryImportGenerator.UnitTests.AttributeForwarding.GeneratedTargetPInvokeTest.VerifyFinalCompilation(Compilation compilation)
          /Users/am11/projects/runtime/src/libraries/System.Runtime.InteropServices/tests/Common/Verifiers/CSharpSourceGeneratorVerifier.cs(154,0): at Microsoft.Interop.UnitTests.Verifiers.CSharpSourceGeneratorVerifier`1.Test.<GetProjectCompilationAsync>d__4[[Microsoft.Interop.LibraryImportGenerator, Microsoft.Interop.LibraryImportGenerator, Version=42.42.42.42, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]].MoveNext()
          /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs(1117,0): at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.<GetSortedDiagnosticsAsync>d__82[[Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier, Microsoft.CodeAnalysis.Testing.Verifiers.XUnit, Version=1.1.2.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35]].MoveNext()
          /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs(1092,0): at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.<GetSortedDiagnosticsAsync>d__81[[Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier, Microsoft.CodeAnalysis.Testing.Verifiers.XUnit, Version=1.1.2.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35]].MoveNext()
          /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs(466,0): at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.<VerifyDiagnosticsAsync>d__69[[Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier, Microsoft.CodeAnalysis.Testing.Verifiers.XUnit, Version=1.1.2.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35]].MoveNext()
          /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs(219,0): at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.<RunImplAsync>d__64[[Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier, Microsoft.CodeAnalysis.Testing.Verifiers.XUnit, Version=1.1.2.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35]].MoveNext()
          /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs(188,0): at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.<RunAsync>d__63[[Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier, Microsoft.CodeAnalysis.Testing.Verifiers.XUnit, Version=1.1.2.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35]].MoveNext()
          /Users/am11/projects/runtime/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/AttributeForwarding.cs(351,0): at LibraryImportGenerator.UnitTests.AttributeForwarding.InOutAttributes_Forwarded_To_ForwardedParameter()
          --- End of stack trace from previous location ---
    Finished:    LibraryImportGenerator.Unit.Tests
  === TEST EXECUTION SUMMARY ===
     LibraryImportGenerator.Unit.Tests  Total: 1, Errors: 0, Failed: 1, Skipped: 0, Time: 2.612s

(was passing until the previous commit on main: 82ed3dc)

@jkoritzinsky
Copy link
Member Author

Opened an issue to track. I'll take a look next week.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants