[Sema] Produce expected diagnostic for invalid operator usage in loop

[Rajveer100]

Resolves #79999

Snippet:

func foo(_ a: Int) -> [Int] { [] }
for _ in foo(-) {}

[Rajveer100]

This may be totally incorrect, let me know the appropriate place/direction for this.

[jameesbrown]

While the current diagnostic is expected, I think we could improve it to be more consistent with other argument mismatches. For example, when passing an argument of the wrong type to a function, we get:

func foo(_ str: String) {}
foo(10) // Cannot convert value of type 'Int' to expected argument type 'String'

A similar diagnostic should be produced when passing an operator as an argument in a context where it is not valid, e.g.,

foo(+) // Cannot convert operator '+' to expected argument type 'String'  

Try tracing where and how AllowArgumentMismatch is created and how this results in emitting a diagnostic in ArgumentMismatchFailure::diagnoseAsError(), as that should give you a good intuition for where the missing piece might be.

Additionally, we should add a test case. Let me know if this seems like a sensible direction.

[xedin]

Sorry but this is not the right place for this fix. When you look at the -debug-constraints output for foo(-) expression, you'd notice that we end up with multiple solutions that all have AllowArgumentMismatch fix. The ConstraintFix has a special method for cases like this diagnoseForAmbiguity, overload needs to be added to AllowArgumentMismatch and the diagnostic logic need to be added there. There is also a question of what actual diagnostic we need to provide here because each solution would have different type on left-hand side but the same on the right, so if there is a single error here it would have to be general enough to cover all of the cases.

[Rajveer100]

Here's my thought process:

Seeing the overload choices, is there a way to retrieve or check if the specific solution points at the argument parameter, like in the example one of the choices:

locator@0x12d1c6ca0 [[email protected]:5:14] with _Concurrency.(file).Instant extension.- as -: (SuspendingClock.Instant, SuspendingClock.Instant) -> Duration

It points at 5:14 while rest are at either the return/decl or other parts of the function expression. A little unsure about the usage of the API.

Also, just to be sure, you meant to add a method as such:

bool AllowArgumentMismatch::diagnoseForAmbiguity(CommonFixesArray commonFixes) const {
  const auto *locator = getLocator();
  for (auto &entry: commonFixes) {
    auto &solution = entry.first;
    
  }
}

And then here we go over the choices and try to diagnose if I understand correctly. I was trying to look at the other cases where obviously each have a different approach depending on the type of ambiguity for example contextual/relabel/invalid member etc.

I believe we could throw the argument mismatch diagnostic right which should cover the cases? I could be missing some though.

[xedin]

I think it might be worth for you to take a look at diagnoseAmbiguityWithFixes, this is the method that analyzes all of the fixes across solutions and tries to group them and invoke appropriate methods. There is currently no diagnoseForAmbiguity override on AllowArgumentMismatch because it's non-trivial to implement - the fixes could point to different argument/parameter pairs or the same pair but either argument or parameter might have different type (i.e. your example with foo(-) all solutions point to the same pair and parameter types are all the same but argument types are all different).

[Rajveer100]

Yeah, I understand the flow now, each of the types return many different types due to the overload, like Int, Int128, Duration. Maybe we could first go over the overload choices and then filter for OverloadedDeclRef, then we check if this is some sort of built-in like +, -, etc or if it's a custom one like **.

I think we can just throw a generalised error like:

error: cannot convert overload type of type '<string_name>' to expected argument type 'Int'

What do you think/suggest?

[xedin]

The thing is that's it's not only about applied operators or functions in general:

func test() -> Int {}
func test() -> String {}

func other(_: Int32) {}

other(test())

But we can diagnose the other way around pretty well:

func test(_: Int) {}
func test(_: String) {}

func compute() -> Double { 0 }

test(compute())

Maybe other(test()) should be diagnosed as ambiguous use of 'test()' with a note for each mismatch...

[Rajveer100]

I have made changes, let me know if these diagnostics work well or if you prefer a different note:

debug-79999.swift:3:9: error: ambiguous use of '-'
 1 | func foo(_ a: Int) -> [Int] { [] } // 'a' can be any type: String, Data, custom type, etc.
 2 | 
 3 | let _ = foo(-) // Type of expression is ambiguous without a type annotation (expected diagnostic)
   |         |- error: ambiguous use of '-'
   |         |- note: type mismatch between '(Duration, Duration) -> Duration' and 'Int'
   |         |- note: type mismatch between '(Int128, Int128) -> Int128' and 'Int'
   |         |- note: type mismatch between '(UInt128, UInt128) -> UInt128' and 'Int'
   |         |- note: type mismatch between '(Float16) -> Float16' and 'Int'
   |         |- note: type mismatch between '(Float16, Float16) -> Float16' and 'Int'
   |         |- note: type mismatch between '(Float) -> Float' and 'Int'
   |         |- note: type mismatch between '(Float, Float) -> Float' and 'Int'
   |         |- note: type mismatch between '(Double) -> Double' and 'Int'
   |         |- note: type mismatch between '(Double, Double) -> Double' and 'Int'
   |         |- note: type mismatch between '(UInt8, UInt8) -> UInt8' and 'Int'
   |         |- note: type mismatch between '(Int8, Int8) -> Int8' and 'Int'
   |         |- note: type mismatch between '(UInt16, UInt16) -> UInt16' and 'Int'
   |         |- note: type mismatch between '(Int16, Int16) -> Int16' and 'Int'
   |         |- note: type mismatch between '(UInt32, UInt32) -> UInt32' and 'Int'
   |         |- note: type mismatch between '(Int32, Int32) -> Int32' and 'Int'
   |         |- note: type mismatch between '(UInt64, UInt64) -> UInt64' and 'Int'
   |         |- note: type mismatch between '(Int64, Int64) -> Int64' and 'Int'
   |         |- note: type mismatch between '(UInt, UInt) -> UInt' and 'Int'
   |         |- note: type mismatch between '(Int, Int) -> Int' and 'Int'
   |         |- note: type mismatch between '(ContinuousClock.Instant, Duration) -> ContinuousClock.Instant' and 'Int'
   |         |- note: type mismatch between '(ContinuousClock.Instant, ContinuousClock.Instant) -> Duration' and 'Int'
   |         |- note: type mismatch between '(SuspendingClock.Instant, Duration) -> SuspendingClock.Instant' and 'Int'
   |         `- note: type mismatch between '(SuspendingClock.Instant, SuspendingClock.Instant) -> Duration' and 'Int'
 4 | 
 5 | for _ in foo(-) { // Failed to produce diagnostic for expression; please submit a bug report (https://swift.org/contributing/#reporting-bugs)

debug-79999.swift:5:10: error: ambiguous use of '-'
 3 | let _ = foo(-) // Type of expression is ambiguous without a type annotation (expected diagnostic)
 4 | 
 5 | for _ in foo(-) { // Failed to produce diagnostic for expression; please submit a bug report (https://swift.org/contributing/#reporting-bugs)
   |          |- error: ambiguous use of '-'
   |          |- note: type mismatch between '(Duration, Duration) -> Duration' and 'Int'
   |          |- note: type mismatch between '(Int128, Int128) -> Int128' and 'Int'
   |          |- note: type mismatch between '(UInt128, UInt128) -> UInt128' and 'Int'
   |          |- note: type mismatch between '(Float16) -> Float16' and 'Int'
   |          |- note: type mismatch between '(Float16, Float16) -> Float16' and 'Int'
   |          |- note: type mismatch between '(Float) -> Float' and 'Int'
   |          |- note: type mismatch between '(Float, Float) -> Float' and 'Int'
   |          |- note: type mismatch between '(Double) -> Double' and 'Int'
   |          |- note: type mismatch between '(Double, Double) -> Double' and 'Int'
   |          |- note: type mismatch between '(UInt8, UInt8) -> UInt8' and 'Int'
   |          |- note: type mismatch between '(Int8, Int8) -> Int8' and 'Int'
   |          |- note: type mismatch between '(UInt16, UInt16) -> UInt16' and 'Int'
   |          |- note: type mismatch between '(Int16, Int16) -> Int16' and 'Int'
   |          |- note: type mismatch between '(UInt32, UInt32) -> UInt32' and 'Int'
   |          |- note: type mismatch between '(Int32, Int32) -> Int32' and 'Int'
   |          |- note: type mismatch between '(UInt64, UInt64) -> UInt64' and 'Int'
   |          |- note: type mismatch between '(Int64, Int64) -> Int64' and 'Int'
   |          |- note: type mismatch between '(UInt, UInt) -> UInt' and 'Int'
   |          |- note: type mismatch between '(Int, Int) -> Int' and 'Int'
   |          |- note: type mismatch between '(ContinuousClock.Instant, Duration) -> ContinuousClock.Instant' and 'Int'
   |          |- note: type mismatch between '(ContinuousClock.Instant, ContinuousClock.Instant) -> Duration' and 'Int'
   |          |- note: type mismatch between '(SuspendingClock.Instant, Duration) -> SuspendingClock.Instant' and 'Int'
   |          `- note: type mismatch between '(SuspendingClock.Instant, SuspendingClock.Instant) -> Duration' and 'Int'
 6 | } 
 7 | 

debug-79999.swift:13:1: error: ambiguous use of 'test'
11 | func other(_: Int32) {}
12 | 
13 | other(test())
   | |- error: ambiguous use of 'test'
   | |- note: type mismatch between 'Int' and 'Int32'
   | `- note: type mismatch between 'String' and 'Int32'
14 | 
15 | //func test(_: Int) {}

[xedin]

Looks much better! I think the error and note could be tweaked a little to talk about "no candidates" matched in case of partial/unapplied reference and "result type" in case when the there is a "call" passed an an argument like in other(test()). Notes should talk about candidates as well.

[Rajveer100]

What could be a nice way to differentiate between the two examples since both are overloaded expr and callers so the flag hits both?

[Rajveer100]

To be exact, I have pushed the changes.

[Rajveer100]

@xedin
Ping