Bug 189047

Created attachment 348291 [details]
Patch

Comment on attachment 348291 [details]
Patch

r=me

Created attachment 348306 [details]
Patch for landing

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.

Created attachment 348308 [details]
Patch for landing

Comment on attachment 348308 [details]
Patch for landing

Clearing flags on attachment: 348308

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

All reviewed patches have been landed.  Closing bug.

<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.

(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.

Reopening to attach new patch.

Created attachment 348317 [details]
Patch

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

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

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

(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.

(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.

(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.

(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.

Re-opened since this is blocked by bug 189085