Skip to content

[Sema] Disallow conditional conformances on objective-c generics. #14628

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

Merged
merged 2 commits into from
Feb 15, 2018

Conversation

huonw
Copy link
Contributor

@huonw huonw commented Feb 14, 2018

There's no way to look up information about objective-c generic
parameters, meaning the runtime cannot check same-type constraints or
conformance requirements, so dynamic casts cannot work. We want to keep
the static and dynamic systems the same, so we have to disable this
functionality from the start (i.e. no conditional conformances on
objective-c types :( ).

Fixes rdar://problem/37524969.

(Plus moving TypeOrExtensionDecl.)

@huonw huonw requested a review from DougGregor February 14, 2018 05:22
@huonw huonw force-pushed the no-conditional-objc-generics branch from 3f3cbd3 to 151e6d8 Compare February 14, 2018 12:36
@huonw
Copy link
Contributor Author

huonw commented Feb 14, 2018

@swift-ci please smoke test.

Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nitpicks about your refactoring (which doesn't seem to be used).

TypeOrExtensionDecl(ExtensionDecl *D);

/// \brief Return the contained *Decl as the Decl superclass.
class Decl *getAsDecl() const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray class here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field is called Decl: clang specifically said I should write class Decl to disambiguate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I guess I would have just renamed the field.


bool operator==(TypeOrExtensionDecl rhs) { return Decl == rhs.Decl; }
bool operator!=(TypeOrExtensionDecl rhs) { return Decl != rhs.Decl; }
bool operator<(TypeOrExtensionDecl rhs) { return Decl < rhs.Decl; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this one makes sense to provide.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just mindlessly duplicating the PointerUnion API.

//===----------------------------------------------------------------------===//
//
// This file defines the TypeOrExtensionDecl struct, separately to Decl.h so
// that this can be included in files that Decl.h includes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably doesn't actually work, because PointerUnion needs to know the alignment of the types in it to know where to stick spare bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to compile.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It compiles as long as you also include Decl.h before the template actually has to be instantiated, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do I address this then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Untangle the headers? Or…ah, it looks like we have swift/AST/TypeAlignments.h for this. ASTNode.h uses it in a similar way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat; changed it to use that header.

@huonw
Copy link
Contributor Author

huonw commented Feb 14, 2018

which doesn't seem to be used

It's used in PrintOptions.h and various things that use that.

@jrose-apple
Copy link
Contributor

Sorry, I know the refactoring is used where it was used before. It doesn't seem to be used in any new places.

@huonw
Copy link
Contributor Author

huonw commented Feb 14, 2018

Ah yeah, it's a follow up to #14554 (comment) but including it here rather than a separate PR since CI is apparently getting hammered...

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code to ban conditional conformances for types using the Objective-C generics model LGTM.

@DougGregor
Copy link
Member

Ugh, is there a weird corner case where we are trying to make a nested type of an ObjC generic class conditionally conform to a protocol, but this code wouldn't notice?

@huonw huonw force-pushed the no-conditional-objc-generics branch from 151e6d8 to da6feab Compare February 15, 2018 05:33
@huonw
Copy link
Contributor Author

huonw commented Feb 15, 2018

Updated to handle nested types.

@swift-ci please smoke test.

There's no way to look up information about objective-c generic
parameters, meaning the runtime cannot check same-type constraints or
conformance requirements, so dynamic casts cannot work. We want to keep
the static and dynamic systems the same, so we have to disable this
functionality from the start (i.e. no conditional conformances on
objective-c types :( ).

Fixes rdar://problem/37524969.
@huonw huonw force-pushed the no-conditional-objc-generics branch from da6feab to dd845c4 Compare February 15, 2018 05:59
@huonw
Copy link
Contributor Author

huonw commented Feb 15, 2018

@swift-ci please smoke test and merge.

3 similar comments
@huonw
Copy link
Contributor Author

huonw commented Feb 15, 2018

@swift-ci please smoke test and merge.

@huonw
Copy link
Contributor Author

huonw commented Feb 15, 2018

@swift-ci please smoke test and merge.

@huonw
Copy link
Contributor Author

huonw commented Feb 15, 2018

@swift-ci please smoke test and merge.

@huonw
Copy link
Contributor Author

huonw commented Feb 15, 2018

@swift-ci please smoke test

@huonw
Copy link
Contributor Author

huonw commented Feb 15, 2018

@swift-ci please smoke test Linux platform

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