Bug 189047 – Add nullablity attributes to JSValue

• Status: REOPENED
• Component: New Bugs

[Keith Miller]

Add nullablity attributes to JSValue

[Keith Miller]

Created attachment 348291 [details]
Patch

[Geoffrey Garen]

Comment on attachment 348291 [details]
Patch

r=me

[Keith Miller]

Created attachment 348306 [details]
Patch for landing

[Keith Miller]

Comment on attachment 348306 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=348306&action=review

> Source/JavaScriptCore/API/JSValue.h:363
> +- (void)defineProperty:(nullable JSValueProperty)property descriptor:(nullable id)descriptor;

Actually, I think this needs to be nullable only when it can be an id and non-null the rest of the time. Will fix.

[Keith Miller]

Created attachment 348308 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 348308 [details]
Patch for landing

Clearing flags on attachment: 348308

Committed r235432: <https://trac.webkit.org/changeset/235432>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43805577>

Normally we’d enclose an API header in NS_ASSUME_NONNULL_BEGIN-NS_ASSUME_NONNULL_END and only annotate the nullable types.

[Keith Miller]

(In reply to mitz from comment #9)
> Normally we’d enclose an API header in
> NS_ASSUME_NONNULL_BEGIN-NS_ASSUME_NONNULL_END and only annotate the nullable
> types.

Oh, I didn't know that existed. I'll upload a new version.

[Keith Miller]

Reopening to attach new patch.

[Keith Miller]

Created attachment 348317 [details]
Patch

[Keith Miller]

Committed r235436: <https://trac.webkit.org/changeset/235436>

[Darin Adler]

Will this break compatibility for Swift programs that use these methods?

[Darin Adler]

Will this break source code compatibility for Swift programs that use these methods?

[Keith Miller]

(In reply to Darin Adler from comment #15)
> Will this break source code compatibility for Swift programs that use these
> methods?

No, it will just generate a warning if the users don't follow the nullability rules. At least according to https://developer.apple.com/swift/blog/?id=25.

[Darin Adler]

(In reply to Keith Miller from comment #16)
> (In reply to Darin Adler from comment #15)
> > Will this break source code compatibility for Swift programs that use these
> > methods?
> 
> No, it will just generate a warning if the users don't follow the
> nullability rules. At least according to
> https://developer.apple.com/swift/blog/?id=25.

I read that, and it’s not what it says. In Objective-C code it just creates warnings. In Swift code, the change is different. I think it *is* a source-incompatible Swift change. But maybe one that is worth doing.

[Keith Miller]

(In reply to Darin Adler from comment #17)
> (In reply to Keith Miller from comment #16)
> > (In reply to Darin Adler from comment #15)
> > > Will this break source code compatibility for Swift programs that use these
> > > methods?
> > 
> > No, it will just generate a warning if the users don't follow the
> > nullability rules. At least according to
> > https://developer.apple.com/swift/blog/?id=25.
> 
> I read that, and it’s not what it says. In Objective-C code it just creates
> warnings. In Swift code, the change is different. I think it *is* a
> source-incompatible Swift change. But maybe one that is worth doing.

Ah, I misread your comment. Yeah, I think it's a source-incompatible Swift change. It seems like it's an incompatible change that we are encouraged to make.

[Keith Miller]

(In reply to Keith Miller from comment #18)
> (In reply to Darin Adler from comment #17)
> > (In reply to Keith Miller from comment #16)
> > > (In reply to Darin Adler from comment #15)
> > > > Will this break source code compatibility for Swift programs that use these
> > > > methods?
> > > 
> > > No, it will just generate a warning if the users don't follow the
> > > nullability rules. At least according to
> > > https://developer.apple.com/swift/blog/?id=25.
> > 
> > I read that, and it’s not what it says. In Objective-C code it just creates
> > warnings. In Swift code, the change is different. I think it *is* a
> > source-incompatible Swift change. But maybe one that is worth doing.
> 
> Ah, I misread your comment. Yeah, I think it's a source-incompatible Swift
> change. It seems like it's an incompatible change that we are encouraged to
> make.

Investigating further it looks like we are trying to avoid source-incompatible changes. I'm gonna revert this for now.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 189085