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

Shorthand nullable-type syntax #77

Closed
wants to merge 5 commits into from
Closed

Shorthand nullable-type syntax #77

wants to merge 5 commits into from

Conversation

SomeRanDev
Copy link
Contributor

Allow T? as shorthand for Null<T>.

@:nullSafety(Strict) {
    var MyList: Array<Int?>? = null;
    MyList = [20, null, 32];
}

Rendered version

@nadako
Copy link
Member

nadako commented Jun 24, 2020

This looks fine to me and I agree with the motivation.

Originally I was against and I'm still a bit undecided regarding adding syntax sugar for nullable types, because I believe that usage of nullables should generally be minimized. However the current situation is that having Null<T> even where it makes sense is so tedious (both to write and read) that people would rather not use null-safety feature at all, so my gut feeling today is that making this more concise will actually help adoption of null-safety, which in turn will make people think more about what is nullable and what is not, which will minimize the unnecessary nullability. :)

Regarding type-as-expression, I think it's the same situation as with the type params - we just don't parse that at all currently, so ? would be a ternary there. This might conflict with the potential EComplexType proposal, if we ever reconsider it, but it sounds like it could be solved with some parenthesis.

@Gama11
Copy link
Member

Gama11 commented Jun 24, 2020

I'm generally in favor of this since it would reduce the visual clutter when working with null safety by a lot.

However, I wonder about the "consequences" considering the the existing optional argument and object field syntax. The following would all be possible, but look confusing:

function foo(?i:Int?) {}

typedef Foo = {
    var ?i:Int?;
}

typedef Foo = {
    ?i:Int?
}

Putting a ? on the type in these cases is redundant, since it's already implied by the optionality of the argument / field. That means it would probably be considered bad style / should maybe be grayed out by IDE diagnostics? In Either case, it probably couldn't be disallowed entirely.

Similar to other languages, there can be whitespace between the type and the ?; however, no whitespace should be the standard

I think we should consider simply forbidding to put whitespace there, similar to the var ?name syntax.

@nadako
Copy link
Member

nadako commented Jun 24, 2020

Regarding the proposal, maybe it's worth showing some awkward possible situation with implicit Null<T> for optional arguments and fields, e.g. function f(?a:Int) and {?a:Int} - this is a place where "Nullable reduncancy" can appear, along with the slight syntatical weirdness if you do function f(?a:Int?) or {?a:Int?}. Also maybe add a note that the printing of Null<T> in compiler messages should probably be consistent and use the new syntax with the question mark as well, but also be careful about printing optional argument and fields to maybe avoid adding ? to the type there...

@RealyUniqueName
Copy link
Member

I think we can emit an error if a redundant ? is spotted on parsing. And just ignore/merge it if Null<Null< is the result of typing or inlining.

@Gama11
Copy link
Member

Gama11 commented Jun 24, 2020

@RealyUniqueName Which redundancy are you talking about? The optional ? with Type? or the Type?? mentioned in the proposal?

@SomeRanDev
Copy link
Contributor Author

SomeRanDev commented Jun 24, 2020

Thanks for the quick feedback!

For starters, the "type-as-expression" was a bit ambiguous when I wrote it due to lack of in-depth compiler knowledge, but I was also thinking about parenthesis as a solution. I'll go ahead and remove some of the uncertainty in the section and commit to explicitness through either parenthesis or Null<T> there.

For the whitespace, I just committed to allowing it since both C# and Kotlin allow it, but quite frankly it looks horrible, even with just a single space and would actually much prefer it force no whitespace. Would be more than happy to change that as well.

In regards to printing, do you mean there should be some differentiation between Null<T> and T? and the output should reflect which style is used, or should all output of Null<T> print with the T? syntax?

And finally, in regards to {?a:Int?}, I'll go ahead and add that to "Drawbacks" (or perhaps it's something to be resolved within "Detailed design"?) Either way, don't know if I'm involved enough with Haxe design to answer that one. Would there be a reason to do it in the first place? Do null-safety features not trigger with {?a:Int} alone? They do; so I guess if there really is no difference, just throwing a warning/error would be fine, right?

@nadako
Copy link
Member

nadako commented Jun 24, 2020

In regards to printing, do you mean there should be some differentiation between Null<T> and T? and the output should reflect which style is used, or should all output of Null<T> print with the T? syntax?

