Skip to content

AST: Fix Type::join() to do the right thing with class metatypes [3.1] #7100

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 29, 2017
Merged

AST: Fix Type::join() to do the right thing with class metatypes [3.1] #7100

merged 1 commit into from
Jan 29, 2017

Conversation

slavapestov
Copy link
Contributor

Fixes rdar://problem/30233451.

@slavapestov
Copy link
Contributor Author

@DougGregor Is this what you had in mind?

@slavapestov slavapestov changed the title AST: Fix Type::join() to do the right thing with class metatypes AST: Fix Type::join() to do the right thing with class metatypes [3.1] Jan 27, 2017
DougGregor
DougGregor requested changes Jan 27, 2017
View reviewed changes
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.

We should narrow this so that it does not produce joins where there isn't actually a subtyping relationship.

lib/AST/TypeJoinMeet.cpp
metatype2->getInstanceType());
if (!result)
return result;
return MetatypeType::get(result);
Copy link
Member

Choose a reason for hiding this comment

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

It's not always true that if T < U, thenT.Type < U.Type in our type system. Let's whitelist the cases where it's true:

  • Both instance types mayHaveSuperclass(), or
  • Both are existential metatypes

and not compute a join in the other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's a counter-example? (Non-existential) protocol metatypes?

Copy link
Member

Choose a reason for hiding this comment

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

Int.Type isn't a subtype of Int?.Type

@DougGregor DougGregor mentioned this pull request Jan 27, 2017
Merged
@DougGregor
Copy link
Member

Your revised master PR LGTM!

@slavapestov slavapestov force-pushed the fix-type-join-metatype-3.1 branch from e29544b to 4d61f48 Compare January 27, 2017 23:08
@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - e29544b924e2e68b7b4e62ac3b42a4e357e97edb
Test requested by - @slavapestov

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - e29544b924e2e68b7b4e62ac3b42a4e357e97edb
Test requested by - @slavapestov

@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@tkremenek tkremenek merged commit e4a064f into swiftlang:swift-3.1-branch Jan 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DougGregor DougGregor DougGregor approved these changes

Assignees

@ematejska ematejska

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@slavapestov @DougGregor @swift-ci @tkremenek @ematejska