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

Adds support for NSUnsupported to indicate non-availiability, and use annotations #1150

Merged
merged 1 commit into from
Aug 3, 2017

Conversation

alblue
Copy link
Contributor

@alblue alblue commented Aug 1, 2017

As suggested in https://lists.swift.org/pipermail/swift-corelibs-dev/Week-of-Mon-20170724/001254.html it would be desirable to have a differential macro (rather than NSUnimplemented()) to indicate whether the functionality is missing by design in this library, to distinguish it from things that are not yet implemented.

This change adds NSUnsupported() as a variant of NSUnimplemented() that has a different error message, and allows the code to be differentiated at the point of definition, as well as uses availability macros on specific methods and types known to not exist on other platforms.

Note that marking the APIs as unavailable instead of deprecated would mean that they would cause compile-time errors. We may want to do that in a future release.

@alblue
Copy link
Contributor Author

alblue commented Aug 1, 2017

@swift-ci please test

@alblue
Copy link
Contributor Author

alblue commented Aug 1, 2017

Looks like an unrelated failure:

  switch (name.getKind()) {

@swift-ci please clean test

@alblue
Copy link
Contributor Author

alblue commented Aug 1, 2017

@swift-ci please clean test

@alblue
Copy link
Contributor Author

alblue commented Aug 1, 2017

@swift-ci please test

2 similar comments
@alblue
Copy link
Contributor Author

alblue commented Aug 2, 2017

@swift-ci please test

@pushkarnk
Copy link
Collaborator

@swift-ci please test

@ianpartridge
Copy link
Collaborator

As discussed on the mailing list, I support this general approach but think we should only add NSUnsupported() in this PR, not start using it as well. We can adopt it in future PRs, perhaps post Swift 4.

@alblue
Copy link
Contributor Author

alblue commented Aug 2, 2017

@ianpartridge There's no reason to not start using NSUnsupported() in place of NSUnimplemented() as it doesn't change the semantics (other than the failure error message). The @available macros are a separate implementation detail though; we could add those subsequently. But AFAIK the swift-4.0 branch has diverged from master anyway, so we're really targeting swift.next here anyway, not the 4.0 branch - at least, that's my understanding @parkera

@spevans
Copy link
Collaborator

spevans commented Aug 2, 2017

@alblue I think the automerge into swift-4.0 branch for corelibs-foundation is still active (according to my github feed)

@ianpartridge
Copy link
Collaborator

Yes it is: a248ca2

@ianpartridge
Copy link
Collaborator

@alblue The semantics are changing though - if NSUnsupported() doesn't mean anything different to NSUnimplemented() then why add it? Deciding which APIs become NSUnsupported() will involve some discussion...

@alblue
Copy link
Contributor Author

alblue commented Aug 2, 2017

@ianpartridge The key difference between NSUnsupported() and NSUnimplemented() is that one may be implemented in the future, the other one won't be. And yes, deciding which APIs need to be annotated is part of the reason for kicking off this thread and change set. The thing is, NSPort and NSPortMessage are both mach-specific concepts, which doesn't make sense on non-mach platforms; and as per pull #857 the Bundle.unload isn't going to be implemented in this project either.

Finally, the goal of having a separate string is so that git grep NSUnimplemented() trends to zero, which won't be the case while it exists in functions that will never be supported ...

@ianpartridge
Copy link
Collaborator

I totally agree about making sure that NSUnimplemented() can trend to zero. And NSUnsupported() seems like a good idea.

But I wonder how we are going to decide which APIs become NSUnsupported() and which are just removed from the project. After all, Apple never included NSUserActivity (for example) here in the first place. Should we be adding those APIs and marking them NSUnsupported()? What are we saying the difference is between Foundation APIs that are missing and those that are marked NSUnsupported()?

@alblue
Copy link
Contributor Author

alblue commented Aug 2, 2017

Good question for @parkera – should we just remove the types instead of deprecating them?

@parkera
Copy link
Member

parkera commented Aug 2, 2017

If the type is there, but marked as unavailable (not deprecated), then it's a pretty clear signal that we do not intend for it to be used, as opposed to it looking like we somehow forgot it. Also, portability efforts would probably be easier.

On the other hand, it could add a bunch of noise.

@parkera
Copy link
Member

parkera commented Aug 2, 2017

There are a bunch of classes on Darwin that we didn't bother even stubbing out here, and it seems like a waste of time to go find all of them (plus categories on Foundation types that higher level frameworks provide on Darwin) and add them, just to mark them unavailable.

When developing cross-platform applications, an `NSUnimplemented()`
is currently used to identify when a method is not and will not be
implemented.

Since `NSUnimplemented()` is used to also mean 'not yet implemented',
using a distinct name for the same functionality will allow the
two states to be distinguished, in the same way that
`NSRequiresConcreteImplementation()` is used to indicate that a
subtype must provide the behaviour.
@alblue
Copy link
Contributor Author

alblue commented Aug 3, 2017

To simplify the contribution, I've rebased and just have the NSUnsupported() macro, which we can then add to types/functions subsequently. I can then push subsequent changes on individual types so we can debate those as needed.

@swift-ci please test

@ianpartridge
Copy link
Collaborator

@swift-ci please test

@ianpartridge ianpartridge merged commit 8615f82 into apple:master Aug 3, 2017
@ianpartridge
Copy link
Collaborator

Thanks @alblue

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.

5 participants