[CSGen] Fix for exponential dictionary literal behavior in SR-3668

[colinhowell]

In ConstraintGenerator::visitDictionaryExpr(), if the dictionary_expr has a
contextual type which is also a dictionary type, use it to add a new type
constraint for each of the dictionary_expr's elements. This has a huge effect
on solver performance for dictionary literals with elements that have
overloading. The approach closely follows that used in
ConstraintGenerator::visitArrayExpr(), which has included a similar
optimization (from git commit a22b391) for over two and a half years.

I've also added a couple of new test cases which would trigger the bug without
this fix.

The problem in SR-3668, with exponential type-checking time for dictionary literals containing closures with overloaded operators, was not present for array literals. Close examination of the type-constraint generation for similar array literals revealed that constraints corresponding to the array_expr's contextual type were added for each of the literal's elements, greatly reducing the work needed by the constraint solver. This optimization was not being done for dictionary literals, and there seemed no reason not to give them a similar optimization. In doing so, I carefully followed the pattern and logic established for this optimization in ConstraintGenerator::visitArrayExpr() while still conforming to the way that dictionary_exprs are handled in ConstraintGenerator::visitDictionaryExpr(). In particular, the added constraints for the dictionary_expr's elements are tuple types of the contextual type's key and value types, just as they are normally handled in visitDictionaryExpr().

The result works as hoped; even long dictionary literals like those mentioned in SR-3668 are processed as quickly as similar array literals. I've added a couple of such dictionary literals as testcases in this commit. The commit passes swift/test. I also ran swift/validation-test and did get one unexpected failure (compiler_crashers/28657-unreachable-executed-at-swift-lib-ast-type-cpp-1344.swift), but this was the first time I'd ever run validation-test, and that failure may have been already present in master before this change, since it doesn't seem obviously related. (I will try running validation-test on master to double-check.)

@rudkx I'd appreciate your comments, since this bug was assigned to you.
@DougGregor Since you're listed as the gatekeeper for this entire area of the compiler, I'd also appreciate your feedback.

Resolves SR-3668.

[colinhowell]

@swift-ci Please smoke test

[CodaFi]

Joe Pamer's job is never done! As it relates to the first optimization he introduced, LGTM. The finer points may also require @xedin's eyes too.

@swift-ci please smoke test.

[CodaFi]

Normally failing the validation tests is bad. Here, it's a "good" thing because you'll need to unmark them as failing and move them to the fixed folder. You can automate this process

../llvm/utils/lit/lit.py -sv ../build/Ninja-DebugAssert/swift-macosx-x86_64/validation-test-macosx-x86_64/compiler_crashers/ | ./utils/resolve-crashes.py

[xedin]

Thanks, @CodaFi! @colinhowell, I will try to take a look tomorrow.

[slavapestov]

Based on the stack trace this is a non-deterministic crasher. It's not related to your change. We need to add 'REQUIRES: deterministic-behavior' to it in a separate patch. I'll do this later today if you don't get around to it.

Awesome patch by the way!

[xedin]

Thanks for awesome patch, @colinhowell! If you don't mind I think there is way to unify regular path with contextual in here, in the following way - currently elementTy is already computed from two dependent member types (the problem is, as you figured out, that doesn't account for presence of contextual type) and then used in the conversion constraint, I would suggest you add else to the if (!CS.getContextualType(expr)) which is going to change elementTy and both dictionaryKeyTy, dictionaryValueTy to be of contextual type (just like you do in your patch), which means that you can just leave the loop that creates conversion constraints as is, and unify code path for both cases. WDYT?

[colinhowell]

@xedin, there may indeed be a way to unify the two code paths, and doing so does look attractive. But after thinking a little about how the code needs to flow, it looks a bit more complex to me than you suggest, since the two paths, while similar, aren't fully identical.

Actually, since visitArrayExpr() already uses the same approach as my proposed patch, one should be able to unify its code paths in similar fashion. Since that function is a bit simpler than visitDictionaryExpr(), I'd prefer to look into how that might work first. Then a similar approach could be used on both functions, keeping the two similar in style.

Since unifying these code paths obviously adds potential for breakage, I think it should be done in a separate patch.

[xedin]

@colinhowell I'm not sure I follow what is going to be more complex, sorry if I'm being ambiguous, I didn't actually mean unify array and dictionary paths together, but rather path with and without contextual type in the visitDictionaryExpr.

Let me give you an example of what I mean - for loop in 1810 - 1818 is exactly the same as 1900 - 1909 with the only difference that one is using elementTy and other is using contextualDictionaryElementType (both are TupleType, and both are combined from key and value element types, in existing case it's DependentMemberType in yours it's contextual). In case there is contextual type present dictionaryTy type variable can be fixed to contextualDictoraryType and dictionaryKeyTy and dictionaryValueTy could be taken from isDictionaryType just like you do right now. It's also good to keep checking for type variables and member types in key/value because that would help diagnostics.

[colinhowell]

@xedin Sorry, you're not being ambiguous; I actually understood exactly what you meant. If anything, I seem to have confused you by digressing about how visitArrayExpr() and visitDictionaryExpr() have similar structure.

You have to remember that I have almost no experience with this code base, and so I'm being very cautious. I've been assuming that any operation I'm not familiar with may have unexpected side effects, and so I've been trying to ensure that the exact same sequence of operations will still be performed when I unify the code paths with and without a contextual type.

In particular, I figured that if I had a usable contextual type:

• I would have to assign it to dictionaryTy instead of creating the type variable at 1823-1824;
• dictionaryTy would have to be set to the contextual type before calling addConstraint() at 1826-1829;
• instead of executing the code at lines 1833-1837, I would have to assign dictionaryKeyTy and dictionaryValueTy from the key-value pair I got from ConstraintSystem::IsDictionaryType() (what does that declaration of locatorBuilder do, anyway? It doesn't seem to be used anywhere. Does it have side effects on locator, or is it just leftover cruft?)
• I would have to avoid executing the blocks at 1911 - 1924.

I could certainly manage all this by setting a flag as soon as I had determined that there was a usable contextual type. But it's possible I'm just being overly cautious, I don't know.

[xedin]

@colinhowell I understand, and we are here to help you out out 100% ! :) Thanks again for working on this and I don't mean to discourage you in any way!

Let me address of your points:

#1 - It's fine to create that type variable and assign a fixed type to it (before creating LiteralConfirmsTo constraint) using CS.assignFixedType(dictionaryTy, contextualDictionaryType), it's actually going to have the same effect as simply returning contextual type and it's always good to double check contextual type structure anyway...

#2 - Indeed, that's what you will have to do, and that's totally reasonable, so we get a nice code reuse as well! locatorBuilder looks redundant to me here, because it should be used in addConstraint but CS.getConstraintLocator is used instead, so you can simply remove it.

#3 - Actually both blocks of code are viable here with and without contextual type, that might be useful for diagnostics or if the contextual type is given has dependent member types for key/value, if everything is structurally fine with contextual type itself, they are going to be no-op anyway.

[colinhowell]

@xedin Thanks, that was a very helpful reply! :)

[xedin]

@colinhowell Happy to help, let you know if you have any other problems!

[slavapestov]

@swift-ci Please smoke test

[DougGregor]

This looks fantastic, thanks!

[rudkx]

@colinhowell Thanks so much for this patch!

I actually noticed this recently as I was looking into issues related to slowness with type checking arrays and browsing through both array and dictionary handling, but hadn't gotten around to fixing it yet.