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 crashes when processing unresolved symbols #68072

Closed
Tracked by #63918
eiriktsarpalis opened this issue Apr 15, 2022 · 7 comments · Fixed by #68828
Closed
Tracked by #63918

System.Text.Json source generator crashes when processing unresolved symbols #68072

eiriktsarpalis opened this issue Apr 15, 2022 · 7 comments · Fixed by #68828
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions Priority:3 Work that is nice to have
Milestone

Comments

@eiriktsarpalis
Copy link
Member

Description

The implementation of System.Text.Json source generation, in particular the mapping layer between Roslyn and System.Reflection types does not seem to handle unresolved symbols particularly well, resulting in crashes when they do turn up.

Reproduction Steps

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

JsonSerializer.Serialize(new MyPoco(), MyContext.Default.MyPoco);

public class MyPoco
{
    //[UnresolvedProperty] // uncomment to trigger crash
    public int Prop { get; set; }
}

[JsonSerializable(typeof(MyPoco))]
public partial class MyContext : JsonSerializerContext
{ 

}

Expected behavior

Should fail with a relevant error message.

Actual behavior

Crashes with NullReferenceException:

CSC : warning CS8785: Generator 'JsonSourceGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'NullReferenceException' with message 'Object reference not set to an instance of an object.'

Regression?

No.

Known Workarounds

No response

Configuration

No response

Other information

The particular error message occurs because we are feeding a null AttributeConstructor value to the ConstructorInfoWrapper type, which provides a mapping layer between IMethodSymbol and System.Reflection.ConstructorInfo.

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

ghost commented Apr 15, 2022

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

Issue Details

Description

The implementation of System.Text.Json source generation, in particular the mapping layer between Roslyn and System.Reflection types does not seem to handle unresolved symbols particularly well, resulting in crashes when they do turn up.

Reproduction Steps

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

JsonSerializer.Serialize(new MyPoco(), MyContext.Default.MyPoco);

public class MyPoco
{
    //[UnresolvedProperty] // uncomment to trigger crash
    public int Prop { get; set; }
}

[JsonSerializable(typeof(MyPoco))]
public partial class MyContext : JsonSerializerContext
{ 

}

Expected behavior

Should fail with a relevant error message.

Actual behavior

Crashes with NullReferenceException:

CSC : warning CS8785: Generator 'JsonSourceGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'NullReferenceException' with message 'Object reference not set to an instance of an object.'

Regression?

No.

Known Workarounds

No response

Configuration

No response

Other information

The particular error message occurs because we are feeding a null AttributeConstructor value to the ConstructorInfoWrapper type, which provides a mapping layer between IMethodSymbol and System.Reflection.ConstructorInfo.

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Apr 15, 2022
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Apr 15, 2022
@eiriktsarpalis eiriktsarpalis added enhancement Product code improvement that does NOT require public API changes/additions Priority:3 Work that is nice to have labels Apr 15, 2022
@ericstj
Copy link
Member

ericstj commented Apr 15, 2022

Should fail with a relevant error message.

It's possible we might just want to do nothing in the case someone has customized their build to this point. If we can count on real compiler errors for unresolved symbols those are better than the source generator complaining it can't find types. Perhaps the only time we'd want the source generator to raise errors was when we were pretty certain that the build would otherwise be successful and the user might be actually trying to use JSON source generator (eg: has a System.Text.Json reference).

@eiriktsarpalis
Copy link
Member Author

In principle I agree, but I think we should at least try to exit gracefully when things don't type check. I'm not sure what prior art or best practices exist in the world of source generators.

@B1Z0N
Copy link
Contributor

B1Z0N commented Apr 16, 2022

Hi! I've tried googling UnresolvedPropertyAttribute, searching it both in this repo and in roslyn repo.

Where it comes from? Please, hint :)

@huoyaoyuan
Copy link
Member

Hi! I've tried googling UnresolvedPropertyAttribute, searching it both in this repo and in roslyn repo.

As its name hits, it comes nowhere. It triggers a compiling error because the name is undefined.

A source generator should handle this gracefully, instead of crashing.

@B1Z0N
Copy link
Contributor

B1Z0N commented Apr 18, 2022

Ahahahah, awkward moment 😀

@eiriktsarpalis
Copy link
Member Author

Interestingly, I came across the same exception when debugging the Roslyn analysis service for #68353. It seems to be failing when processing this attribute:

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 3, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 5, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 4, 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 Priority:3 Work that is nice to have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants