final vs never vs null

[back2dos]

Following up on #7818:

class Main {
  static function main() {
    var aFinal:{ final foo:Int; } = { foo: 42 };// works, because of top down inference
    var aNever:{ var foo(default, never):Int; } = aFinal;// no problem
    var aNull:{ var foo(default, null):Int; } = aNever;// still no problem
    
    aNull = aFinal;// Cannot unify final and non-final fields ... what?
  }
}

This is weird. One could argue that unifying never with null creates a type hole: once access is null, you can use @:privateAccess to read/write the value. You can even start with final, promote to never, promote to null and mutate via @:privateAccess. One "solution" would be to disallow never to unify with null but it'd break a lot of existing code and make things more awkward than they already are.

I would propose that:

1. null and never get treated as being the same for structures/interfaces and final may unify with both. Code written against read-only structures/interfaces should be able to operate on immutable objects (just not the other way around).
2. @:privateAccess cannot access null on structures/interfaces, only on classes. Reasons:

   • it closes the "type hole"

   • I'm relatively optimistic that very little code actually uses @:privateAccess on structures/interfaces

   • it actually makes sense to only allow bypassing access restrictions on concrete implementations, because on structures/interfaces it can go absolutely wrong:

     interface HasFoo { var foo(get, null):Int; }
     
     class Test implements HasFoo { 
     
       @:isVar public var foo(get, set):Int; 
         function get_foo() return foo; 
         function set_foo(param:Int) 
           return 
             this.foo = if (param > 100) 100; 
             else if (param < 0) 0; 
             else param; 
     
       function new() {} 
       static function main() { 
         var t = new Test(); 
         t.foo = 1000; 
         trace(t.foo);//100 - of course ... foo will never be > 100
         var f:HasFoo = t; 
         @:privateAccess f.foo = 1000; 
         trace(t.foo);//1000 ... whahooops! 
       } 
     }

[Simn]

I don't think I want to mess with @:privateAccess, but I agree otherwise.

[back2dos]

Ok, I've mentioned @:privateAccess a lot there, but it's beside the point. The question is: could we plainly disallow all access to null on interfaces/structures? And if so, is there any reason not to do it?

[Simn]

I don't want to change @:privateAccess. To me it's on the same level as access through Dynamic or reflection: You subvert the type-system and are responsible for the consequences. Given that we will never have 100% safety guarantees here while Dynamic and setField exist, I see no reason to restrict @:privateAccess.

[ncannasse]

As long as it works only in the final-to-null way I don't see a problem.
aFinal = aNull should error.

[Simn]

This is the diff of the tests that changed: Simn@a534053

So this change would allow:

• final-anon-field to null-anon-field
• final-class-field to never-anon-field and null-anon-field

[Simn]

I'm going through all occurrences of AccNo and AccNever to check the places where one behaves differently than the other:

type.ml

let is_physical_var_field f =
	match f.cf_kind with
	| Var { v_read = AccNormal | AccInline | AccNo } | Var { v_write = AccNormal | AccNo } -> true

Technically, somebody could call this for TAnons as well, but it only makes sense for class fields where the distinction is correct. Should be fine

fields.ml

	| Var v ->
		match (match mode with MGet | MCall -> v.v_read | MSet -> v.v_write) with
		| AccNo when not (Meta.has Meta.PrivateAccess ctx.meta) ->

This means that @:privateAccess only works on null, not never. That seems good.

			| (MGet | MCall), Var {v_read = AccNever} ->
				AKNo f.cf_name

This is in the context of abstracts.

nullSafety.ml

let should_be_initialized field =
	match field.cf_kind with
		| Var { v_read = AccNormal | AccInline | AccNo } | Var { v_write = AccNormal | AccNo } -> true

This is again in the context of class fields, so the distinction is good.

initFunction.ml

if v.v_write <> AccNever && not (Meta.has Meta.CoreApi cl.cl_meta) then com.warning "@:readOnly variable declared without `never` setter modifier" cf.cf_pos;

This shows up in a function named handle_class so it's probably about a class.

gencpp.ml

let is_readable class_def field =
   (match field.cf_kind with
   | Var { v_read = AccNever } when not (is_physical_field field) -> false

let is_writable class_def field =
   (match field.cf_kind with
   | Var { v_write = AccNever } when not (is_physical_field field) -> false

There's a class_def here, so it's about a class.

      match field.cf_kind, follow field.cf_type with
      | Var { v_read = AccInline; v_write = AccNever },_ ->
         script#writeOpLine IaInline;

This matches the specific combination of inline-access where v_write is always AccNever.

genphp7.ml

let is_inline_var (field:tclass_field) =
	match field.cf_kind with
		| Var { v_read = AccInline; v_write = AccNever } -> true

See above.

optimizerTexpr.ml

	| FInstance (c,_,cf) | FStatic (c,cf) | FClosure (Some(c,_),cf) ->
		begin match cf.cf_kind with
			| Method MethDynamic -> false
			| Method _ -> true
			| Var {v_write = AccNever} when not c.cl_interface -> true

This is again within the context of a class/interface (FInstance, FStatic or FClosure with a class).

matcher.ml

				| TField(_,FStatic(c,({cf_kind = Var {v_write = AccNever}} as cf))) ->
					PatConstructor(con_static c cf e.epos,[])

Class context (due to FStatic). This one seems a bit weird actually, but that's a separate issue.

typeloadFields.ml

if (set = AccNormal && get = AccCall) || (set = AccNever && get = AccNever)  then error (name ^ ": Unsupported property combination") p;

This is in class context.

--

Summary: Looks like we're good.

[Simn]

Not touching this again for 4.0. We have to rethink this later.