[Diagnostics][Qol] SR-11295 Emit diagnostics for same type coercion.

[LucianoPAlmeida]

Emit diagnostic when try to perform a type coercion to the same type.

Resolves SR-11295.

cc @harlanhaskins

[xedin]

It would be great if you could convert this into ConstraintFix format (there are couple of examples already available) which is marked as warning. If you look at CSGen you'll see that when we generate constraints for CoerceExpr in visitCoerceExpr it creates Conversion constraint, you could add a new LocatorPathElt to uniquely identify that conversion comes from coercion and when it's simplified in matchTypes add a new warning if both type1 and type2 are equal.

[LucianoPAlmeida]

@xedin Thank's for all the guidance here :))
Just a question, at thematchTypes we are receiving type1 and type2 as type variables, so the comparison is not working right now (why I changed for WIP), there's a way to get the fixed types there? Or should we move this check to a point where the types are solved e.g. to CSApply?
edit: Correction, only type 1 is a type variable. e.g. In a _ = a as String expr, the type of a gets here as a type variable. Also, getFixedType passing the typeVar is returning nullptr.

[LucianoPAlmeida]

Hey guys, sorry for so many questions 😅
I think the implementation is almost ready to be able to be reviewed, but one issue that I'm not sure how to handle is when we having a coercion on a literal e.g. '_ = 1 as Int64' or '"Hello" as NSString',
The solver is infering the type by context so the literal is infered as a builtin initializer and the type is always equals.

---Type-checked expression---
(integer_literal_expr type='Int64' location=.../swift/test/expr/cast/sr11295.swift:20:5 range=[.../swift/test/expr/cast/sr11295.swift:20:5 - line:20:5] value=1 builtin_initializer=Swift.(file).Int64.init(_builtinIntegerLiteral:) initializer=**NULL**)

And an expression like '_ = 1 as Int64' which should not trigger the warning ends-up doing it.

So the question is if we should skip literals or adding the warnigs specials cases like only trigger the warning if coerceExpr->getSub():
is a LiteralIntegerExpr and we trying coerce to exactly Int. Other types are ok e.g. Int8, ...
is a StringLiteralExpr and we trying coerce to exactly String. Other types are ok e.g. NSString...
is a FloatLiteral ...

Or maybe there is a better way to handle those special cases...

edit: Was thinking that maybe the literals could have their own diagnostics like:
_ = "Hello" as String // String literal will be already infered as String in this context. Fix it remove it.
_ = "Hello" as NSString // This should be ok
_ = 1 as Int // Interger literal will be already infered as Int in this context. Fix it remove it.
_ = 1 as Int64 // This should be ok

[xedin]

@LucianoPAlmeida Here is the suggestion how to simplify this - instead adding new TypeCoercion element in addExplicitConversionConstraint you can do it directly in ConstraintGenerator::visitCoerceExpr in CSGen and avoid doing that if sub-expression is a literal.

[LucianoPAlmeida]

@xedin
Do you mean in CSGen do something like

auto fromType = CS.getType(expr->getSubExpr());
auto locator = [&]() {
  if (!isa<LiteralExpr>(expr->getSubExpr()))
     return CS.getConstraintLocator(expr, LocatorPathElt::TypeCoercion());
  return CS.getConstraintLocator(expr);
}();

CS.addExplicitConversionConstraint(fromType, toType,
                                   /*allowFixes=*/true, locator);

?
I've tried something like this before, but inside the addExplicitConversionConstraint this locator is added to a disjunctionConstraint and when this path was added there, it ended up hitting a assertion inside getDisjunctionChoice when called on CSApply here. I'm not sure why it happens, but that's what cause this path addition to endup being placed only on the coercion constraint inside addExplicitConversionConstraint.

[xedin]

@LucianoPAlmeida Yes, that's exactly what I mean. Also the problem you are facing in CSApply seems to be related to the fact that logic around determining choice locator has to match logic in CSGen otherwise it'll try to lookup overload without TypeCoercion element and fail.

[LucianoPAlmeida]

@xedin Still in progress here, sorry for taking so long to get this one ready (there are some edge cases to handle and many tests breaking) and also sorry for so many questions. But there some few issues that I'm not really sure how to handle.
One is if we have an expression like

let x: Bool = 3/4 as Float > 1/2 as Float 

Where the solver binds the type of the expression 3/4 and 1/2 as float,

Type variables:
  $T3 as Float @ locator@0x117b14a50 [Binary@swift/test/expr/cast/sr11295.swift:3:16 -> function result]
  $T1 as Float @ locator@0x117b14890 [IntegerLiteral@swift/test/expr/cast/sr11295.swift:3:15]
  $T2 as Float @ locator@0x117b14948 [IntegerLiteral@swift/test/expr/cast/sr11295.swift:3:17]
  $T0 as (Float, Float) -> Float @ locator@0x117b13e00 [OverloadedDeclRef@swift/test/expr/cast/sr11295.swift:3:16]

And by that the types the handled to match are always equals, which is right for what I could see in TypeChecker.rst since every time the solver makes a guess it re-attempts to simplify the system getting to match types with type1 == Float and type2 == Float, but with the actual logic for this diagnostic it will always endup being emmited. This kinda applies also to the literal cases mentioned before. So I'm not sure what kinda check can be done to avoid emmit it for those cases.

[xedin]

@LucianoPAlmeida Maybe a sensible thing would be to limit this warning to situation when coerced type comes from DeclRefExpr? Everything else is bound to attempt a type from right-hand side of a coercion as a binding just like what happens with operator 3 / 4 where overloads are going to be filtered to return Float.

[LucianoPAlmeida]

@LucianoPAlmeida Maybe a sensible thing would be to limit this warning to situation when coerced type comes from DeclRefExpr? Everything else is bound to attempt a type from right-hand side of a coercion as a binding just like what happens with operator 3 / 4 where overloads are going to be filtered to return Float.

I end-up limiting only to DeclRefExpr and also if the subExpr is another coercion implicit or not e.g. Double(1) as Double or 1 as Double as Double :)

One other question is if we should handle composition here. Like one of the tests that break is

func f(b: Base<Int>) {
    let _ = base as Base<Int> & AnyObject 
}

with the actual logic, the warning is emitted for the Base<Int> type in the coercion, but should we apply the fix-it to remove the Base<Int> & from the coercion?
edit: There is a way to know if the cast side of the CoercionExpr is a composition?

Another question is about the tests under the test/ClangImporter directory, where there are a lot of coercion as tests of c/objc types e.g. _ = DISPATCH_TIME_FOREVER as CUnsignedLongLong and since the type is actually the same the warning is being emitted here. But I'm not sure if is correct to update those tests with the expected warning, because is not the exact prorpuse of them. What do you think @xedin ?

Other thing is the case with type alias

typealias Double1 = Double
typealias Double2 = Double

func f(d: Double1) {
  let d2 = d as Double2 // emits: casting expression to 'Double2' (aka 'Double') doesn't change the type
}

The question here is since maybe is not obvious that Double1 is a typealias to Double too, maybe is a better diagnostic to mention both types so it would be more clear. So the message for when types are alias would be casting expression of type 'Double1' (aka 'Double') to 'Double2' (aka 'Double') doesn't change the type or some other message more clear indicating that both types are Double.

[xedin]

@LucianoPAlmeida Don't worry about it, I'll take a look! Regarding squash - it be great if you could squash changes which are logically together into one commit.

[xedin]

More of a meta-comment - It looks like we add more and more warnings so it might make sense to do a follow-up by adding diagnoseAsWarning() method to the FailureDiagnostic interface and rename classes like this to be Warning instead of Error...

[LucianoPAlmeida]

@xedin Thank's for the review, I'll make the changes as soon as possible :)

[xedin]

No problem!

[LucianoPAlmeida]

@xedin All addressed :)

[xedin]

@swift-ci please smoke test

[xedin]

@swift-ci please smoke test macOS platform

[xedin]

Thank you, @LucianoPAlmeida!

[LucianoPAlmeida]

Thank you @xedin for all the reviews here
Just one thing, there is a follow-up fixing up the places where this warning showed in the stdlib #27165 can you take a look on that too or cc someone from stdlib to review :)