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

System.Text.JSon Source Generator Diagnostic in the error list doesnt link me to docs or navigate me to the file where the issue is #60314

Closed
mikadumont opened this issue Oct 12, 2021 · 15 comments · Fixed by dotnet/docs#27163
Assignees
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@mikadumont
Copy link

mikadumont commented Oct 12, 2021

Description

I am using the System.Text.JSon Source Generator and am getting the following warning in my error list:

image

This analyzer doesn't follow the analyzer guidelines for it doesnt navigate me to the file where the issue is or link me to docs to understand the issue better. Also should the severity be a warning or a suggestion? Anything that will be a warning or error should go through an analyzer review but let me know your thoughts. @jeffhandley @layomia

Reproduction Steps

I am on Version 17.0.0 Preview 5.0 [31807.358.d17.0]

I am using the following code:

using System.Text.Json;
using System.Text.Json.Serialization;

namespace SolarSystem;

internal record Astronomer(string FirstName, string LastName, string? MiddleName = null);

[JsonSerializable(typeof(Astronomer))]
internal partial class AstronomerJsonContext : JsonSerializerContext
{
}

class Program
{
    static void Main(string[] args)
    {
        var astronomer = new Astronomer(FirstName: "Jane", LastName: "Doe");

        string json = JsonSerializer.Serialize(astronomer, AstronomerJsonContext.Default.Astronomer);
    }
}

And I get the following warning in my error list but clicking on the warning doesnt navigate me to where the warning is coming from. Also the analyzer ID link doenst link me to docs.

image

Expected behavior

I am using the following code:

using System.Text.Json;
using System.Text.Json.Serialization;

namespace SolarSystem;

internal record Astronomer(string FirstName, string LastName, string? MiddleName = null);

[JsonSerializable(typeof(Astronomer))]
internal partial class AstronomerJsonContext : JsonSerializerContext
{
}

class Program
{
    static void Main(string[] args)
    {
        var astronomer = new Astronomer(FirstName: "Jane", LastName: "Doe");

        string json = JsonSerializer.Serialize(astronomer, AstronomerJsonContext.Default.Astronomer);
    }
}

And I get the following warning in my error list but clicking on the warning doesnt navigate me to where the warning is coming from. Also the analyzer ID link doenst link me to docs.

image

I would expect clicking on the warning in the error list will navigate me to where the warning is and the clicking on the ID link will take me to docs to understand what the warning is.

Actual behavior

I am using the following code:

using System.Text.Json;
using System.Text.Json.Serialization;

namespace SolarSystem;

internal record Astronomer(string FirstName, string LastName, string? MiddleName = null);

[JsonSerializable(typeof(Astronomer))]
internal partial class AstronomerJsonContext : JsonSerializerContext
{
}

class Program
{
    static void Main(string[] args)
    {
        var astronomer = new Astronomer(FirstName: "Jane", LastName: "Doe");

        string json = JsonSerializer.Serialize(astronomer, AstronomerJsonContext.Default.Astronomer);
    }
}

And I get the following warning in my error list but clicking on the warning doesnt navigate me to where the warning is coming from. Also the analyzer ID link doenst link me to docs.

image

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Oct 12, 2021
@ghost
Copy link

ghost commented Oct 12, 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

Description

I am using the System.Text.JSon Source Generator and am getting the following warning in my error list:

image

This analyzer doesn't follow the analyzer guidelines for it doesnt navigate me to the file where the issue is or link me to docs to understand the issue better. Also should the severity be a warning or a suggestion? Anything that will be a warning or error should go through an analyzer review but let me know your thoughts. @jeffhandley @layomia

Reproduction Steps

I am on Version 17.0.0 Preview 5.0 [31807.358.d17.0]

I am using the following code:

using System.Text.Json;
using System.Text.Json.Serialization;

namespace SolarSystem;

internal record Astronomer(string FirstName, string LastName, string? MiddleName = null);

[JsonSerializable(typeof(Astronomer))]
internal partial class AstronomerJsonContext : JsonSerializerContext
{
}

class Program
{
    static void Main(string[] args)
    {
        var astronomer = new Astronomer(FirstName: "Jane", LastName: "Doe");

        string json = JsonSerializer.Serialize(astronomer, AstronomerJsonContext.Default.Astronomer);
    }
}

And I get the following warning in my error list but clicking on the warning doesnt navigate me to where the warning is coming from. Also the analyzer ID link doenst link me to docs.

image

Expected behavior

I am using the following code:

using System.Text.Json;
using System.Text.Json.Serialization;

namespace SolarSystem;

internal record Astronomer(string FirstName, string LastName, string? MiddleName = null);

[JsonSerializable(typeof(Astronomer))]
internal partial class AstronomerJsonContext : JsonSerializerContext
{
}

class Program
{
    static void Main(string[] args)
    {
        var astronomer = new Astronomer(FirstName: "Jane", LastName: "Doe");

        string json = JsonSerializer.Serialize(astronomer, AstronomerJsonContext.Default.Astronomer);
    }
}

And I get the following warning in my error list but clicking on the warning doesnt navigate me to where the warning is coming from. Also the analyzer ID link doenst link me to docs.

image

I would expect clicking on the warning in the error list will navigate me to where the warning is and the clicking on the ID link will take me to docs to understand what the warning is.

Actual behavior

I am using the following code:

using System.Text.Json;
using System.Text.Json.Serialization;

namespace SolarSystem;

internal record Astronomer(string FirstName, string LastName, string? MiddleName = null);

[JsonSerializable(typeof(Astronomer))]
internal partial class AstronomerJsonContext : JsonSerializerContext
{
}

class Program
{
    static void Main(string[] args)
    {
        var astronomer = new Astronomer(FirstName: "Jane", LastName: "Doe");

        string json = JsonSerializer.Serialize(astronomer, AstronomerJsonContext.Default.Astronomer);
    }
}

And I get the following warning in my error list but clicking on the warning doesnt navigate me to where the warning is coming from. Also the analyzer ID link doenst link me to docs.

image

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: mikadumont
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

I've noticed this as well. Not certain why but I suspect it's because we're hardcoding all diagnostics to Location.None. We would probably need to pass one of the values stored in the underlying ISymbol.Locations property.

@layomia @ericstj should we consider addressing this for 6.0?

@eiriktsarpalis eiriktsarpalis added the enhancement Product code improvement that does NOT require public API changes/additions label Oct 13, 2021
@ericstj
Copy link
Member

ericstj commented Oct 13, 2021

Anything that will be a warning or error should go through an analyzer review but let me know your thoughts. @jeffhandley @layomia

I believe these diagnostics did go through API review, but perhaps there is a gap here in the process. cc @terrajobst as well.

should we consider addressing this for 6.0?

It's a good bug to fix, but it's not blocking. My inclination would be to fix in 7.0, but I don't have a great feel for what the tooling bug bar is around this sort of thing. @mikadumont @mavasani @sharwell what we do if this bug were present in an roslyn-analyzers? Would you take a fix in ask-mode or just fix in the next release?

the analyzer ID link doesn't link me to docs.

We should be able to fix this. @layomia do we have the right documentation for our added diagnostics?

@layomia
Copy link
Contributor

layomia commented Oct 18, 2021

For now I'll consider this a 7.0 issue (early fix) - I'll create PRs providing the right location for the diagnostics we emit, and also prioritize providing corresponding documentation.

@layomia layomia self-assigned this Oct 18, 2021
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Oct 18, 2021
@layomia layomia added this to the 7.0.0 milestone Oct 18, 2021
@jeffhandley
Copy link
Member

Would you take a fix in ask-mode or just fix in the next release?

I don't think this meets the bar for GA Ask Mode, but we've utilized servicing releases to improve the experience of analyzers shipped through roslyn-analyzers.

Generally speaking, changing the diagnostic location of a warning is considered a breaking change though, as existing #pragma statements might become incorrect and a build break could be introduced. In this scenario though, we have both the source generation angle, the fact that the generated code won't work with the diagnostic in place (hinting that perhaps these should be errors?), and the existing diagnostic doesn't have a location, which means #pragma suppressions can't be used here. With all of that, I think this could be considered for a 6.0.x servicing release.

I would recommend a holistic review of the diagnostics that can be generated from the Json source generator feature, illustrating the IDs, locations, messages, severity levels, and URLs that are rendered. From that, changes could be made into 7.0 and considered for backporting into 6.0.x.

@terrajobst
Copy link
Member

Generally speaking, changing the diagnostic location of a warning is considered a breaking change though, as existing #pragma statements might become incorrect and a build break could be introduced.

Agreed. However, I think we can be judicial for the most part. In practice, if we just change the location within a statement it's not likely to affect pragmas as pragmas need to be on their own line and surrounding parts of a statement/expression would render the code fairly unreadable. It can still break some people, but given the probably is low enough I think we could say that this is acceptable after SDK updates.

@zejji
Copy link

zejji commented Oct 22, 2021

Is there a separate issue for actually implementing the ability in the System.Text.Json source generator to deserialize a type which defines init-only properties?

As far as I can tell, this limitation makes it impossible to use the source generator with an API that returns record types (either directly or as a nested property), which pretty much makes it completely unusable for my use case (since I am making heavy use of records). I suspect this issue will also render the source generator dead in the water for most other people who are making use of other recent C# features. However, I would be very keen to watch out for developments in this area, as I'm conscious it's still early days for the implementation.

I'd be grateful if someone could confirm the position and/or link the related issue if there is one.

@eiriktsarpalis
Copy link
Member

@zejji there's #58770. I would recommend upvoting the issue and sharing any feedback there.

@zejji
Copy link

zejji commented Oct 22, 2021

@eiriktsarpalis - Many thanks!

@mikadumont mikadumont changed the title System.Text.JSon Source Generator Diagnostic in the error list is doesnt link me to docs or navigate me to the file where the issue is System.Text.JSon Source Generator Diagnostic in the error list doesnt link me to docs or navigate me to the file where the issue is Oct 28, 2021
@mikadumont
Copy link
Author

@terrajobst received customer feedback on this as well. Copying here:

When there's an error in the generated code make the error go to the generated source code so the user can try to figure out what went wrong. Most of the time I do that and I can understand why there's an issue (followed by a github report).

@mikadumont
Copy link
Author

Additional customer feedback copying here:

Compiler errors reported from generated code without being linked back to user markup / code that caused them.

@terrajobst
Copy link
Member

Yeah, we need to fix this. This seems like a major speed bump.

@eiriktsarpalis
Copy link
Member

@terrajobst you mean it should be serviced for 6?

@terrajobst
Copy link
Member

terrajobst commented Nov 1, 2021

I'd think it would make the bar for SDK servicing, yes.

@layomia
Copy link
Contributor

layomia commented Nov 2, 2021

I would recommend a holistic review of the diagnostics that can be generated from the Json source generator feature, illustrating the IDs, locations, messages, severity levels, and URLs that are rendered. From that, changes could be made into 7.0 and considered for backporting into 6.0.x.

Currently working on putting these artifacts together for review (mostly code changes to return the correct locations).

@ghost ghost locked as resolved and limited conversation to collaborators Jan 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants