[Sema] Penalize type mismatches that lead to @dynamicMemberLookup types

[gregomni]

Resolves #74700

The problem with this issue is that the overload with a Binding parameter seems better because Binding has @dynamicMemberLookup, so the argument fix seems lower impact than the missing member error.

The change here is to use the same idea from param/argument fixes where if you get a second param fix on the same call, it is very unlikely to be the correct overload. Here, do the same thing for defaulting a generic parameter to Any. If we're defaulting a generic parameter in a call that already has an argument fix, it's not likely to be the correct overload.

The increase by 6 is the lowest amount that will give the correct diagnosis in the original issue code IF pull request #74692 gets merged (which raises the cost of inventing a member). Otherwise an increase by 4 would be enough.

[gregomni]

@swift-ci Please smoke test.

[xedin]

It would be good to figure out how to do this without having look for fixes because
generally speaking a generic parameter could be involved in multiple arguments or result positions, it might be not inferrable because i.e. there was a member or some other remote problem that propagated placeholders to arguments/result without recording a fix (just like in example with the issue). I see a couple of ways here - either be more aggressive with argument mismatch fixes i.e. if there an argument mismatch and there are some type variables involved in the type - increase the impact of the mismatch fix or increase default impact of the DefaultGenericArgument fix.

[gregomni]

Increasing default impact of DefaultGenericArgument fix in order to make this particular issue give a good diagnosis would require a big number, which is likely to affect a lot.

Increasing impact of an argument mismatch with a type containing type variables seems better, and is an approach I looked at doing here - I'll go back and try that.

[gregomni]

Made it so the change is to increase impact of an argument mismatch with a type containing type variables. The bridging and diagnostics test changes aren't really improvements or declines in the diagnosis, just different decisions. The other changes are improvements.

[gregomni]

@swift-ci Please smoke test.

[gregomni]

@swift-ci Please smoke test.

[xedin]

Hm, I think we need to be more precise here if we wanted to do it this way because some of the type variables might not be opened generic parameters and others might be deducible from other positions, my worry is this is going to impact all argument-to-parameter conversions that involve generic types.

I think we'd need to get creative here. To properly diagnose situations like this we'd need to discern when generic parameter cannot be inferred due to lack of information and when it's due to some mismatch failure in a constraint that involves it indirectly. The latter should have higher impact. I was suggesting placeholders before but we can also store generic parameters involved in mismatches in the constraint system or we could add a new TVO attribute and add it, this could be applicable to argument-to-parameter positions at first and get expanded from there.

[gregomni]

Ok - how's this?

Made more precise by checking that if type2->hasTypeVariable() that at least one of those type variables is for a generic parameter and that generic parameter is anchored in the same call expression that we are currently looking at.

[xedin]

Such parameter could still be inferred from a different argument or a result type right?

[gregomni]

I don't think so. I mean, it could be inferred elsewhere, yes, but if it was inferred elsewhere, then the parameter type in this spot would no longer contain the type variable, it would be whatever the resolved type was.

[xedin]

What I meant was - you are making a decision here to increase a score based on a type variable that could still be resolved from other place, so now diagnostics are dependent on inference order…

[xedin]

So the following example:

@dynamicMemberLookup 
struct Binding {
  subscript(dynamicMember member: String) -> String { "" }
}

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

test(42)

Would be diagnosed as an error between Int argument and String parameter instead of ambiguity because Binding is a @dynamicMemberLookup, that doesn't seem right to me...

[gregomni]

Yes, that's true. The ambiguity vs wrong type of the argument diagnoses are at least both at the right spot, and so causes much less confusion to the user. The issue that motivates this PR, where the wrong overload choice cascades into an entirely off-base diagnosis at Text(user.nam) seems much worse to me.

(Not sure that it matters, but in practice, where Binding has a generic parameter, we'd already choose the String argument overload because of the extra DefaultGenericArgument fix.)

Edit to add: In fact, I think that's a reasonable parallel. Where the parameter involves a type that is generic, we'll end up diagnosing based on the non-generic overload choice. This is very similar. @dynamicMemberLookup is another sort of "genericness".

[xedin]

I don't think I agree, diagnostic in my example should produce a note per choice because they are equally incorrect, we shouldn't regress this. Also imagine a situation when overload with a Binding would actually be a better choice i.e. the argument is a superclass instead of subclass:

class Binding {
}

@dynamicMemberLookup
class Sub : Binding {
  subscript(dynamicMember member: String) -> String { "" }
}

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

func other(v: Binding) {
  test(v)
}

It would be incorrect to only produce a diagnostic about how Binding is not convertible to String.

[gregomni]

Another try: This is now essentially your suggestion to "store generic parameters involved in mismatches in the constraint system". If we then at a later point try to lookup a member of an instance type which has a mismatched opened generic param, we disallow dynamicMemberLookup. That way, in the motivating issue, both branches of the overload choice disjunction get the missing member error, and diagnoses gets decided based on the other fixes.

Added some more testing to test/Constraints/issue-74700.swift to verify that we still get ambiguity where desired, and that multiple openings of the same generic param decl (some correct, some mismatched) all work as expected.

[xedin]

I don't think these changes in general behave as you'd wish because the base type is simplified before performMemberLookup is called so if the generic parameter is deducible or i.e. some logic in repairFailures is going to match it while deciding on an exact problem behind the mismatch the information about the original type variable is going to be lost...

I tried running your reduction without code changes with main compiler and it looks like the solver cannot even find a valid solution for the first ForEach.init overload, it runs into issues with a contextual type. Looking at the output makes me wonder if we underestimate the impact of placeholder propagation to the dependent member types at the moment. Perhaps every attempt to resolve a member on a placeholder base should result in a score increase (caching the result to make sure that score is increased only once per member type), that might be the way to make sure that choosing the second overload of ForEach.init incurs a large impact because all of the same type requirements there are effectively comparing placeholders.

[gregomni]

@swift-ci Please smoke test.

[gregomni]

@swift-ci Please smoke test

[gregomni]

@swift-ci Please smoke test.

[gregomni]

@swift-ci Please smoke test.