Closed kevin-montrose closed 3 years ago
I think technically you can end up with a null
this
, but practically speaking it's not something that happens in sane code
Inside an object? Yes it's rare but can happen. Inside an extension method? Oh it certainly can and bit me recently...
I think technically you can end up with a null
this
, but practically speaking it's not something that happens in sane codeInside an object? Yes it's rare but can happen.
Yeah, I think it's practically only when you have call
instead of callvirt
on a non-virtual instance method. The C# compiler will generate callvirt
s so you get a null check (and then the JIT will de-virtualize them), so you tend to be doing something "weird" to get into this case.
Inside an extension method? Oh it certainly can and bit me recently...
Extension methods are actually static methods, so there isn't actually a this
pointer to be weird about.
I've made all the appropriate changes to this branch I think, I'll merge into vNext
in the next day or so... want to give myself some time to think through any additional gotchas.
Expanding tests to cover all of this definitely exposed some additional nuances, which has given me a bit of pause about just merging it.
Makes Cesil aware of null annotations, as raised in #25.
Visible changes
Formatter
,Getter
,InstanceProvider
,Parser
,Reset
,Setter
, andShouldSerialize
will now gain one of three "handlings" for the rows they take or produce and values they either take or produce. These handlings, while not visible themselves, can inject null checks into the generated code that reads and writes rows.string?
andint?
are considered nullable in the same way, whilestring
andint
are equally non-nullablestring?
) and "semantically nullable" types (likeint?
).Setter
s andGetter
s) no "handling" is inferred.Getter
returns a non-nullableobject
an explicit check will be made to ensure that it did not return null (test)Setter
that takes a non-nullable row with anInstanceProvider
than may produce null rows)Getter
s backed by instance methodsInstanceProvider
s backed by constructors will not have a null check inserted after they are invokedAllowNullRows()
andForbidNullRows()
methods, which return wrappers that are identical except for their "handling" of rows:Getter
,InstanceProvider
,Reset
,Setter
, andShouldSerialize
Getter
s,Reset
s,Setter
s, andShouldSerialize
s that do not operate on rowsAllowNullValues()
andForbidNullValues()
methods, which return wrappers that are identical except for their "handling" of values:Formatter
,Getter
,Parser
, andSetter
AllowNullXXX()
methods if the relevant type is not "semantically nullable", that is if it is not a reference type or a nullable value type.Formatter
, when two wrappers are chained their "handling" will be combined so that runtime checks will still fail earlyFormatter
s this means that if either wrapper forbids nulls, the chained pair will also forbid nullsParser
s one which createsstring
and one which createsstring?
and then chain them (creating a newParser
which createsstring?
) no exception will be raised if, when invoked at runtime, the firstParser
produces anull
since it is legal for the whole (chained)Parser
to produce nullsAllowNullXXX()
orForbidNullXXX()
methods called, the change applies to the wrapper as a whole, in keeping with the previous point.Commentary
I went back and forth on how much checking to do and when to do it (that is, whether to do it at "compile" time when readers/writers are created or at "runtime" when they are used), as well as how much client's should be allowed to "subvert" null handling.
Ultimately I settled on "doing it all at runtime" and "clients can subvert basically everything". I experimented with failing faster, and imposing more restrictions on clients, but ultimately it was too easy for code that "obviously" works to cause validation to fail.
The end goal is to, by default, fail quickly and helpfully if reading or writing values with subvert the constraints implied by nullable reference types I think this approach achieves that, while also allowing for clients to override or customize Cesil's behavior.
Internal notable changes
NullHandling
Utils
to support the above