The latter. We don't retain the details of the original syntax anyway, especially in the typed tree.

@nadako
Copy link
Member

nadako commented Jun 24, 2020

Regarding {?a:Int?}: while it looks funny, I don't consider it particularly confusing to read, and this is really a rare edge case as there's no need to write like that, since optionality already implies nullability in these situations. What I'm thinking is that when the compiler prints such a type in e.g. its error messages maybe some care needs to be taken to print {?a:Int} instead of {a?:Int?} :)

@skial skial mentioned this pull request Jun 24, 2020
1 task
@Simn
Copy link
Member

Simn commented Jun 24, 2020

I don't like how this looks. IMO Null<T> is actually more readable, especially if we start nesting things.

I acknowledge that this is easier to type, but in that case it should be solved at IDE-level.

@back2dos
Copy link
Member

Since we already have ?Type (via ComplexType.TOptional) I wonder why we should have Type? as well.

@kaikoga
Copy link

kaikoga commented Jun 25, 2020

TL;DR: While I'm not against Null<T>s are treated special and granted a T? syntax, I don't expect this proposal would come true.


Null<T> (and possibly T?) is clearly a valid Haxe type. And surely ?T is also a valid Haxe type syntax, but in reality it is something that works outside "type" context.

I'm afraid to say that we don't have optional types but actually optional arguments/fields that are coincidentally marked in ComplexTypes (because things like TFunction(args:Array<ComplexType>, ret:ComplexType) and TField relies on ComplexType to carry information about whether it is optional or not).
Proof: We can turn first arg of Int->Void into optional by writing ?Int->Void. So ?Int looks like an optional type. But we cannot take (a:Int)->Void and turn it into (a:?Int)->Void. We must write (?a:Int)->Void, or the compiler complains Optional type not allowed here. While both ?Int->Void and (?a:Int)->Void are valid Haxe types, there is no convincing reason (except compiler implementation detail) that we should wrap the optional named type as TOptional(TNamed(_)) in this exact order. And we cannot write ?Type anywhere else, which gives a question whether ?Type is a first class type.

Not to mention that optional and nullable works differently (i.e. optional args can be skipped but nullable args cannot), I think we should at least try to not mix up optional and nullable, they are meant to work in a completely different layer. TypeScript is one example which has optional properties and arguments, just like we do, but (unsurprisingly) lacks shorthand nullable-type syntax.

And I'm sorry if I summoned Nic's veto, as this state would confuse beginners even more (hey I've just defined a typedef with nullable field in var notation, wait where should I put the ? to make that field optional?).

Summary: We should first clean up and break optional, which would not happen, before making another use of the ? character, I regret.

P.S. I personally welcome a haxe-evolution that deletes the ternary (I don't mind typing if (a) b else c).

@back2dos
Copy link
Member

back2dos commented Jun 25, 2020

We must write (?a:Int)->Void, or the compiler complains Optional type not allowed here.

That's circular reasoning. For the parser (a:?Int)->Void is perfectly valid syntax already and the typer may well interpret it as (a:Null<Int>)->Void instead of emitting said error.

For the legacy function syntax, there's indeed some ambiguity, as ?Int->Void makes the argument not just nullable but also optional. It should remain as is, since fans of legacy syntax can (and for reasons of consistency should!) still use the old school Null<Int>->Void if they want nullable mandatory args. When using the new function type syntax, users can explicitly distinguish between mere nullability and full optionality via ?-placement.

Maybe postfix ? is better still, but adding syntax often means a breaking change in the macro API. So if there's a sane way to reuse existing syntax, it's worth considering, because it makes it easier to move forward.

P.S. I personally welcome a haxe-evolution that deletes the ternary (I don't mind typing if (a) b else c).

Many (most?) people would, because it often stands in the way of adding sane syntax. Unfortunately it's quite widely used, so it's doubtful such a breaking change will ever come ;)

@Simn Simn added this to the 2021-12 milestone Nov 8, 2021
@Simn
Copy link
Member

Simn commented Nov 9, 2021

We have rejected the proposal in the haxe-evolution meeting today.

The syntax has no clear advantage other than potentially being shorter. There were also concerns about possible syntactic ambiguities that could arise from this.

@Simn Simn closed this Nov 9, 2021
@Simn Simn added the rejected label Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants