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

Nullable reference types #712

Closed
charlesroddie opened this issue Jan 16, 2021 · 20 comments · Fixed by #715
Closed

Nullable reference types #712

charlesroddie opened this issue Jan 16, 2021 · 20 comments · Fixed by #715

Comments

@charlesroddie
Copy link

Optional value-type results of stored procedures are correctly given as Nullable by EFCorePowerTools.
It would be good to have this apply to reference types too, e.g. string? in Result classes.
This would make use of this library much more type-safe.

@ErikEJ
Copy link
Owner

ErikEJ commented Jan 16, 2021

That would require you to support nullable types in your project, and use a new C# version??

@charlesroddie
Copy link
Author

charlesroddie commented Jan 16, 2021

Auto-generated code requires an explicit #nullable directive in source

So users shouldn't need to explicitly enable anything in their project files.

@ErikEJ
Copy link
Owner

ErikEJ commented Jan 16, 2021

But it would still require a new C# version??

@charlesroddie
Copy link
Author

charlesroddie commented Jan 16, 2021

Yes C# 8. For netstandard2.1 and netcore3.0 and above, users won't need to do anything.
For netstandard2.0, users will need to add <LangVersion>8.0</LangVersion> to the .csproj.

@ErikEJ
Copy link
Owner

ErikEJ commented Jan 16, 2021

Ok, so could you give a simple example of the changes required on the generated code?

@charlesroddie
Copy link
Author

charlesroddie commented Jan 16, 2021

A procedure UsersInClass returns an Email(nvarchar, null). Currently this class is created:

public partial class UsersInClassResult
{
    public int Id { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public string Email { get; set; }
    public byte? Role { get; set; }
}

The change would be to add a ? in Email:

    public string? Email { get; set; }

@ErikEJ
Copy link
Owner

ErikEJ commented Jan 16, 2021

Thanks, and the #nullable directive??

@charlesroddie
Copy link
Author

charlesroddie commented Jan 16, 2021

I missed some = null! which you need to remove a warning.

// <auto-generated> This file has been auto generated by EF Core Power Tools. </auto-generated>
#nullable enable
using System;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations.Schema;

namespace EFCorePowerTools
{
    public partial class UsersInClassResult
    {
        public int Id { get; set; }
        public string FirstName { get; set; } = null!;
        public string LastName { get; set; } = null!;
        public string? Email { get; set; }
        public byte? Role { get; set; }
    }
}

@ErikEJ
Copy link
Owner

ErikEJ commented Jan 16, 2021

Hmm what is the code after FirstName, is byte not a value type??

@charlesroddie
Copy link
Author

charlesroddie commented Jan 16, 2021

If you have

public string FirstName { get; set; }

then you are saying that FirstName is not null, but there is no non-null value it's being initialized to, and = null! is a way to avoid the warning. You are handling the guarantees of non-nullness manually and some other code will presumably set a non-null value prior to the user seeing it. I think it's the way to deal with traditional C# classes with default values and setters. Anything wrong with my C# @Happypig375 ?

is byte not a value type

Yes the byte? is unchanged.

@ErikEJ
Copy link
Owner

ErikEJ commented Jan 16, 2021

How would the code generation tool distinguish between FirstName/LastName and Email??? 😕

They are all strings ???

    public string FirstName { get; set; } = null!;
    public string LastName { get; set; } = null!;
    public string? Email { get; set; }

@charlesroddie
Copy link
Author

Presumably it gets info on nullability from SQL. It's certainly getting it correctly for value types at present (e.g. in the above, Id(int, non null) is an int, while Role(tinyint, null), is a byte?).

@ErikEJ
Copy link
Owner

ErikEJ commented Jan 16, 2021

Sorry, I was mixing parameters and result set shape.

So for a non-nullable string (and other reference types) , generate:

public string LastName { get; set; } = default;

and for a nullable string (and other reference types), generate

public string? Email { get; set; }

Is that correctly understood?

dotnet/efcore#15520

@ErikEJ
Copy link
Owner

ErikEJ commented Jan 16, 2021

OK, interested in doing a PR?

@charlesroddie
Copy link
Author

Currently I'm investigating 3 options for improving our database process (EFCorePowerTools / FSharp.Data.Sqlclient / SqlPlus.net). If we decide on EFCorePowerTools I can do a PR but if not I will not be able to.

@ErikEJ
Copy link
Owner

ErikEJ commented Jan 16, 2021

No worries, I will make the proposed change!

@Happypig375
Copy link

@charlesroddie All good.

@ErikEJ
Copy link
Owner

ErikEJ commented Jan 17, 2021

I plan to implement this as an advanced option initially.

ErikEJ added a commit that referenced this issue Jan 19, 2021
…ure result sets

fixes #712

Also fix NRE in Handler
ErikEJ added a commit that referenced this issue Jan 19, 2021
…ure result sets (#715)

fixes #712

Also fix NRE in Handler
@stgenov
Copy link

stgenov commented Dec 9, 2021

Please add a row
#nullable enable
directive after the row
because without it VS 2022 (.net Core 6.0) gives a warning:
CS8669: The annotation for nullable reference types should only be used in code within a '#nullable' annotations context. Auto-generated code requires an explicit '#nullable' directive in source.

the warning appears on every row with string?

@ErikEJ
Copy link
Owner

ErikEJ commented Dec 9, 2021

@stgenov pls create an new issue with details about your csproj.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants