[MiscDiagnostics] Update diagnostics for IUO to Any coercion

[theblixguy]

This PR updates the diagnostics for implicit coercion from T! to Any.
Resolves SR-10199.

[theblixguy]

cc @xedin

[theblixguy]

Could you take another look @xedin? I ran the tests locally and have got a few failing ones, all for implicit coercions from IUO to Any. I'll fix them now

[xedin]

Looks much better now, few more comments inline and test-case is missing.

[theblixguy]

Thanks @xedin, I have made the changes and updated the existing tests. Not sure if I should add another test case because the existing ones are good enough? I think we lost some warnings for cases where we should be emitting a warning.

[xedin]

Still needs some work but looks much better now!

[theblixguy]

Thank you @xedin, I have made the changes you requested. I can't use getSelfForInitDelegationInConstructor() for a MemberRefExpr because the function expects an UnresolvedDotExpr as argument.

[xedin]

@theblixguy Looks like for the MemberRefExpr you don't need that check at all. It always uses Member locator and what ever decl was associated with it - https://github.com/apple/swift/blob/master/lib/Sema/CSGen.cpp#L993

[theblixguy]

@xedin Sounds good! However, there's one issue left now, which is that FromType and ToType are not always correct when we create the fix in CSSimplify. Consider this example:

func warnCollectionOfOptionalToAnyCoercion(_ a: [Int?], _ d: [String : Int?]) {
  takesCollectionOfAny(a, d)
}

func takesCollectionOfAny(_ a: [Any], _ d: [String : Any]) {}

Here, our FromType ends up being Int? in the fix, rather than [Int?]. It's losing the array for some reason

[xedin]

@theblixguy That's because matchTypes is recursive... Let me think if we could do anything to workaround. How much worse are diagnostics because of that? I think we should still point to the right place in the source at least?

[theblixguy]

@xedin We end up saying expression implicitly coerced from 'Int?' to 'Any' instead of expression implicitly coerced from '[Int?]' to '[Any]'. We point to the right source locations, it's just the types either lose the array or one level of optionality in some other cases. In some rare cases, we lose the entire diagnostic (probably because it only has 1 level of optionality, so Int? becomes Int and we lose the diagnostic)! Maybe we can store the original types somewhere so we can refer to them later.

[theblixguy]

Here's an idea from the top of my head: we can create a vector to hold a struct containing a locator and the two types. If we don't already have a struct in there with the same locator, we push one. Otherwise, we skip. Later when creating the fix, we can retrieve the struct (using the locator as "key") and extract the original types and pass it to the fix.

Something like

struct MatchTypesCacheItem {
  ConstraintLocator *locator;
  Type *type1;
  Type *type2;

  ...
}

std::vector< MatchTypesCacheItem > matchTypesCache;

[xedin]

@theblixguy I'm to a fan for trying to keep a stack of types like that in matchTypes because that's on the hot path...

Tell me one thing - in that example from your previous comment - what locator does warning have?

[theblixguy]

@xedin The locator is

