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

Treat constructor arguments as required properties #78151

Open
krwq opened this issue Nov 10, 2022 · 5 comments
Open

Treat constructor arguments as required properties #78151

krwq opened this issue Nov 10, 2022 · 5 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json
Milestone

Comments

@krwq
Copy link
Member

krwq commented Nov 10, 2022

Compiler required keyword semantics

private class Test
{
    public required string Name { get; } // error

    public Test(string name)
    {
        Name = name;
    }
}

don't allow to compile above code because property doesn't have a setter - this makes sense because argument needs to be passed in the constructor anyway making required effectively redundant.

On the other hand when deserializing JSON Name does not have to be passed in the payload even though the only way to construct is through constructor - we will use default value (i.e. if value was int we'd pass 0 or null for string for you automatically):

var obj = System.Text.Json.JsonSerializer.Deserialize<Test>("{}"); // no error

public sealed class Test
{
    public string Name { get; }

    public Test(string name)
    {
        Name = name;
    }
}

Consider changing semantics here and make Name required instead making above code create error. Possibly there should be an option switch to opt-in to suggested behavior given this change will be breaking.

Example API suggestion (possibly consider enum and more generic name in case we need more related options in the future):

public partial class JsonSerializerOptions
{
    public bool RequireConstructorParameters { get; set; }
}

Related: #78098

@krwq krwq added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 10, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 10, 2022
@ghost
Copy link

ghost commented Nov 10, 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

Compiler required keyword semantics

private class Test
{
    public required string Name { get; } // error

    public Test(string name)
    {
        Name = name;
    }
}

don't allow to compile above code because property doesn't have a setter - this makes sense because argument needs to be passed in the constructor anyway making required effectively redundant.

On the other hand when deserializing JSON Name does not have to be passed in the payload even though the only way to construct is through constructor - we will use default value (i.e. if value was int we'd pass 0 or null for string for you automatically):

var obj = System.Text.Json.JsonSerializer.Deserialize<Test>("{}"); // no error

public sealed class Test
{
    public string Name { get; }

    public Test(string name)
    {
        Name = name;
    }
}

Consider changing semantics here and make Name required instead making above code create error. Possibly there should be an option switch to opt-in to suggested behavior given this change will be breaking.

Example API suggestion (possibly consider enum and more generic name in case we need more related options in the future):

public partial class JsonSerializerOptions
{
    public RequireConstructorParameters { get; set; }
}

Related: #78098

Author: krwq
Assignees: -
Labels:

api-suggestion, area-System.Text.Json

Milestone: -

@krwq krwq changed the title Treat constructor arguments as required properties by default Treat constructor arguments as required properties Nov 10, 2022
@krwq
Copy link
Member Author

krwq commented Nov 10, 2022

alternative design for that would be to provide Modifier for contract resolver but it's currently tricky to implement as a separate library because we don't have parametrized ctor APIs available yet (so basically user has to repeat JsonConstructor logic).

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Nov 10, 2022

Proposal looks good to me. We should also include provision for configuring requiredness on the type level:

public sealed class Test
{
    [JsonConstructor(RequireParameters = true)]
    public Test(int p1, int p2, int p3, int p4, ...)
    {
    }
}

and at the individual parameter level:

public sealed class Test
{
    public Test(int p1, [JsonRequired] int p2, int p3, int p4, ...)
    {
    }
}

@krwq
Copy link
Member Author

krwq commented Nov 10, 2022

Extra overload of JsonConstructorAttribute sound good but for JsonRequired - we put [JsonConverter(...)] and [JsonNumberHandling(...)] directly on the property so I'm not sure why we'd do it differently for [JsonRequired]...

@krwq krwq added this to the 8.0.0 milestone Nov 23, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Nov 23, 2022
@krwq
Copy link
Member Author

krwq commented Nov 23, 2022

marking as 8.0 because we should look at this in conjunction with other 8.0 items.

@krwq krwq modified the milestones: 8.0.0, Future Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json
Projects
None yet
Development

No branches or pull requests

2 participants