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

Issue diagnostic warnings when custom JsonSerializerContext types don't indicate serializable types #58698

Open
Tracked by #77019
Jericho opened this issue Sep 5, 2021 · 9 comments
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors source-generator Indicates an issue with a source generator feature
Milestone

Comments

@Jericho
Copy link

Jericho commented Sep 5, 2021

When my custom serialiazer context class has zero JsonSerializable attributes, I get an error message which does not help me understand what the problem is.

I simply get the dreaded:

CS0534	'MyJsonSerializerContext' does not implement inherited abstract member 'JsonSerializerContext.GetTypeInfo(Type)'

I do not get any additional information to help me realize that I forgot to add JsonSerializable attributes to my custom class. It would be extremely helpful to get an error message similar to Your custom serializer context class must have at least one JsonSerializable attribute.

Repro:

public static class Program
{
    public static void Main()
    {
        Console.WriteLine("Hello World!");
    }
}

// Notice there are no JsonSerializable attributes here.
internal partial class MyJsonSerializerContext : JsonSerializerContext
{
}

As an aside: Is there a way to get more detailed information when something goes wrong with the code generator? Does it generate a log somewhere?

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Sep 5, 2021
@Jericho
Copy link
Author

Jericho commented Sep 5, 2021

By the way, I want to be completely clear that I fully understand that a custom serializer context without any JsonSerializable is not a valid scenario. I am not saying that the code generator should accept this situation. However, I think that a better/more descriptive error message would be helpful and would allow developers to resolve the situation easily.

@layomia
Copy link
Contributor

layomia commented Sep 7, 2021

Yes we can log a diagnostic error for this scenario.

As an aside: Is there a way to get more detailed information when something goes wrong with the code generator? Does it generate a log somewhere?

Yes we log diagnostic errors for other scenarios, e.g. when a custom context class or one of its enclosing types is not partial.

@layomia layomia added this to the 6.0.0 milestone Sep 7, 2021
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Sep 7, 2021
@layomia layomia self-assigned this Sep 7, 2021
@Jericho
Copy link
Author

Jericho commented Sep 7, 2021

Yes we log diagnostic errors for other scenarios

How do I access this log?

@layomia
Copy link
Contributor

layomia commented Sep 7, 2021

It's part of the error log shown during the build (visible both in command line & VS), just like the CS0534 error you're seeing above.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 7, 2021
@Jericho
Copy link
Author

Jericho commented Sep 9, 2021

@layomia The reason I asked about a more detailed log is because CS0534 is a very generic message that, unfortunately, does not give us any clue about what caused the problem. Consider this: let's say I was to raise an issue and said "I'm getting CS0534, help me figure out what's going on", I'm sure you would tell me that I haven't provided enough information.

So, let me rephrase my question: how can we find more detailed information when we get generic errors like CS0534?

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 10, 2021
@layomia
Copy link
Contributor

layomia commented Sep 10, 2021

When you get CS0534 when using the source generator, it means that there's another underlying issue that caused the generator not to emit the expected code. The generator emits various diagnostics which explain failure reasons, but it doesn't do it in all cases as this issue shows. We just need to emit a diagnostic for this scenario like proposed in #58768.

A fix for this doesn't meet the bar to be ported to 6.0, so I'll move this issue to future.

@layomia layomia modified the milestones: 6.0.0, 7.0.0 Sep 10, 2021
@layomia layomia removed their assignment Sep 10, 2021
@layomia layomia added the source-generator Indicates an issue with a source generator feature label Sep 22, 2021
@eiriktsarpalis eiriktsarpalis added the Priority:3 Work that is nice to have label Oct 15, 2021
@jeffhandley jeffhandley modified the milestones: 7.0.0, Future Jul 10, 2022
@jeffhandley jeffhandley removed the Priority:3 Work that is nice to have label Jul 10, 2022
@eiriktsarpalis eiriktsarpalis added the enhancement Product code improvement that does NOT require public API changes/additions label Sep 2, 2022
@layomia layomia changed the title System.Text.Json code gen: unhelpful error when custom serializer context is not decorated with any JsonSerializable Issue diagnostic warnings when custom JsonSerializerContext types don't indicate serializable types Dec 2, 2022
@layomia
Copy link
Contributor

layomia commented Dec 2, 2022

Up for grabs. #58768 attempted to fix this for 6.0 but we ran out of time. Note there'll be some perf testing to make sure we don't regress typing performance for large VS solutions.

@layomia layomia added the help wanted [up-for-grabs] Good issue for external contributors label Dec 2, 2022
@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 8.0.0 Jun 7, 2023
@eiriktsarpalis
Copy link
Member

We also need to make sure this diagnostic doesn't trigger for classes that provide an explicit override for the GetTypeInfo method. No doubt many users do this so this could trigger a source breaking change if we don't account for that case.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jul 5, 2023

Moving to future for now as we figure out a way to do this without compromising editor performance or triggering false positive diagnostics.

@eiriktsarpalis eiriktsarpalis modified the milestones: 8.0.0, Future Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors source-generator Indicates an issue with a source generator feature
Projects
None yet
4 participants