[SR-13240] non-specific diagnostic when calling a function with two wrong-type arguments

[marcrasi]

Previous ID	SR-13240
Radar	rdar://problem/65842034
Original Reporter	@marcrasi
Type	Bug
Status	Closed
Resolution	Done

Environment

swift-DEVELOPMENT-SNAPSHOT-2020-07-14-a-ubuntu20.04

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Bug, 5.3Regression, DiagnosticsQoI, TypeChecker
Assignee	@LucianoPAlmeida
Priority	Medium

md5: d23f6c9602953bf690f36dffaa470bb3

Issue Description:

func twoargs(_ x: String, _ y: String) {}

func test() {
  let x = 1

  // Actual diagnostic:
  //   error: type of expression is ambiguous without more context
  //   twoargs(x, x)
  //   ^~~~~~~~~~~~~
  //
  // Desired diagnostic:
  //   error: cannot convert value of type 'Int' to expected argument type 'String'
  //   twoargs(x, x)
  //           ^
  //
  //   error: cannot convert value of type 'Int' to expected argument type 'String'
  //   twoargs(x, x)
  //              ^
  twoargs(x, x)
}

[theblixguy]

cc @xedin @hborla looks like a regression since 5.2. It seems like we have a failed constraint (argument conversion from Int to String) in the system and so salvage() bails out when it sees it and we don't end up actually solving the system in salvage mode.

[LucianoPAlmeida]

This problem seems to be related to this early return on CSSimplify https://github.com/apple/swift/blob/71f6797c8fc2c27e40f452ed77385ed4226dcd7a/lib/Sema/CSSimplify.cpp#L3657 where we have a single overload choice and two arg to param constraint failures where the second one is not "diagnose" let the solver is unable to form a solution for that, therefore leading to an ambiguous diagnostic ... @xedin, initially I thought that only consider this "fixed" and just return as diagnosed would be able to handle this, but the reality is that this being as it is important for a lot of diagnostics

[LucianoPAlmeida]

This also leads to ambiguous diagnostics in situations like

infix operator ---
 
func --- (_ lhs: String, _ rhs: String) -> Bool {
  return true
}
let x = 1
 
x --- x // type of expression is ambiguous without more context

It is basically the same case

[theblixguy]

IIRC you get a diagnostic if you have two different variables (x and y) so it seems like this only happens when you have >1 failure associated with the same argument?

[LucianoPAlmeida]

Do you mean like

func twoargs(_ x: String, _ y: String) {}

func test() {
  let x = 1
  let y = 1

  twoargs(x, y) // type of expression is ambiguous without more context
}

infix operator ---

func --- (_ lhs: String, _ rhs: String) -> Bool {
  return true
}
let x = 1
let y = 1

x --- y // type of expression is ambiguous without more context

In this case, we still get ambiguous for the same reason, the problem is when having more than one argument failure (for any argument) e.g. in those cases args #0 and #1 fail on the same call anchor. I really don't know what would be an approach to solve this, as the comment in line 3644 states ("we can consider overload unrelated"), but this leads to problems like those examples here if all overloads are ignored ... and change this would impact a lot of other diagnostics.
Maybe we could change this to return this as diagnosed, and implement a custom logic on `ArgumentFix::diagnoseAmbiguity`... not really sure

[theblixguy]

Oh sorry, I think I got confused when I was testing out a few other cases. I thought I remembered seeing a diagnostic when using different variables instead of the same one.

[xedin]

I think it might work to start recording all of the argument failures and increase a score significantly after each one to indicate that the more argument are mismatching the least desirable this overload choice is...

[LucianoPAlmeida]

@xedin I've seen that there is already a score increasing for argument fix here https://github.com/apple/swift/blob/506aa1dfa77b5b0af0842c1d90197ca9450da9a3/lib/Sema/CSSimplify.cpp#L9812 so in that case this should be increased even more or just remove this check and record them all should be sufficient to handle this?

[xedin]

@LucianoPAlmeida I'm taking about this bit https://github.com/apple/swift/blob/506aa1dfa77b5b0af0842c1d90197ca9450da9a3/lib/Sema/CSSimplify.cpp#L3643-L3657 - `repairFaiures` rejects to fix more than one argument (if it finds multiple failures in a single component), I think we need to actually switch that to record a fix and increase a score even higher, so this logic should go to https://github.com/apple/swift/blob/506aa1dfa77b5b0af0842c1d90197ca9450da9a3/lib/Sema/CSSimplify.cpp#L9812 instead.

[LucianoPAlmeida]

Right @xedin let's try that!

[LucianoPAlmeida]

Fixed on master @marcrasi can you please test on the next available snapshot? thanks

[marcrasi]

I verified it. Thanks!!