Use abstract instead of typedef for Null<T>
#4316
Conversation
|
I think this is really good, but I'm not sure it's a good moment for this. On Thu, Jun 11, 2015 at 8:38 PM, Simon Krajewski notifications@github.com
|
|
We already fixed SPOD and I doubt there's a lot of code which is affected by this. As I said there's plenty of time to address this change in other macros for the next release. This change is fundamental for any notion of null-safety and I don't think we should delay it because some macros could break. Type.hx even warns you: |
|
Good to know, I hadn't looked at the files changed. Still, I don't quite share you optimism. Every optional field or argument My OCaml being bad, I cannot really tell if that has changed but search the Every macro that just relies on Context.follow to reduce I don't mean to be defeatist, just pointing out that "a lot" might be I think it's awesome if Haxe gets null safety. I'm all for it and I have no On Fri, Jun 12, 2015 at 8:50 AM, Simon Krajewski notifications@github.com
|
|
Context.follow still follows Null. The only thing I can see breaking is macros checking for TType Null explicitly. There are really only two options here:
|
|
I agree with Juraj that it has the potential for more than enough breaking. Also, I have not actually decided if we should move forward null safety or not for 4.0. While I appreciate null safety in OCaml, I think it will cause more verbosity in many places when interacting with native APIs that have not been designed with null safety in mind. I then think we should postpone any change in that direction for now. |
|
I don't understand why anyone would be against optional null-safety. That seems really absurd... I don't even know what to say to that. I would have considered this to be a huge step for Haxe and you're just blocking it off for reasons I don't understand. Everyone I spoken to was looking forward to this change. Too puzzled to argue right now... |
|
i would argue that null-safety is a huge step forward for type-safety in haxe. With a postfix force operator like |
|
Even null-safety aside, isn't it actually more correct to have |
|
I don't think that's a good idea in general to introduce language changes in an optional manner (apart when doing transitions from big'o releases). Let's say we make it optional (off by default), then people turning it on will get errors in other people code, which mean we will have two different "versions" of the language, one being not compatible with the other. I don't think that additional fragmentation is worth having for now. |
|
I don't think this will introduce any fragmentation - since any code that On Sun, Jun 14, 2015, 15:27 Nicolas Cannasse notifications@github.com
|
|
@waneck Yes but that we should assume that we want to have it on by default, and that people which will turn it off will not write any library with this flag enabled. |
|
it should actually only be turned off by now until it gets enabled by default later (4.0??). |
|
Just to resume my thoughts in general : I'm much more interested in improving the global usability in Haxe wrt problems that users are facing (unicode, native interaction, IDE support, etc) than introducing breaking changes (or even non breaking changes) in the language. 3.0 was designed to be as stable as possible, I'm quite satisfied with the way it is right now. I'm not expecting to make a 4.0 this year or maybe even next year. I think there's already plenty of things for us to do in the meantime. |
|
For all intents and purposes this change is well worth it even if we decide to not go for null-safety. I always considered it to be just a matter of time until we change |
|
@Simn I'm fine with Null typedef. Is there an actual advantage at Null being an abstract without null safety that I overlooked ? because I can see the disadvantage (breaking code). "if we break, break early" works well when it's related to a change we will make anyway, or in order to unify behaviors that vary between platforms. |
|
The advantage is that |
|
That letting Null and Int unify is a very bad idea from a performance-oriented perspective should be obvious. It makes writing static targets much harder (it adds a lot of complexity to the C target, by forcing run-time checks into otherwise perfectly translatable code paths). This isn't just a cosmetic issue (like e.g. short lambdas) - this is a correctness issue and a performance issue. |
|
which equal function are we talking about ? because usually we are considering equality if follow a == follow b, and since follow will still follow null abstract I don't think that makes much difference. We could modify type_eq to deal with Null typedef (or Null abstract), but I'm a bit afraid of the breaking that would occur then. We could easily experiment with the current typedef implementation (enabling more strict comparison) and see how it goes. @ousado if you're proposing that |
|
The new Null cases in type.ml are there to retain compatibility. As I said in the initial post the idea is to remove them in Haxe 4 so we can clean up the mess that |
|
@Simn I agree that the variance thing is a annoying, but I fail to see how using an abstract here will help. Either |
|
This is not about the unification of |
|
Yes, so it's about type_eq, which is used to compare type parameters. We could make a specific case for Null typedef there, and enable this more strict checking with a define to let people test their code with this and see what kind of errors we get, that would surely give us the feedback we need without any breakage. |
|
I've just made a small patch to showcase that. I'm getting not funny errors on some codebase I have. |
|
I still don't get what's the issue with Null becoming an abstract. This On Sun, Jun 14, 2015, 16:56 Nicolas Cannasse notifications@github.com
|
|
@waneck it breaks all macro code which are checking for TType(Null) + has the potential to break some other parts of the compiler without us noticing until someone reports Even the small change I made in my patch breaks the following:
function getAll() : Array<{ i : Item, qty : Int }> return [for( i in inventory ) { i : fetch(i), qty : i.qty }];However the fetch method returns Both are easy to fix for me since I know what's going on under the hood. I know for instance I can do I'm not sure we should allow such errors from occurring for all the people using Haxe |
|
@ncannasse I quite like that it breaks on the given examples. |
|
To clarify: This PR does not break abstract variance yet, it merely prepares for doing so in the future by removing a special case as opposed to adding one like in your approach. You have conceded that breaking abstract variance is desirable, which justifies this PR, You are also okay with adding a define for your approach which invalidates your previous fragmentation argument. The only argument you have against this is that TType(Null) in macros no longer matches (I'm ignoring the "other parts of the compiler" argument - that's true for virtually any change we make). If that alone is enough to reject this PR then I'll just have to accept that we have different philosophies with regards to language design. I see this as a huge wasted opportunity. |
|
@Simn desirable does not mean we should not experiment with it first. From my little patch I already see several problems with this, so in the end we might actually not change variance handling if it introduce more issues than it solves, or we will do it only in some sub cases, such as basic types with no recursion. I do think that my patch allows us experimenting with the idea without any breaking, so yes that's enough to reject the PR. |
|
@sledorze these are two examples of perfectly valid code that the compiler should not complain about. Being type safe for me is about getting help from the compiler, not having it annoying me doing extra typing for things that are correct. |
|
@ncannasse Everybody agrees that more typesafety (and more uncorrect program rejected) implies that some implicitly correct program can also be rejected. It's an acceptable tradeoff that the developper has to explicitly force the compiler not to reject unprovably correct code. The added benefit is that it will be easier to maintain as the code exhibit this intend and everyone will have a greater confidence into their program behaviour when no such intend are used. |
|
This certainly went differently than I expected ... -.- Just to be clear: I have some reservations about the timing. That's it. As for the counter-arguments:
Having an opinion about punctuation was important enough to introduce So again, I think it's a just matter of timing and communication. For On Mon, Jun 15, 2015 at 12:36 AM, Nicolas Cannasse <notifications@github.com
|
|
Lol, welcome to the "Sorry I invoked ktulu with a half joke" club...
|
|
Woohoooo! Do I now get free drinks or something? ^^ On Wed, Jun 17, 2015 at 6:14 PM, delahee notifications@github.com wrote:
|
|
For the record we started yesterday discussing about this (among other things) with @Simn . I have to leave for two weeks of holidays then we'll resume the discussion when i'm back ;) |
|
That's a positive note! I wish you the best vacation :-P |
|
It's a big step thus I think we should do it ASAP, before Haxe being too popular to change. |
|
So... it's been a few months and I just read through the comments again. Having gained some distance to all this, I still think that this is the right thing to do and that we should do it as soon as possible. I don't see a strong case against the content of this change. The only argument is a somewhat nebulous "it might break something" and a notion of "it might not be necessary", both of which can be used to block off anything regardless of its nature. The only argument I can relate to at all is @back2dos' timing concerns. While I don't think I agree with it, we can certainly discuss it. Regardless, I'm adamant about this change being made eventually. |
Simn
force-pushed
the
null_as_abstract
branch
from
72703f0 to
d11ad30
Compare
|
It will break some complex macros like e.g. we have in our code base, but OTOH I find it quite annoying to always remember that Anyway, I find this breaking change is for the greater good similar to #3514 and will be happy to adapt. As for the timing concerns, I am really not sure. It's not like we haven't break stuff during 3.x (I personally had to fix our codebase between 3.1.3 and 3.2.0), but this is a behavioural change. However, if we talk about Haxe 4, maybe we could already start working on it (at least plan concrete changes, create a switch like As for the development focus, I think it's kind of wrong to assume that our "development resource" can be easily switched to e.g. IDE or Unicode support. For example, in my vision, @Simn is really great at streamlining the compiler and language, but can't be arsed to work on an IDE, especially when we have people in the community that actually do that (@elsassph and others). I think it's pretty the same for Unicode (could we use help of @mandel59 for bringing unicode to haxe?). Sorry if that was too straightforward from me, but the point is: I don't think we can make core compiler "people" effectively work on e.g. IDE, but we can instead make use of the community and as a result improve both the compiler AND ide,unicode and whatnot. |
|
I still don't see the need to change things here, but if Simon wants to do it then ok. It will still silently break code in macros that are using Null detection (I have several of them) so it's not possible to change that in 3.x. I don't think it's possible to have the two in the compiler at the same time with a flag, so maybe an experimental branch is necessary so people can test against it |
|
Maybe an abstraction such as On Mon, Nov 30, 2015 at 1:27 PM, Nicolas Cannasse notifications@github.com
|
|
@back2dos you speak my mind on it! |
|
I like that idea too. Something else I admittedly didn't really think through is this: abstract ANull<T>(T) from T to T { }
typedef Null<T> = ANull<T>; |
|
I LIKE IT 2015-11-30 12:44 GMT-02:00 Simon Krajewski notifications@github.com:
|
|
Meh, my idea is not worth it and causes too much internal friction. I think going for |
|
What does the return value of |
|
Yes. /**
Extracts the `T` of `Null<T>` and returns `Some(T)` if successful, None
otherwise.
If `t` is null, the result is unspecified.
**/
static public function extractNullType(t:Type):Option<Type> {
return switch (t) {
case TType(_.get() => { name: "Null", pack: [] }, [t]): Some(t);
case TAbstract(_.get() => { name: "Null", pack: [] }, [t]): Some(t);
case TType(_) | TLazy(_): extractNullType(follow(t, true));
case TMono(_):
var t2 = follow(t, true);
switch (t2) {
case TMono(_): None; // Avoid stack overflow
case _: extractNullType(t2);
}
case _: None;
}
} |
|
getNullType is good for people that want to keep both behaviors, but that doesn't solve pattern matching, which will still fails silently if we change that now. |
|
I'm aware of that, and it will silently fail if we change it later too. Still, I don't think that's something a post to haxelang and some documentation couldn't fix. |
I actually think it is a good idea to specify this as the standard to |
|
I'm fine with providing some transition support functions as long as the changes does not actually happen before 4.x |
|
I don't really see what we gain this way other than more potentially broken macros, but I'm content as long as we get this change eventually. I guess I'll have to start a Haxe 4 branch soon! |
|
I can provide some testing for that branch with my 2 code bases :-P |
|
Can no longer be merged due to the source refactoring. |
For the most part this changes some TType to TAbstract and moves match cases around where necessary. There are a few special cases in type.ml to retain the current behavior and not break anything.
The idea is this:
to TfromNull<T>and provide some sort of.force():Tfield (for 3.3.0).Array<Int>andArray<Null<Int>>(for Haxe 4.0.0).This PR might break macros that check for TType("Null") right now. However, we have plenty of time to communicate that before the next release so it shouldn't be a problem.
@ncannasse: If you have any reservations about doing this please speak up now. I would like to merge this really soon because it's quite merge-conflict-vulnerable.