[SR-12689] Diagnose arg mismatch with exact same fixes across multiple solutions

[LucianoPAlmeida]

In this case, there are multiple solutions that record the same exact argument mismatch fix.
So this makes sure if all the fixes are the exact same arg and param types across solutions, we just diagnose.

cc @xedin @hborla :)

Resolves SR-12689.

[hborla]

Looks great, thank you!

[xedin]

@LucianoPAlmeida Why special case this instead of letting it be diagnosed as no exact matches ...?

[LucianoPAlmeida]

@LucianoPAlmeida Why special case this instead of letting it be diagnosed as no exact matches ...?

humm, in this case, there is no ambiguous overload, there is only one overload on the solutionDiff that had the exact same fixes recorded for all the solutions.
For what I could see there is some logic that removes some overload choices from the diff here
https://github.com/apple/swift/blob/727e2220412ffa576738007404f46925d1c3f635/lib/Sema/CSRanking.cpp#L1413, but not exactly sure if that is why it ends up with a single overload in this case.

[xedin]

@LucianoPAlmeida UnsafeRawPointer.init is ambiguity here. There are 4 solutions, 2 with one argument mismatch and others with 2.

[xedin]

I think the problem here is that we are ranking overloads although in presence of fixes we shouldn't be doing that because it's not apples to apples comparison. I want to adjust that and only rank on score instead.

[LucianoPAlmeida]

I think the problem here is that we are ranking overloads although in presence of fixes we shouldn't be doing that because it's not apples to apples comparison. I want to adjust that and only rank on score instead.

Humm, so the strategy would be not ranking if any overload solution has at least one fix?
Anyways I'll get back to this by the end of this week.

Thanks @xedin @hborla :)

[xedin]

Not ranking based on declarations yes, let it be ambiguous different overloads have the same "fix" weight.

[LucianoPAlmeida]

Right,
The issue is actually not with the rankings, the situation is very specific and can be reproduced in a simpler example:

func foo(_ u: Int) -> String { "" }
func foo(_ u: String) -> String { "" }

func bar(_ u: Int) {}

bar(foo(1 as Double))

These situations produce exactly the same situation where for each solution the solver will produce two sets of fixes where each solution has two fixes (one argument mismatches for foo disjunction choice being attempted) and (one argument mismatch for the return of foo applied to the argument in bar) which is not being actually handled on diagnoseAmbiguityWithFixes because the logic can only handle cases where all the aggregated fixes point to the overload diff locator anchor, which can't happen in those situations because there are two sets of fixes anchored on different arguments for different function applications.

So my question is if the better approach would be to:

1. As implemented here right now, use the set of arg fixes the diagnose the mismatch of String -> Int in bar each solution produces exactly the same fixes?

2. Tweak diagnoseAmbiguityWithFixes to handle situations like this? And diagnose the no match candidates ambiguity on foo? I trying this approach right now :)

Also this also happens with other mismatches e.g. coercion, I didn't investigate yet but probably a similar situation...

foo(1 as Double) as Int // type of expression is ambiguous without more context

So let me know what you think @xedin @hborla :)

Edit: Actually the first option is not viable because in cases where the return types are different the argument mismatches wouldn't be the same...

func foo(_ u: Int) -> String { "" }
func foo(_ u: String) -> Double { 0 }

func bar(_ u: Int) {}

bar(foo(1 as Double))

[LucianoPAlmeida]

Just changed to WIP because although this fixes the problem, there are still 2 diagnostic regressions I still don't know how to solve them :(

[LucianoPAlmeida]

Right, ready for review :)
cc @xedin @hborla

[xedin]

@LucianoPAlmeida It seems like #2 is what we should be working towards with a tweak that it should diagnose all of the fixes in common and not just one so in that example it should diagnose ambiguity + result mismatch with what is expected by bar...

[LucianoPAlmeida]

@LucianoPAlmeida It seems like #2 is what we should be working towards with a tweak that it should diagnose all of the fixes in common and not just one so in that example, it should diagnose ambiguity + result mismatch with what is expected by bar...

Hey @xedin :)
Yeah, the way I implemented it just diagnosed the ambiguity with a tweak on the notes.
I'm not really sure about how to go about the mismatches to bar tho, because for the case like

func foo(_ u: Int) -> String { "" }
func foo(_ u: String) -> Double { 0 }

func bar(_ u: Int) {}

bar(foo(1 as Double))

There is 2 solutions each with 2 fixes.
solution 0: fix for foo arg #0 Double -> Int and fix for bar arg #0 String -> Int
solution 1: fix for foo arg #0 Double -> Stringand fix for bar arg #0 Double -> Int

So for the set of aggregated fixes in foo we diagnose ambiguity, but what to do about the fixes to bar because the arg type depends on the result of the foo which is not actually the same ... so what should we diagnose... another ambiguity?

[xedin]

That's exactly what I'm currently pondering on.

I think in this case we should just diagnose two invalid conversions... Current plan is to (once again) change the way we do stuff in diagnoseAmbiguityWithFixes because it kind of starts becoming the same mess of special cases as we had before...

It seems to me (and @hborla agrees) that this whole problem boils down to us needing to find an intersection of solutions based on fixes and their locations.

For each distinct fix/location pair we need to figure out whether it's included in all solutions, if all of the solutions match exactly on this criteria I think it's reasonable to diagnose as if there was just one solution because this means that the problems are universal e.g. contextual type mismatch + r-value type on left-hand side of += operator. Otherwise fallback to ambiguity diagnostic for a "common" anchor.

I think this covers case you mentioned without requiring any special handling because all of the fixes are going to be shared across solutions.

[xedin]

Hm, it actually might be kind of weird to diagnose conversions here, I think we should add a criteria that checks whether each distinct fix is related directly to multiple distinct overload choices and diagnose that appropriately...

[LucianoPAlmeida]

Thank you @xedin I'll work on that :)

One question ...

For each distinct fix/location pair we need to figure out whether it's included in all solutions, if all of the solutions match exactly on this criteria

The actual aggregate fix logic takes into account the fix kind, this should be the case here too?

[xedin]

I think so, that's what I mean why fix + location there :)

[LucianoPAlmeida]

@xedin @hborla I think I've implemented in the way I understand from what you described, but let me know what you think :)

[xedin]

@LucianoPAlmeida overall this is exactly what I imagined this supposed to work, thank you! I just wanted to check why stop after after diagnosing non-overload related ambiguities here - https://github.com/apple/swift/pull/31359/files#diff-27038c7aac8e7016c5ab7bceb4a03651R3015 why not diagnose everything?

[xedin]

On the side note I think we can simplify diagnoseAmbiguityWithFixes method if we extract everything related to diagnosing a single aggregate into a separate static function and instead of building this giant dictionary we build a set of district fixes first (fix kind + location) and based on that aggregate as a second pass over solutions.

[LucianoPAlmeida]

I just wanted to check why stop after diagnosing non-overload related ambiguities here - https://github.com/apple/swift/pull/31359/files#diff-27038c7aac8e7016c5ab7bceb4a03651R3015 why not diagnose everything?

Hey @xedin :)
That's is because of situations like this: https://github.com/apple/swift/blob/d1f798bedb61d4c137573360e6e623670efedc2a/test/type/protocol_composition.swift#L176
Here each solution produces a DefineMemberBasedOnUse fix, that is diagnosed as unresolved_member_no_inference that is diagnosed properly by DefineMemberBasedOnUse::diagnoseForAmbiguity so if we diagnose that I felt that there's no need to point the ambiguous of & ... since we already diagnosed an "ambiguity" in some way.

[xedin]

