Skip to content

Commit

Permalink
fixes in no nullable
Browse files Browse the repository at this point in the history
  • Loading branch information
@olmobrutall
olmobrutall committed Feb 16, 2019
1 parent add8d37 commit 28c71a1
Show file tree
Hide file tree
Showing 3 changed files with 5 additions and 14 deletions.
1 change: 0 additions & 1 deletion Signum.Engine/Operations/Graph.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,6 @@ public class Execute : _Execute<T>, IExecuteOperation
Type? IOperation.ReturnType { get { return null; } }
Type? IOperation.StateType { get { return null; } }
public bool AvoidImplicitSave { get; set; }
public bool AvoidImplicitSave { get; set; }

Type IEntityOperation.BaseType { get { return Symbol.BaseType; } }
bool IEntityOperation.HasCanExecute { get { return CanExecute != null; } }
Expand Down
14 changes: 2 additions & 12 deletions Signum.Utilities/Extensions/StringExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ public static string DefaultText(this string? str, string defaultText)
}

#pragma warning disable IDE0052 // Remove unread private members
static readonly Expression<Func<string, string>> EmtpyToNullExpression = a => a == null || a.Length == 0 ? null : a;
static readonly Expression<Func<string, string>> DefaultToNullExpression = a => a == null || a.Length == 0 ? null : a;
#pragma warning restore IDE0052 // Remove unread private members
[ExpressionField("EmtpyToNullExpression")]
public static string? EmtpyToNull(this string? str)
public static string? DefaultToNull(this string? str)
{
if (!str.HasText())
return null;
Expand Down Expand Up @@ -836,15 +836,5 @@ public static string[] SplitNoEmpty(this string text, params char[] separators)
{
return text.Split(separators, StringSplitOptions.RemoveEmptyEntries);
}

public static string EmptyToNull(this string text)
{
return string.IsNullOrEmpty(text) ? null : text;
}

public static string WhiteSpaceToNull(this string text)
{
return string.IsNullOrWhiteSpace(text) ? null : text;
}
}
}
4 changes: 3 additions & 1 deletion Signum.Utilities/Reflection/TupleExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ public static bool IsTuple(Type type)

private static bool IsTupleDefinition(Type genericTypeDefinition)
{
return genericTypeDefinition == TupleOf(genericTypeDefinition.GetGenericArguments().Length);
var numParameters = genericTypeDefinition.GetGenericArguments().Length;

return numParameters <= 8 && genericTypeDefinition == TupleOf(numParameters);
}

public static Type TupleOf(int numParameters)
Expand Down

2 comments on commit 28c71a1

@olmobrutall
Copy link
Collaborator Author

@olmobrutall olmobrutall commented on 28c71a1 Feb 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C# 8 Nullable reference types

C# 8 is around the corner and contains a feature I've been actively promoting for a few years (when Roslyn was in Codeplex!) to distinguish nullability not only in value types (Nullable<T>) but also in reference types (using NullableAttribute, flow analysis and warnings).

The idea is to reduce the number of NullReferenceExceptions, most of them produced by the lack of formality in declaring whether a property, parameter, return type etc, accepts or not null values.

A few links to introduce the feature:
https://blogs.msdn.microsoft.com/dotnet/2017/11/15/nullable-reference-types-in-csharp/
https://msdn.microsoft.com/en-us/magazine/mt829270.aspx?f=255&MSPPError=-2147217396

Implementation caveats

The general idea is clear, if you want a value, whether reference or value type, to be null just put ? in the type. This also works inside generics, like List<string?>? is a nullable list of nullable string.

But while nullability with value types is rock solid, because there is a concept of default(T) value for any value type, the same is not true for reference types. For example:

int?[] nullableNumbers = new int?[100]; 
nullableNumbers[0]; // is null, because null is default(int?) OK

int[] numbers= new int[100]; 
numbers[0]; // is 0, because 0 is default(int), OK

class Person { ... }

Person?[] nullablePersons= new Person?[100]; 
nullablePersons[0]; // is null, because null is default(Person?), OK

Person[] persons= new Person[100]; 
persons[0]; // is null, because null is default(Person), but Person is NOT NULLABLE!!!

Even more, the full feature is opt-in for old projects and based on warnings, while is opt-out for new projects.

That means, null should not be in not nullable reference values, and the C# compiler is going to make his best efforts to avoid this, but in reality at the CLR level they do fit inside the variable, and in fact you can put a value easily if you want:

Person p = null!; //The ! mark is a cast to not-null version of the type.

In practice this is not an issue except in two cases:

  • Arrays because they are initialized with a fixed size, but not List<T> where elements are introduced one by one.
  • Fields and properties in classes The C# compiler will produce a new warnings for every not-nullable reference type field that is not initialized in the constructor.
warning CS8618: Non-nullable property 'TestProperty' is uninitialized.

When is this comming?

You can start using it by installing Visual Studio 2019 Preview and using the NotNull branches of Framework and Extensions.

The whole thing will get merged the 2nd of April when Visual Studio 2019 is released https://devblogs.microsoft.com/visualstudio/

How it works in Signum.Entities