locator@0x7fd1ea89bb38 [Call@/Users/suyashsrijan/Desktop/test.swift:2:3 -> apply argument -> comparing call argument #0 to parameter #0 -> generic argument #0

locator@0x7fd1ea89bad0 [Call@/Users/suyashsrijan/Desktop/test.swift:2:3 -> apply argument -> comparing call argument #1 to parameter #1 -> generic argument #1

[xedin]

@theblixguy Ok, that's good! This means that if you strip away generic argument part of the locator and simplify it, it would point to the expression which corresponds to the argument, what to try that?

[theblixguy]

@xedin Okay! This is only for a CallExpr (or DeclRefExpr in this case, which points to the argument), right? I can do CS.getConstraintLocator(expr, ConstraintLocator::FunctionArgument) and that gives the right diagnostic!

But this may not work if the scenario is different i.e. when we're returning [Int?]? Just thinking of what strategy to use when changing the locator path. Also, it's rather tricky to remove the path and re-construct the locator (tried with builder, removing the last element and recreating a locator - but the initialiser for the locator is private, should I make it public?).

[xedin]

@theblixguy You can't actually use CS.getConstraintLocator(expr, ConstraintLocator::FunctionArgument) because it's not always going to return want you think it would. I'm not actually talking about new getOverloadChoiceIfAvailable, that's already correct, but rather how to get fromType and toType for diagnostic message from constraint system instead of trying to save them in the fix.

My suggestion would be to try multiple different examples and see what locators are going to be and if it would be possible to deduce types from locators (e.g. you can find type variables by locator as well by iterating over TypeVariables array in ConstraintSystem). We already figured that in case of apply expressions (and not just calls) you can get both argument and parameter based on the information stored in the locator (ApplyArgToParam stores both positions), I think that's the case for member references as well.

[theblixguy]

Thanks @xedin for the suggestion! We don't always have all the type variables we need in CS. For example:

func foo(value x: Int?) -> Any {
 let y: Any = x
}

in this case, we'll only have one type variable in CS.getTypeVariables() - one for x, but none for y. When we have at least 2 type variables, it's really simple to get the types we want - when I tested with a few examples, the destination type ends up being the 1st TV and the source type ends up being the 2nd TV.

I am not sure if there is one solution for this - we probably need different approaches depending on what the locator is or how many type variables are there. Maybe storing the original types somewhere might be simpler.

[xedin]

Sorry I'm not say that we should away use type variables... I think that it might be okay to pinpoint types like new diagnostic would do e.g. instead of [Int?] mention Int? as long as diagnostic points to the right location in source. @jrose-apple WDYT?

[theblixguy]

Oh sorry, I misunderstood! We can probably use a combination of type variables, locators and perhaps anchor to determine the two types (I gave it a shot and it seems to work sometimes), but at that point, I would say it starts to feel like a total hack and we might as well just store the original types in a vector in the constraint system as I originally suggested. I am not sure if there's going to be any noticeable impact on performance because we will only store it once (per locator) and it might be useful in future fixes.

Also, while we have regressed slightly in terms of showing the correct type in the diagnostic, the other problem is we actually don't emit any diagnostic in some cases because the types we store in the fix aren't correct (ex: it lost one level of optionality, or we get the element type rather than array type, etc - due to the recursion in matchTypes). That's something which absolutely needs to be addressed.

Example:

/Users/suyashsrijan/Documents/swift-src/swift/test/Sema/diag_unintended_optional_behavior.swift:157:52: error: expected warning not produced
  takesCollectionOfAny([a[0]], ["test" : a[0]]) // expected-warning {{expression implicitly coerced from 'Int?' to 'Any'}}

Here we lost the optionality so we get Int as source type and Any as destination type and thus, no warning.

[xedin]

Let’s just fix this in MiscDiagnostics and take time to experiment with warning fix.

[jrose-apple]

I don't like losing the Array part, because there's no expression in that statement that has the type Int?. If it's hard to keep it intact, though, I think we could assuage my misgivings by rephrasing the text: "expression performs implicit coercion from 'Int?' to 'Any'".

[theblixguy]

I have updated the tests with the new diagnostic - ran it locally and all of them pass. @xedin could you run the smoke tests if everything looks good to you?

[xedin]

@swift-ci please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 5456455

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 5456455

[theblixguy]

Unrelated failure on Linux:

22:55:56 The following tests FAILED:
22:55:56 1 - TestFoundation (ILLEGAL)
22:55:56 Errors while running CTest
22:55:56 FAILED: CMakeFiles/test.util

[xedin]

@swift-ci please test Linux platform

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 13232be

[xedin]

@swift-ci please test Linux platform

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 13232be

[theblixguy]

Same unrelated failure with TestFoundation - the Swift tests passed though.

[xedin]

@swift-ci please test Linux platform

[xedin]

It was a tough day for Linux CI yesterday, should be resolved now :)

[theblixguy]

Thank you @xedin! The tests have passed now 🎉 I will create a PR now to cherry-pick this to the 5.1 branch.

[xedin]

No problem! Please cherry-pick a squashed commit I have merged.