[Sema] Copy key path component types when merging solutions #38648
Conversation
|
@swift-ci Please smoke test |
ahoppen
force-pushed
the
pr/apply-key-path-results-when-merging-solutions
branch
from
1055a21 to
ae312b0
Compare
|
IIUC this is only a problem for result builders, because we might determine the type of a key path in a dependent component. Prior to this change we weren’t copying that type over to the constraint system again to make sure it’s included in the merged solution. Thus, the type lookup would fail when doing code completion. I updated the PR a fair bit because I forgot to reset |
|
@swift-ci Please smoke test |
| @@ -570,6 +576,19 @@ ConstraintSystem::SolverScope::~SolverScope() { | |||
| } | |||
| truncate(cs.addedNodeTypes, numAddedNodeTypes); | |||
|
|
|||
| // Remove any node types we registered. | |||
| for (unsigned i : reverse(range(numAddedKeyPathComponentTypes, | |||
| cs.addedKeyPathComponentTypes.size()))) { | |||
There was a problem hiding this comment.
In what situations previously assigned types could be re-assigned? Does it actually happen that constraints are generated twice for a particular key path expression in result builder body?
There was a problem hiding this comment.
I don’t know of any, I just followed the same pattern that we have for NodeTypes. I suspect that the same situations that apply for NodeTypes also apply for KeyPathComponentTypes.
There was a problem hiding this comment.
I understand, would be an interesting separate experiment to remove it from both places because i think overrides like that shouldn’t be allowed…
There was a problem hiding this comment.
I can try that in a follow-up PR.
There was a problem hiding this comment.
I just tried removing addedNodeTypes and it causes all sorts of failures in the test suite. IIUC we assign new type variables to a call expression as we try different overloads for it, for example.
If we need to keep addedNodeTypes, I would argue to also keep addedKeyPathComponentTypes so they share the same conceptual implementation.
There was a problem hiding this comment.
Let’s merge this yes. I was actually talking about removing only if statement in rollback phase and leaving only .erase :)
There was a problem hiding this comment.
I tried that and it also fails. There’s one case where we just set a node’s type to the one it already has. If we remove erase it again afterwards, that causes crashes. But even when ignoring that special case, things fail. For example test/Constraints/sr13183.swift. I haven’t looked too deeply into why that is the case. You’ve probably got a better understanding of the constraint system to actually judge what’s going on there and if it’s intended.
There was a problem hiding this comment.
Interesting, I going to take a closer look at that…
In 2eeff36 I forgot to copy key path component types when applying a solution to the constraint system. That caused a crash in key path code completion. Fixes rdar://81118700 [SR-14979]
ahoppen
force-pushed
the
pr/apply-key-path-results-when-merging-solutions
branch
from
ae312b0 to
f42f961
Compare
|
@swift-ci Please smoke test |
|
@xedin Are you OK with me merging this and looking at removing |
In #38389 I forgot to copy key path component types when applying a solution to the constraint system. That caused a crash in key path code completion.
Fixes rdar://81118700 [SR-14979]