[ConstraintSystem] Fix not warn about some redundant coercions

[LucianoPAlmeida]

@xedin
I just took a continuation on your #27877
Fixed the cases on tests and added the rdar as test case is as_coercion.swift
But if you think is better to revert the original PR and fix it there, let me know :)
I'm staying tuned on github notifications all day for this, feel free to just ping me for anything 👍

rdar-56608085

References: #27880 #26710

[LucianoPAlmeida]

There are some SourceKit tests failing locally, but I think is unrelated

    Swift-Unit :: SourceKit/SwiftLang/./SourceKitSwiftLangTests/CursorInfoTest.CursorInfoMustWaitDueDeclLoc
    Swift-Unit :: SourceKit/SwiftLang/./SourceKitSwiftLangTests/CursorInfoTest.CursorInfoMustWaitDueOffset
    Swift-Unit :: SourceKit/SwiftLang/./SourceKitSwiftLangTests/CursorInfoTest.CursorInfoMustWaitDueToken
    Swift-Unit :: SourceKit/SwiftLang/./SourceKitSwiftLangTests/CursorInfoTest.CursorInfoMustWaitDueTokenRace
    Swift-Unit :: SourceKit/SwiftLang/./SourceKitSwiftLangTests/CursorInfoTest.EditAfter
    Swift-Unit :: SourceKit/SwiftLang/./SourceKitSwiftLangTests/CursorInfoTest.EditBefore
    Swift-Unit :: SourceKit/SwiftLang/./SourceKitSwiftLangTests/CursorInfoTest.FileNotExist
    Swift-Unit :: SourceKit/SwiftLang/./SourceKitSwiftLangTests/EditTest.DiagsAfterEdit
    Swift-Unit :: SourceKit/SwiftLang/./SourceKitSwiftLangTests/EditTest.DiagsAfterReopen

[xedin]

I didn't merge my PR because I think this case is supposed to warn, $0 is a float based on contextual type, it can't be anything else...

[xedin]

I think we should revert and take a closer look at how to fix some of the cases here, preserving ones like let _: (Float) -> Void { $0 as Float }...

[xedin]

The test in question is validation-test/Sema/type_checker_perf/fast/rdar17077404.swift just remove REQUIRES line from it and run.

[LucianoPAlmeida]

I think we should revert and take a closer look at how to fix some of the cases here, preserving ones like let _: (Float) -> Void { $0 as Float }...

Sure. btw Thank you for reverting this :)

I can work on that, but still not sure how to do it. I mean ... how to know that the solver is inferring the type from context, but not from the coercion it self. I'll think about it ...
Also, sorry for all the trouble, maybe we can add something that will catch this situation on the CI as part of the new PR too.

[LucianoPAlmeida]

The test in question is validation-test/Sema/type_checker_perf/fast/rdar17077404.swift just remove REQUIRES line from it and run.

Ohh yeah, I just meant in the CI to catch this on smoke test or something ...

[xedin]

It's possible to distill that perf test down to something which could be added to regular tests.

[LucianoPAlmeida]

I'll look into this in the weekend
And again, thank you for reverting the PR 👍

[xedin]

No problem! I think we need to check whether DeclRefExpr associated with sub-expression does have a concrete type associated with it and only diagnose when it does, tthat would avoid trying to diagnose parameters associated with generic arguments...

[LucianoPAlmeida]

No problem! I think we need to check whether DeclRefExpr associated with sub-expression does have a concrete type associated with it and only diagnose when it does, tthat would avoid trying to diagnose parameters associated with generic arguments...

Humm ... but that is was being done here?

     // If coerced type is not known upfront let's not warn
     // because the type could be inferred from coercion.
     auto coercedType = cs.getType(expr->getSubExpr());
     if (coercedType->is<TypeVariableType>())
       return false;

And the problem is even if it don't have a concrete type, maybe it could be inferred by a context that is not the coercion itself. Like in the broken test

let _: (Float) -> Void { $0 as Float } // This $0 `DeclRefExpr here don't have a concrete type, but it should diagnose it here as you mention. because the type solver is infering by context, but not the coercion itself.

[xedin]

Kind of, but I think we can be smarter about it... Essentially we need to figure out where the type of the sub-expression came from. After thinking about this some more I think we would have to change the way types associated with type variables are tracked in constraint system, similar to how we do PotentialBindings where each type has location information associated with it. That's probably the best way to distinguish situations like this. Doing that would also help us in other places e.g. figuring out whether a "hole" has been bound to (or defaulted to) some type.

/cc @hborla

[LucianoPAlmeida]

Essentially we need to figure out where the type of the sub-expression came from.

@xedin Maybe we don't exactly know where it comes from... I fixed the issue with a simple check for contextual type, which works because if the type is inferred by context, at the point we are performing the attempt there will be a contextual type on the system. But if the type is inferred by the coercion itself, at this point there will a nullptr contextual type there.

    auto coercedType = cs.getType(expr->getSubExpr());
    if (coercedType->is<TypeVariableType>() &&
        cs.getContextualType().isNull()) {
      return false;
    }

That seems a reasonable solution to me, but I'm not sure, since I don't have enough experience to afirm if this is 100% correct. So let me know your thoughts on this :)
But I've openned the PR anyways because we can continue this discussion there 👍