The way we define Entities doesn't fit nicely with the current implementation for two reasons.

  • We don't define a constructor. We typically create an empty entity and we let the user field the mandatory fields, one by one, in the UI. This is an implication of having the same entity for the UI and for the DB. An advantage that I don't want to lose. So we disable this particular warning (CS8618) in the entities assembly, feel free to disable it as well in other assemblies or particular regions of code if it's too annoying for DTOs or other classes were you prefer using object initialize instead of a constructor.

  • Entities are typically invalid when they are created, and validated the first time you try to save them. Still, I like to be able to express whether something is nullable in the DB directly by placing a ? in the type.

This means that all the guarantees that c# 8 nullable reference types bring, are just disable for new entities, in exchange, we can use the feature to know about whether things are mandatory and not null in the database, and we don't need to use ! everywhere after we retrieve and entity or make LINQ queries.

Example:

You can check this commit signumsoftware/southwind@d61f242 to see how the diff before and after C# nullable reference types in Southwind.

List of changes:

  • NotNullValidatorAttribute is not necessary anymore, if the type is not nullable then there is an implicit NotNullValidatorAttribute added by the franework. If for some reason you don't want it, you can place a [NotNullValidator(Disable=true)] attribute.

  • StringLengthValidator doesn't have AllowNulls property anymore, the implicit NotNullValidatorAttribute handles the empty string case too.

  • NotNullabeAttribute and NullableAttribute have been renamed to ForceNotNullableAttribute and ForceNullableAttributerespectively. They allow you to set nullability in the DB differently as in C#, typically not necessary.

  • Properties that before didn't have NotNullValidator, or had AllowNulls = true now just get a ? in the type.

How it works in Signum.React

I've used the oportunity to simplify the conversion between C# and TS so:

class PersoneEntity : Entity
{
    public string FirstName { get; set;}
    public string? MiddleName { get; set;}
    public string LastName { get; set;}
    public CountryEntity Country { get; set; }
    public ProviceEntity? Province { get; set; }
}

Gets translated to just:

interface PersoneEntity extends Entity
{
    firstName : string;
    middleName: string | null;
    lastName: string;
    country: CountryEntity;
    province: ProvinceEntity | null;
}

As you see, the mapping is quite simple, if there is a ? you get a | null, whether is a value or a reference type. Also note, undefined is not allowed anymore... but new entities the fields are undefined!!.

This is consistent with the lack of guarantees for new entities in C#, and prevents the error of assigning undefined (instead of null) to reset a property and then doing nothing because undefined is not serialized.

How to migrate

  1. Change the project files one by one (Entities, Logic, Load, Tests, React) adding the following options:
 <NullableContextOptions>enable</NullableContextOptions>
 <TreatWarningsAsErrors>true</TreatWarningsAsErrors>

Note: I set warnings as errors to make conformity with check mandatory.

Also you may need to replace, if C# 8 is still in beta by the time you do this.

<LangVersion>latest</LangVersion>
<LangVersion>8.0</LangVersion>

Finally, you need to remove the CS8618 globally in your Entities assembly (and optionally in the other assemblies to)

<PropertyGroup>
    <NoWarn>1701;1702;8618</NoWarn>
</PropertyGroup>
  1. Replace entities using the following regexe
## Strings ## 
# Add AllowNulls if missing
FIND: StringLengthValidator\(Min
REPL: StringLengthValidator(AllowNulls = false, Min

FIND: StringLengthValidator\(Max
REPL: StringLengthValidator(AllowNulls = false, Max

# Remove AllowNulls and add ? when necessary 
FIND: StringLengthValidator\(AllowNulls *= *false *, *(.*\n *public string)
REPL: StringLengthValidator($1

FIND: StringLengthValidator\(AllowNulls *= *true *, *(.*\n *public string)
REPL: StringLengthValidator($1?


## Entities and Lite<T> ##
# Add ? Everywhere
FIND: (\w+Entity|Lite<\w*Entity>|\w+Embedded|\w+Symbol) (\w+) *{ *get;
REPL: $1? $2 { get;

# Remove NotNullAttributes from MLists
FIND: ((NotNullable|NotNullValidator|NotNullableAttribute|NotNullValidatorAttribute), *)(.*\n *public MList)
REPL: $3

FIND: (\[(NotNullable|NotNullValidator|NotNullableAttribute|NotNullValidatorAttribute)\])(.*\n *public MList)
REPL: $3

# Remove NotNullAtrributes and ? when together
FIND: ((NotNullable|NotNullValidator|NotNullableAttribute|NotNullValidatorAttribute), *)(.*\n *public (\w+Entity|Lite<\w*Entity>|\w*Embedded|\w+Symbol))\?
REPL: $3

FIND: (\[(NotNullable|NotNullValidator|NotNullableAttribute|NotNullValidatorAttribute)\]).*\n *(public (\w+Entity|Lite<\w*Entity>|\w*Embedded|\w+Symbol))\?
REPL: $3
  1. Manually fix errors in the Logic, Load, Test, and React by adding ? to the types or, sometimes, ! to expressions when you feel smarter than the flow analysis.

Conclusion

Finally the nullability story is simplified, just write ? for nullable fields, doesn't matter if they are value types, string, entities, lites, MList or the elements in an MList.

The only thing to keep in mind: all is a lie, you could have nulls inside of reference types if you want it, and it will happen for new entities both in C# and TS.

Enjoy a future free of NullReferenceExceptions 😄

image

@MehdyKarimpour
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

Please sign in to comment.