[Sema] Extend callee diagnosis to multiple generics #1319
Conversation
|
@slavapestov Thanks for the last merge! Here's the next update. |
| SmallVector<Type, 4> &substitutions) | ||
| : dc(dc), archetypes(archetypes), substitutions(substitutions) {} | ||
| GenericVisitor(DeclContext *dc, TypeSubstitutionMap &archetypesMap) | ||
| : dc(dc), archetypesMap(archetypesMap) {} |
There was a problem hiding this comment.
Can you fix the indentation so that : is moved over by 2 or 4 spaces?
|
It looks like an improvement but I don't quite understand what you're doing with archetypesMap there. Perhaps just a matter of adding more comments? |
gregomni
force-pushed
the
generic-func-args
branch
from
9eb6f4e to
207f0ce
Compare
|
Did your suggestions on indentation/empty() and tried adding a couple more comments, although I still feel like those could be improved. |
Straightforward extension of the previous work here to handle callees with multiple generic parameters. Same type requirements are already handled by the ArchetypeBuilder, and protocol conformance is handled explicitly. This is still bailing on nested archetypes. Pre-existing tests updated that now give better diagnoses.
gregomni
force-pushed
the
generic-func-args
branch
from
207f0ce to
f8dbb20
Compare
|
Okay, improved comment, squashed/pushed. It wasn't that the replacement archetype should've been matched to the original, it should've been matched to itself, and hopefully the new comment explains adequately why. |
|
Ok. As a future improvement, do you think there's any way to express this without destructuring SubstitutedType? I'm wondering if instead CSDiag should be passing some more information down from the ApplyExpr -- substitutions are already recorded there. There are only a couple of places left that still look at SubstitutedType and they all feel a bit hacky -- I'd like to rip it out some day. |
|
Yes, I think you could ask the constraint system for all its type variables, and then for each variable, add to the map its archetype (if any) -> its fixed type (if any). And then you wouldn't need the substitution type restructuring at all. |
|
archetypes already are "fixed types". You're thinking of type variables? The constraint system introduces archetypes when a type variable is bound to a generic parameter in a signature. The signature is "opened" and archetypes are constructed to encapsulate the conformances of that generic parameter, as well as any nested associated types. |
|
Sorry, right. Was looking through the TypeVariableType public API and misreading what that did. I guess in order to do away with destructuring the SubstitutionTypes, I'd need to walk through the CS constraints and get bindings - that's essentially the info that I want in the archetypeMap. In any case, the info has to be in the CS, the SubstitutionTypes in parameters passed in here are just a convenient side-effect that I'm talking advantage of, not something that is irreplaceable. |
|
@lattner What do you think? |
|
@gregomni I think if the current approach passes tests we can merge it anyway since it improves the diagnostics. But it does seem like you've identified something here that could be generalized and cleaned up, and it would be great if you could investigate it further. |
|
@swift-ci Please test |
[Sema] Extend callee diagnosis to multiple generics
Straightforward extension of the previous work here (last pull was #1229) to handle callees with multiple generic parameters. Same type requirements are already handled by the ArchetypeBuilder, and protocol conformance is handled explicitly. This is still bailing on nested archetypes.
Pre-existing tests updated that now give better diagnoses.