Skip to content

Protocol typealias fixes #2992

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 5 commits into from
Jun 12, 2016

Conversation

slavapestov
Copy link
Contributor

Finally this fixes my earlier patch that got reverted:

14d0be2

This wasn't doing the right thing for TypeAliasDecls. NFC for now,
but exposed by some other changes I am working on.
@slavapestov
Copy link
Contributor Author

@swift-ci Please test

This is a big refactoring of resolveTypeInContext() which makes
the function clearer to understand by merging various special
cases and generalizing the logic for walking parent and superclass
contexts to cover more cases.

This improves typealiases in protocols a bit:

1) Previously a typealias in a protocol either had to be concrete,
   or consist of a single path of member types from Self, eg
   Self.A.B. Lift this restriction, so we can now write things like

protocol Fireworks {
  associatedtype Exploding
  typealias Exploder = Exploding -> [Exploding]
}

2) Protocol typealiases can now be accessed via qualified lookup
   on concrete types. Getting this working for unqualified lookup
   requires further refactorings which will be in a subsequent
   patch.
Avoid calling lookupMemberType() with an existential base
altogether.

With the previous resolveTypeInContext() patch, a compiler_crasher
regressed and hit this, because the behavior of lookupMemberType()
changed to return nullptr in fewer cases.
…n-types earlier

With the previous resolveTypeInContext() patch, a few compiler
crashers regressed with this problem, presumably because we were now
performing lookups in more contexts than before.

This is a class of problems where we would attempt a recursive
validation:

1) Generic signature validation begins for type T
2) Name lookup in type context finds a non-type declaration D nested in T
3) Generic signature validation begins for D
4) The outer generic context of D is T, but T doesn't have a generic
   signature yet

The right way to break such cycles is to implement the iterative
decl checker design. However when the recursion is via name lookup,
we can try to avoid the problem in the first place by not validating
non-type declarations if the client requested a type-only lookup.

Note that there is a small semantic change here, where programs that
were previously rejected as invalid because of name clashes are
now valid. It is arguable if we want to allow stuff like this or not:

class A {
  func A(a: A) {}
}

or

class Case {}
enum Foo {
  case Case(Case)
}

However at the very least, the new behavior is better because it
gives us an opportunity to add a diagnostic in the right place
later. The old diagnostics were not very good, for example the
second example just yields "use of undeclared type 'Case'".
In other examples, the undeclared type diagnostic would come up
multiple times, or we would generate a cryptic "type 'A' used within
its own definition".

As far as I understand, this should not change behavior of any existing
valid code.
@slavapestov slavapestov force-pushed the protocol-typealias-fixes branch from cfca3ed to 3127264 Compare June 11, 2016 23:37
@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test and merge

@swift-ci swift-ci merged commit 78b38cd into swiftlang:master Jun 12, 2016
@slavapestov slavapestov deleted the protocol-typealias-fixes branch June 12, 2016 08:22
fromType = resolver->resolveGenericTypeParamType(selfType);

if (assocType) {
// Odd special case, ask Doug to explain it over pizza one day
Copy link
Member

Choose a reason for hiding this comment

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

🍕

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