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

Provide locations for src-gen diagnostic output #61430

Merged
merged 3 commits into from
Nov 16, 2021

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Nov 10, 2021

Contributes to #60314. Next PR to fix the issue will be in dotnet/docs to provide more info on the docs website.

@layomia layomia added this to the 6.0.0 milestone Nov 10, 2021
@layomia layomia self-assigned this Nov 10, 2021
@ghost
Copy link

ghost commented Nov 10, 2021

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

Issue Details

Contributes to #60314.

Author: layomia
Assignees: layomia
Labels:

area-System.Text.Json

Milestone: 6.0.0

@layomia layomia force-pushed the Diagnostics branch 2 times, most recently from fd59059 to 1ac8c8d Compare November 11, 2021 00:08
public static Location? GetDiagnosticLocation(this PropertyInfo propertyInfo)
{
PropertyInfoWrapper? propertyInfoWrapper = propertyInfo as PropertyInfoWrapper;
Debug.Assert(propertyInfoWrapper != null);
Copy link
Member

Choose a reason for hiding this comment

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

If this invariant is always true, it begs the question why we need all the System.Reflection wrappers for Roslyn symbols. Not in scope for this PR, but in 7.0.0 I would recommend mapping roslyn symbols directly to sourcegen metadata to avoid abstraction leaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a few cases, we are able to share code between source-gen and reflection serializers since the symbols are expressed as reflection wrappers, e.g. the code in https://github.com/dotnet/runtime/blob/4cf86c2abfe9b2fad5ab2a48e4b0717ff0daf27a/src/libraries/System.Text.Json/Common/ReflectionExtensions.cs

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

LGTM, other than a few issues needing attention.

@layomia
Copy link
Contributor Author

layomia commented Nov 15, 2021

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1463647226

@github-actions
Copy link
Contributor

@layomia backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Provide locations for src-gen diagnostic output
Using index info to reconstruct a base tree...
M	src/libraries/System.Text.Json/gen/TypeGenerationSpec.cs
M	src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorDiagnosticsTests.cs
M	src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorTests.cs
Auto-merging src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorDiagnosticsTests.cs
Auto-merging src/libraries/System.Text.Json/gen/TypeGenerationSpec.cs
CONFLICT (content): Merge conflict in src/libraries/System.Text.Json/gen/TypeGenerationSpec.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Provide locations for src-gen diagnostic output
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants