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

Type array access on Array<T> as Null<T> #6825

Merged
merged 7 commits into from
Feb 12, 2018

Conversation

RealyUniqueName
Copy link
Member

Current behavior

var a = ['hello'];
$type(a[1000]); // String
trace(a[1000]); //null

Behavior of this PR:

var a = ['hello'];
$type(a[1000]); // Null<String>
trace(a[1000]); //null

This PR adds Null<> for array read access on all platforms except Int, Bool, Float on static targets.
In addition to proper typing it also makes possible to check array access for null safety (i'm making a plugin for that).

@nadako nadako requested review from ncannasse and removed request for nadako January 24, 2018 12:37
@nadako
Copy link
Member

nadako commented Jan 24, 2018

I think it's for @ncannasse to review.

@skial skial mentioned this pull request Jan 25, 2018
1 task
@ncannasse
Copy link
Member

We don't make any difference between Null<String> and String and that can cause many issue with abstracts or other platform specific not nullable basic types (Single, hl.UI16 etc.) so I don't think this should be merged.

@RealyUniqueName
Copy link
Member Author

RealyUniqueName commented Jan 25, 2018

We don't make any difference between Null and String

While there is no difference for Haxe type system, macros/plugins may want to deal with Null<>. And it's just semantically correct, because of Array spec.

that can cause many issue with abstracts

What kind of issues?

or other platform specific not nullable basic types (Single, hl.UI16 etc.)

What if we add @:notNullable meta to such types?

@RealyUniqueName
Copy link
Member Author

RealyUniqueName commented Feb 1, 2018

or other platform specific not nullable basic types (Single, hl.UI16 etc.)

They already have @:notNull meta. I added a commit which handles it.

@ncannasse
Copy link
Member

@RealyUniqueName you need to follow the types, since a TMono/TLazy might link to a basic type

@RealyUniqueName RealyUniqueName mentioned this pull request Feb 1, 2018
@RealyUniqueName
Copy link
Member Author

@RealyUniqueName
Copy link
Member Author

Can i merge this?

@ncannasse
Copy link
Member

Looks ok, but the readByte() commit has nothing to do with the fix, I think ?

@RealyUniqueName
Copy link
Member Author

RealyUniqueName commented Feb 12, 2018

It has. Compiler complains on signtature difference with the parent method. Because this one is infered as Null<Int> while the parent one is Int

@RealyUniqueName RealyUniqueName merged commit 9afd1a6 into HaxeFoundation:development Feb 12, 2018
@ncannasse ncannasse mentioned this pull request Feb 13, 2018
RealyUniqueName added a commit that referenced this pull request Feb 13, 2018
@RblSb RblSb mentioned this pull request Dec 2, 2021
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.

3 participants