Open back2dos opened 5 years ago
I don't think I want to mess with @:privateAccess
, but I agree otherwise.
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?
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
.
As long as it works only in the final-to-null way I don't see a problem. aFinal = aNull should error.
This is the diff of the tests that changed: https://github.com/Simn/haxe/commit/a534053c156d8c61c5df3453f8c745c987f13171
So this change would allow:
I'm going through all occurrences of AccNo
and AccNever
to check the places where one behaves differently than the other:
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
| 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.
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.
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.
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
.
let is_inline_var (field:tclass_field) =
match field.cf_kind with
| Var { v_read = AccInline; v_write = AccNever } -> true
See above.
| 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).
| 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.
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.
Not touching this again for 4.0. We have to rethink this later.
Following up on #7818:
This is weird. One could argue that unifying
never
withnull
creates a type hole: once access isnull
, you can use@:privateAccess
to read/write the value. You can even start withfinal
, promote tonever
, promote tonull
and mutate via@:privateAccess
. One "solution" would be to disallownever
to unify withnull
but it'd break a lot of existing code and make things more awkward than they already are.I would propose that:
null
andnever
get treated as being the same for structures/interfaces andfinal
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).@:privateAccess
cannot accessnull
on structures/interfaces, only on classes. Reasons:@:privateAccess
on structures/interfacesit actually makes sense to only allow bypassing access restrictions on concrete implementations, because on structures/interfaces it can go absolutely wrong: