[CS] Don't crash when default argument is magic literal and types don't match

[theblixguy]

This PR fixes a compiler crasher which happened when the default argument was a magic literal (like #line) and the type conversion failed. For example:

func foo(x: Int) {}
func foo(line: String = #line) {}
foo()

Resolves SR-11085.

[theblixguy]

cc @xedin

We're crashing because we're using CTP_CannotFail when calling typeCheckParameterDefault and when the types don't match, we end up calling diagnoseContextualConversionError where we hit an assertion if we have CTP_CannotFail.

[xedin]

@DougGregor or @slavapestov Do you guys know why DeclChecker::visitFuncDecl doesn't invoke checkDefaultArguments like it does for subscript and enum? Is that oversight or intentional?

[theblixguy]

Hmm I guess that might be better than this check. I noticed we do call checkDefaultArguments in typeCheckAbstractFunctionBodyUntil, but maybe that’s not enough.

[slavapestov]

I'd prefer it if we fixed the error by ignoring the invalid default argument entirely in the constraint system or declaration checking instead of handling failure in CSApply. Applying a solution should never fail.

Note that checkDefaultArguments() is called for declarations in primary files only.

[xedin]

I totally I agree, my idea behind asking about checkDefaultArguments was in hope that we could have avoided adding this declaration to overload set, but it seems like we just need to build more constraints to check defaults as part of the solving. All of the typeCheckExpr calls inside of CSApply should be phased out one by one.

[theblixguy]

@xedin What would be the best place to do this?

[xedin]

@theblixguy Probably while arguments to parameters are being matched in matchCallArguments we need to double check that default expr is valid for each missing argument, that's going to be interesting though because you'd have to generate constraints for an expression which is not a part of original AST and then drop everything once the scope is done.

You'll need a new storage in ConstraintSystem to keep that up so it could be rolled back later just like function builders do (since they also generate new constraints once they detect that one of the parameters has @functionBuilder attribute).

[theblixguy]

Hmm why do we need to do this for magic literal expressions but not others? For example: func foo(line: String = 22) {} throws an error as expected and doesn't crash.

[xedin]

I think we need to do that in general. You might want to start with editor placeholder which has very similar problem, if you look at ExprRewritter::visitEditorPlaceholderExpr you'll see that it does call typeCheckExpression there but fixing that would be easier because it could be done in CSGen, idea is the same though.

[theblixguy]

Okay! Could we do the same for functions or function calls i.e. generate a constraint to match (bind/equal?) the argument to the parameter in CSGen? Or do we have to do it in ConstraintSystem::TypeMatchResult constraints::matchCallArguments? I was hoping to do this decl checking but seems like that's not an option.

If the only way to do it is in matchCallArguments, then do I need to loop over the arguments and add a new constraint to match it to the parameter type?

[xedin]

Can't really use matchCallArguments directly in CSGen but it pretty much does all of the work for looping and determining which arguments are defaulted for you, so all you need to get that default expr, generate constraints for it and let the solver do the rest :)

[theblixguy]

@xedin Hmm we're not actually calling matchCallArguments in this case. Also, I have access to the args and params inside matchCallArguments but not the expression. That should be enough to create an Equal constraint?

[slavapestov]

@theblixguy @xedin I think this problem only exists with magic literals, not in general.

When I define a function like func foo(x: Int = 123), SILGen emits a small thunk to return the default argument expression. A call foo() will reference this thunk. If the expression is ill-typed, eg func foo(x: Int = "hi") a call to foo() from another source file does not touch that expression at all. So it doesn't matter if its type hasn't been checked -- we can still safely assume that the thunk's type matches the parameter's type.

However if you have a magic literal expression, SILGen will emit that expression at the call site, instead of referencing a thunk. So if checkDefaultArguments() hasn't run, SILGen might try to emit an invalid, unchecked expression.

Checking the expression type every time matchCallArguments() runs is fine, as long as you only do it for magic default arguments. You don't have to create a whole constraint system either, there's only a fixed set of them and each one has a known type.

A better approach would be to use a request evaluator request, to only check magic default arguments once for each unique function referenced.

[xedin]

@theblixguy I was talking about this place https://github.com/apple/swift/blob/master/lib/Sema/CSSimplify.cpp#L964, there matchCallArguments detects that parameter hasn't been fulfilled with an argument. Checking validity of the default expression shouldn't require creating a new constraint system either, solver just needs to generate new constraints just like function builders do but it's easier because we don't have to record newly generated expressions.

[theblixguy]

@xedin Okay, but if parameterBindings[paramIdx] is empty then how do we get the type of the argument (i.e. the default value)? I can get the type of the parameter via params[paramIdx]. Do we need to cast callee to a function and then extract its parameters/arguments? Also, to add the new constraint, is this enough: cs.addConstraint(ConstraintKind::Equal, paramTy, defaultValueTy, locator)?

auto allParams = dyn_cast<FuncDecl>(callee)->getParameters()->getArray();
for (auto param : allParams) {
  if (param->isDefaultArgument()) {
      auto defaultValueExpr = param->getDefaultValue();
      auto defaultValueTy = cs.getTypeChecker().typeCheckExpression(defaultValueExpr, param->getDeclContext());
      auto paramTy = param->getType();
      cs.addConstraint(ConstraintKind::Equal, paramTy, defaultValueTy, locator);
  }
}

This works for me in the meantime, because the default expression does not have a type yet which is why I need to call typeCheckExpression.

[xedin]

@theblixguy If there is no argument it means that parameter is going to use a result of default expression as argument which is exactly the situation which requires us to make sure that default expression is valid. We need to generate constraints for default expression and create defaultType ArgumentConversion paramType constraint.

In this situation we don't really need a type-check call, we can just do generateConstraints(defaultValueExpr, param->getDeclContext()) and retrieve a types from expression cache ConstraintSystem::getType(Expr *) const e.g. defaultType = getType(defaultValueExpr).

[theblixguy]

@xedin Okay, does it look good to you now?

[xedin]

I think that's the right approach, we just need to get some details straight.

[theblixguy]

@xedin Thanks, I have addressed your comments!

[theblixguy]

@xedin I have created a new fix for this to emit a tailored diagnostic.

[xedin]

Awesome! Couple of small fixes and we should be good to go!

[theblixguy]

@xedin Thanks, I have addressed your comments!

[xedin]

@swift-ci please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - c6e96869d5209ab3e044688ce1d10581444cc242

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - c6e96869d5209ab3e044688ce1d10581444cc242

[theblixguy]

@xedin Can you re-run the CI? Fixed a crash

[xedin]

@swift-ci please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 2eeb2e68b70a96e35b75eabc137c94852d0d7067

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 2eeb2e68b70a96e35b75eabc137c94852d0d7067

[theblixguy]

@xedin Can you run the CI again? I addressed a comment from Hamish. Hopefully no more changes required after this!

[xedin]

@theblixguy could you please squash all of the smaller commits before we merge this?

[theblixguy]

@xedin Done! (took way too long for some reason :/)

[xedin]

Perfect, thank you!

[xedin]

@swift-ci please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 0a32ef01d3516837441dad8e37fff747b6735049

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 0a32ef01d3516837441dad8e37fff747b6735049

[xedin]

@theblixguy Sorry but you'd have to resolve a conflict in CSFix...

[theblixguy]

@xedin Done!

[xedin]

@swift-ci please test

[xedin]

@swift-ci please test source compatibility

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 1a1bff4

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 1a1bff4

[theblixguy]

@xedin Thanks, should I cherry-pick this to 5.1?

[xedin]

Unfortunately this is a bit too late for 5.1

[theblixguy]

Okay, no problem :)