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

Final vs. never vs. null #8011

Merged
merged 7 commits into from Mar 31, 2019
Merged

Conversation

Simn
Copy link
Member

@Simn Simn commented Mar 18, 2019

Aligns the behavior of null and never field access on anonymous structure types. See #7838 for more information.

closes #7838

@skial skial mentioned this pull request Mar 18, 2019
1 task
@Simn
Copy link
Member Author

Simn commented Mar 18, 2019

@ncannasse
Copy link
Member

ncannasse commented Mar 18, 2019 via email

@back2dos
Copy link
Member

Uhm, there's still a type hole:

// not possible, for good reasons:
t(typeError(finalField = nullField));
// does the same but compiles just fine:
neverField = nullField;
finalField = neverField;

One of the two assignments should fail, and I think it'd be better if the latter did. Non-final fields don't unify with final fields, while the converse is true. It's a simple and sound rule.

Is still think there's another issue: To me never is a hint for classes only and I think it'd be best to really be clear about what it means for structures / interfaces. Right now, it is broken. In prior exchanges Simn has maintained that @:privateAccess is some uncontrollable type-system-subverting magic that is supposed to not work. I disagree with that alone, but the problem is way larger than that. You can use @:allow / @:access or even plain subtyping to get write access to a field that's not there and will therefore crash on static platforms:

class Test {
  static function main() {
    try {
      new Foo().bar();
      trace('worked');
    }
    catch (e:Dynamic) {
      trace('fail $e');
    }
  }
}

interface IFoo {
  var foo(get, null):Int;
}

class Foo implements IFoo {
  public var foo(get, never):Int;
  function get_foo() return 42;
  public function new() {}
  public function bar() {
    var foo:IFoo = new Foo();
    foo.foo = 42;
  }
}

I think there are really two different options:

  1. disallow private access via null on structures / interfaces and then consider null and never fully equivalent. Meaning foo.foo = 42; shouldn't compile. Arguably, that could be fixed by just not allowing private access to implemented interfaces (which we probably really shouldn't allow in the first place), but that would still leave the option to gain access via @:allow / @:access and unless that's now also considered a subversion of the type system, it requires fixing.
  2. keep it as is, then never MUST NOT unify with null, because null demands the presence of a physical field (a difference that should not matter, but begins to if there's a way to access the field). Meaning public var foo(get, never):Int;

I absolutely prefer the first option. It's perfectly sound and very unlikely to break code. What's more, I just don't see the use case for private access on null fields for interfaces / structures.

@Simn
Copy link
Member Author

Simn commented Mar 20, 2019

Fixed the type hole.

That interface situation is another bug we should fix. That is a declaration-site problem because the implementer should not have more restrictive access than the interface.

I don't think access control should subvert never. In which concrete case does this happen?

@back2dos
Copy link
Member

Like so:

class Test {
  @:access(IFoo)
  static function main() {
    try {
      var foo:IFoo = new Foo();
      foo.foo = 42;
      trace('worked');
    }
    catch (e:Dynamic) {
      trace('fail $e');
    }
  }
}

interface IFoo {
  var foo(get, null):Int;
}

class Foo implements IFoo {
  
  public var foo(get, never):Int;
  function get_foo() return 42;
  public function new() {
  }
}

And if we can agree that that shouldn't compile, perhaps you could find it in you to give me a more tangible argument on why it's a good idea to still have it compile with @:privateAccess and then fail at runtime?

@Simn
Copy link
Member Author

Simn commented Mar 20, 2019

That is the same issue as before. The problem here isn't the access but the declaration. I think that class should not exist like that in the first place.

We have to distinguish declarative subtyping (interfaces) from structural subtyping. A null write-access on an interface variable tells me that the implementing class may access this field. A null write-access on a structure tells me that nobody can access it and thus makes it equal to never. So while you group interfaces with structures in this discussion, I group them with classes.

However, thinking about it like that, I agree that @:privateAccess should not be allowed to subvert access through structures. Access control (which is really more accurately described as visibility control) should already not allow to influence structures.

So to summarize:

  • @:privateAccess on structure field should do nothing
  • classes should not be allowed to implement a null access as never

@Simn
Copy link
Member Author

Simn commented Mar 21, 2019

Thinking about this again, it might actually be desirable for a class to restrict itself to not access a field. However, this isn't compatible with our definition of what a physical field is, as you've demonstrated. This would require tagging these fields if they come from an interface, but at that point things get confusing and I'm not sure it's worth the complications.

Either way, I'd like to merge this PR because the changes should be good. We can discuss the interface and @:privateAccess situation a bit later.

@back2dos
Copy link
Member

Yep, as far as final vs. non-final is concerned, I think we're good ;)

@Simn Simn merged commit 3096727 into HaxeFoundation:development Mar 31, 2019
@Simn Simn deleted the never_final_null branch March 31, 2019 06:39
RealyUniqueName added a commit that referenced this pull request Apr 3, 2019
RealyUniqueName added a commit that referenced this pull request Apr 5, 2019
* Revert "Final vs. never vs. null (#8011)"

This reverts commit 3096727.

* test for #8079, #8081, #8093

* fix test for #8079
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

final vs never vs null
3 participants