[Parse/AST/Sema] Split parsing for typealias & associatedtype, allow typealias in protocols and generic constraints #1610
Conversation
|
This pull is the actual code/feature changes from #1557, after that pull got revised to just contain |
|
@gregomni Awesome! The standard library team has been waiting for this feature for a long time! If you want a stress test for your implementation, try adding this to the standard library (we wanted to do this for a log time!): extension Collection {
typealias Element = Iterator.Element
}and replacing |
gregomni
changed the title
| } | ||
|
|
||
| // expected-error @+1 {{'CB' typealias 'E' can not be used as a generic constraint; use associatedtype instead}} | ||
| func go1<T : CB, U : Col where U.Elem == T.E>(col: U, builder: T) { // expected-error {{'E' is not a member type of 'T'}} |
There was a problem hiding this comment.
Oh, why can't E be used in generic constraints? Is this a temporary limitation?
There was a problem hiding this comment.
Yes, it's hopefully just temporary - I'm trying to break these changes into smaller pieces. This pull avoids touching ArchetypeBuilder except for the temporary diagnosis with the limitation. I have some more work there (stashed at the moment...) that makes generic constraints work for straightforward alias cases (like the typealias Element = Iterator.Element that you mentioned), but it isn't quite correct yet. It'll be next up!
There was a problem hiding this comment.
I see -- looking forward to it!
gregomni
changed the title
|
@gribozavr Added a second commit with the (limited) generic constraint support. Would love some feedback on whether this seems like a good approach, and whether the intermediate step of parsing but not generically constraining with protocol type aliases ought to be committed separately first. Thanks! |
|
CC @jopamer @cwillmor @DougGregor for archetype builder changes. Please help to review this PR, it will unblock a massive simplification to the standard library's generic constraints! |
gregomni
changed the title
| ParserStatus Status; | ||
|
|
||
| Status |= | ||
| parseIdentifierDeclName(*this, Id, IdLoc, tok::colon, tok::equal, |
There was a problem hiding this comment.
Style: please indent this line 4 spaces since it's part of the |=.
|
These look good to me. @DougGregor - any objections to me merging? |
| // Resolve this nested type to this type alias. | ||
| pa = new PotentialArchetype(this, alias); | ||
|
|
||
| if (alias->hasUnderlyingType()) { |
There was a problem hiding this comment.
Shouldn't we call resolveDeclSignature() if the typealias doesn't have an underlying type yet?
|
Some comments above. The general direction looks very good (thanks for working on this!), but I'd like to avoid the need to walk through IdentTypeReprs... there should be more-general type checker mechanisms to handle these cases. |
|
@DougGregor Awesome. Thanks for the detailed feedback! |
|
Added a commit that fixes the indentation issues, better diagnosis for type aliases in protocols that look like they are Swift 2 intended-to-be-associatedtypes, and added tests. Thanks for the feedback @benlangmuir! |
|
@DougGregor I think all the feedback has been addressed, tests added, tests pass, etc., except that the added code in TypeCheckGeneric is still using IdentTypeRepr to do its job, and it handles 'Self' in a terrible way. Any suggestions there? |
| // If we're in a protocol and don't see an '=' this might be leftover Swift 2 | ||
| // code intending to be an associatedtype. | ||
| diagnose(TypeAliasLoc, diag::typealias_inside_protocol_without_type) | ||
| .fixItReplace(TypeAliasLoc, "associatedtype"); |
There was a problem hiding this comment.
Please indent here. Also, it would be better recovery to simply treat this as an associated type, so aside from the fact that we produce an error, we still build it as an AssociatedTypeDecl and the rest of compilation can continue under that assumption. One of the principles we try to follow with Fix-Its is to act like the user wrote what we suggest.
|
Getting very close! Just a few minor comments; per comment about on TypeCheckGeneric, did you try using substMemberTypeWithBase? |
|
@DougGregor I think this may be ready now? |
| AssociatedTypeLoc = consumeToken(tok::kw_typealias); | ||
| diagnose(AssociatedTypeLoc, diag::typealias_inside_protocol_without_type) | ||
| .fixItReplace(AssociatedTypeLoc, "associatedtype"); | ||
| Status.setIsParseError(); |
There was a problem hiding this comment.
You don't need to set Status.setIsParseError(), since you've recovered completely via the Fix-It. That way, we can continue as if the user never made the mistake at all.
|
With that one tweak to remove the unnecessary "setIsParseError()", please merge! I can hear the happiness from Standard Library-land. |
…protocols. Split up parsing of typealias and associatedtype, including dropping a now unneeded ParseDeclOptions flag. Then made typealias in a protocol valid, and act like you would hope for protocol conformance purposes (i.e. as an alias possibly involved in the types of other func/var conformances, not as a hidden generic param in itself). Also added support for simple type aliases in generic constraints. Aliases to simple (non-sugared) archetype types (and also - trivially - aliases to concrete types) can now be part of same-type constraints. The strategy here is to add type aliases to the tree of PotentialArchetypes, and if they are an alias to an archetype, also to immediately find the real associated type and set it as the representative for the PA. Thus the typealias PA node becomes just a shortcut farther down into the tree for purposes of lookup and generating same type requirements. Then the typealias PA nodes need to be explicitly skipped when walking the tree for building archetype types and other types of requirements, in order to keep from getting extra out-of-order archetypes/witness markers of the real associated type inserted where the typealias is defined. Any constraint with a typealias more complex than pointing to a single nested associated type (e.g. `typealias T = A.B.C.D`), will now get a specialized diagnoses.
|
@DougGregor Cool! Thanks for taking the time to go through all these changes. |
|
Removed the setIsParseError, squashed, merging... |
[Parse/AST/Sema] Split parsing for typealias & associatedtype, allow typealias in protocols and generic constraints
|
@gregomni @DougGregor @jopamer Thank you very much! 👏 I'll try it in the library now! |
|
W00t! |
|
Sorry @gregomni, I think I broke it: https://bugs.swift.org/browse/SR-937 |
|
@DougGregor apparently this isn't quite working (also should be under a flag until reviewed, right?) |
|
Yep, it's working in simple cases, but apparently isn't quite right. Working on it here: https://bugs.swift.org/browse/SR-938. |
Split up parsing of typealias and associatedtype, including dropping a now unneeded ParseDeclOptions flag.
Then made typealias in a protocol actually valid, which works great except that you can't use them in where clauses in generics now. So produce a specific diagnosis if you try to use them as if they were an associatedtype in a where clause, but otherwise they act like you would hope for protocol conformance purposes (i.e. as an alias possibly involved in the types of other func/var conformances, not as a hidden
generic param in itself).