Remove default argument from getContextualType

[Jumhyn]

#39412 added a distinction between contextual types as external to the constraint system, and contextual types as they should be used in constraints. We introduced the forConstraint argument with a default value to keep the PR focused on the issue at hand—this followup removes the default argument and audits the other locations where we call getContextualType to make sure that we are using the correct 'version' of the type.

[Jumhyn]

@xedin Most existing uses of getContextualType were just for diagnostic purposes where we don't want opened types. This part in inferClosureType is the only one that seemed questionable but since we only use the result from the contextual type (and do the opening once we have combined with the separately inferred param types) it seemed like we shouldn't get the opened version of the type here directly. WDYT?

[xedin]

This type is going to be used by DefaultClosureType constraint in ConstraintGenerator::visitClosureExpr. I'd suggest we actually pass a opened contextual type to inferClosureType and remove replaceInferableTypesWithTypeVars from it and resolveClosure.

[Jumhyn]

The use of replaceInferableTypesWithTypeVars in resolveClosure is for a type that comes from an AttachedPropertyWrapperTypeRequest, rather than a contextual type so that may require a different approach (unless I'm confused about what you're thinking).

[xedin]

I think I just got confused thinking about some other stuff related to resolveClosure, please disregard that bit :)

[Jumhyn]

Also in inferClosureType we don't use the contextual type info at all to infer the param types, we only look at the declared types. Similarly, we only use the contextual type for the result if we don't have a declared type. So if we passed in the already-opened type then we may end up with orphaned type variables in the event that inferClosureType doesn't end up using the contextual type in the constraint system. 🤔

[Jumhyn]

We already access the constraint system at that point so I don't think we need the callback, we can just call getContextualType directly:

      Type resultTy = [&] {
           // checks for explicit type
          
           // No explicit result type, let's see if there is a contextual one
           if (CS.getContextualType(closure,
                                    /*forConstraint=*/false)->is<FunctionType>()) {
               auto constraintTy = CS.getContextualType(closure,
                                                        /*forConstraint=*/true);
               return constraintTy->castTo<FunctionType>()->getResult();
           }
           ...
      }();

The problem is that as soon as we call CS.getContextualType(closure, true) to get the proper result type for a constraint we've generated type variables for the parameter type wshich will never get connected to the rest of the system.

[Jumhyn]

I think in your example opening would have to happen anyway because contextual type would be used in a contextual conversion constraint.

In that example we don't get the contextual type from getContextualType, we get it from getTypeForPattern via generateInitPatternConstraints, which pulls the type out from the TypedPattern directly and bypasses the contextual type map. So the type variables we end up hooking up to the system are different from the ones that we generate in getContextualType since getTypeForPattern calls replaceInferableTypesWithTypeVars itself.

[xedin]

Ugh, this is really-really bad but okay let's keep it the way it is.

[Jumhyn]

Yeah, I was not excited to discover that 😅

If you're okay with merging this as-is I can next look into whether it would be feasible to use the contextual type directly in generateInitPatternConstraints/getTypeForPattern to avoid this issue, but I suspect that will potentially have wider reaching effects that should be addressed separately from this PR.

[xedin]

Yeah, I think it's okay to merge it as-is, thank you!

[Jumhyn]

@swift-ci please smoke test

[Jumhyn]

@swift-ci please smoke test Windows Platform

[Jumhyn]

@swift-ci please smoke test Windows Platform

[Jumhyn]

@swift-ci please smoke test windows platform

[Jumhyn]

@swift-ci please smoke test windows platform

[Jumhyn]

@xedin Sorry, started a new job and let this one fall off the radar. Still good to merge?

[xedin]

Yeah, let's get this in, thank you, @Jumhyn!