Are there any other examples of that? What I'm worried about is that we are not going to show all the relevant errors and developers would end up fixing one error just to get to the other similar to climbing a ladder where in contrast if they have seen all of the errors it might be more obvious what to do...

[LucianoPAlmeida]

No, I think there were only two "regressions" initially this and another one that I think was related to other changes I made halfway progress

Edit: The other case is in fact related
https://github.com/apple/swift/blob/9c8be746b9d5b25511f2cf6df9427cdfa37d93b2/test/Misc/misc_diagnostics.swift#L57

We endup producing an extra binary operator '<' cannot be applied to operands of type 'MyArray<_>.Type' and 'Int.Type' if we don't return after diagnosing the common fixes.

But these are the only cases I've seen :)

[LucianoPAlmeida]

On the side note I think we can simplify diagnoseAmbiguityWithFixes method if we extract everything related to diagnosing a single aggregate into a separate static function and instead of building this giant dictionary we build a set of district fixes first (fix kind + location) and based on that aggregate as a second pass over solutions.

I'll address this, just finishing to adjust to diagnose another case that I found that is not handled ... :(

[LucianoPAlmeida]

On the side note I think we can simplify diagnoseAmbiguityWithFixes method if we extract everything related to diagnosing a single aggregate into a separate static function and instead of building this giant dictionary we build a set of district fixes first (fix kind + location) and based on that aggregate as a second pass over solutions.

@xedin One thing is that if I understand it correctly this will make us iterate over all the fixes and solutions for each (fix + location) entry?

Also, I think that diagnose single aggregate will require some extra logic for cases where for
overload based diagnostics in some cases there are (same location + different kind) between aggregates, so we have to keep track if we already diagnose the overload, but that may not be a big problem ...

[xedin]

Yes, that's right. First build a set of all unique kind/location parts, then use each to find whether its include into all of the solutions. My intuition is that fixes which are local to some solutions are most likely a result of ambiguities that manifest differently based on overload choice.

[xedin]

I'm currently working on moving diagnoseAmbiguityWithFixes to be attempted before findBestSolution in salvage a lot of my changes are actually very similar to what you are trying to do. Do you mind waiting a bit so I can get my PR in shape to see how my changes would affect yours? This is going to be one hell of a merge conflict if we try to do this in parallel :)

[LucianoPAlmeida]

Do you mind waiting a bit so I can get my PR in shape to see how my changes would affect yours? This is going to be one hell of a merge conflict if we try to do this in parallel :)

No problem! Let me know when you have your changes set :)

[LucianoPAlmeida]

Ahh also, @xedin when possible can you take a look at ff121353c19b3d751e8ea23cf0c690d0fa9939a6? that's another case and this was the best I could come up with and I think is fine ... but maybe you have some other option suggestion :)

[xedin]

It seems like we should differentiate when reference is applied and when it's not because result diagnostic is weird for situations when reference is passed as an argument...

[LucianoPAlmeida]

It seems like we should differentiate when reference is applied and when it's not because result diagnostic is weird for situations when reference is passed as an argument...

Yeah, you right that makes total sense :)

[LucianoPAlmeida]

@xedin, I just adjust this last case
Didn't change anything on diagnoseAmbiguityWithFixes, so it is fine, will wait until your changes :)

[xedin]

Thank you!

[LucianoPAlmeida]

I was looking through some JIRA SRs and found this SR-12705 which I think is going to benefit from the improvements being done on diagnoseAmbiguityWithFixes.
Just adding here to track, so we remember to add a regression test for it too if that is indeed the case

func bindToInt(
  _ base: UnsafeMutableRawPointer,
  at offset: Int
) -> UnsafeMutablePointer<Int> {
  let result = (base + offset).bindMemory(to: Int.self, count: 1) // cannot convert value of type 'UnsafeMutableRawPointer' to expected argument type 'Int'
 // value of type 'Int' has no member 'bindMemory'
  return result
}

[LucianoPAlmeida]

Superseded by #31713