C#: Extract and use ambiguous type information for call target resolution

[tamasvajk]

This PR

• extracts
  • candidate type symbols reported by Roslyn when facing ambiguous types
  • invoked method names
• modifies MethodCall::getTarget to look up candidate call targets when the qualifier of the call has an ambiguous type.

[tamasvajk]

@hvitved, @michaelnebel Could you have a look at this PR? It's not exactly ready for review. DCA is showing significant slowdown. But I'd like to get some early feedback on the approach, which significantly changes how we come up with call targets.

[michaelnebel]

Why are all dynamic calls excluded?
A far as I understand, a call can be considered dynamic, if just one of the arguments are.

[tamasvajk]

That's a good point. We could include dynamic_member_name in MethodCall::getACandidateTarget, where we use invocation_member_name. But there's no need to store dynamic member names duplicately in the DB.

[tamasvajk]

I've done this in 82c675a.

[michaelnebel]

Why is this needed?

[tamasvajk]

I think getArgumentForParameter was not handling correctly params before. The corresponding tests also report more argument-parameter pairs now. See here.

In case of params, we were already checking the types with isValidExplicitParamsType, which I think checks whether an array type is convertible to another array type, such as string[] to object[]. For the expanded form, we'd need to check the types agains the element type of the array.

void M1(string s, params int[] a) {}
void M2(string s, params double[] a) {}

M1("", 1, 2, 3);         // Viable
M1("", 1.1, 2.2, 3.3);   // This is not a viable call to M due to `1.1` having `double` as a type, which is not convertible to the element type `int` of array type `int[]`.

M2("", 1, 2, 3);         // Viable, `int` is implicitly convertible to `double`. 
M2("", 1.1, 2.2, 3.3);   // Viable

[tamasvajk]

@hvitved raised the point below that isImplicitlyConvertibleTo might not work for generics. This is something that we should check and fix here. final override Expr getArgumentForParameter should work for traced DBs.

[michaelnebel]

Including base types here somehow seems related to dispatching. Could this be a potential performance killer? Would it make sense to initially only look at type alternatives without taking base types into account?

[tamasvajk]

The test cases show that including the base classes is needed. At the same time, it can easily be a performance killer.

[michaelnebel]

What happens, if we only look at "one" level of alternatives? Does that cover most cases?

[tamasvajk]

I've done this in a4eb47f

[michaelnebel]

I think this looks really interesting.
Have you found the source of the performance issues?

[tamasvajk]

@hvitved Have you got a chance to do an early review of this PR?

[hvitved]

Some initial comments.

[hvitved]

Modeling it in this way means that we cannot distinguish when a type is used unambiguously and when it is used ambiguously, that may be too simplistic.

[tamasvajk]

Wouldn't not exists(this.getAnAmbiguousAlternativeType()) mean that the type is used unambiguously?

[hvitved]

I may have misunderstood how it works, but consider this example:

namespace N1 {
    class C {}
}

namespace N2 {
    class C {}
}

namespace N3 {
    class C {}
}

namespace N4 {
    using N1;
    using N2;
    
    class C3 {
        C Field; // either `N1.C` or `N2.C`, not `N3.C`
    }
}

namespace N5 {
    using N1;
    using N3;
    
    class C3 {
        C Field; // either `N1.C` or `N3.C`, not `N2.C`
    }
}

AFAICT, N1.C, N2.C, and N3.C will be considered equal modulo getAnAmbiguousAlternativeType, but that is an over approximation

[tamasvajk]

No, your understanding is correct, we over approximate. What would be your suggestion? (Let's continue this discussion on Slack)

[hvitved]

I don't think this works if either the parameter type or the argument type contains type parameters.

[tamasvajk]

We can definitely improve this further. At the same time, I don't think it's critical. It means that there are cases when we don't find a candidate call target, doesn't it? The opposite can also happen: we may find too many candidate call targets. These are expected as we haven't implemented the entire resolution logic.

[tamasvajk]

DCA performance looks good. Removed/added alert count doesn't seem as good as I expected. We do find 9 new cs/hardcoded-credentials in aspnetboilerplate/aspnetboilerplate which was prompting this change in the first place. At the same time, we only find 3 additional alerts and we lose 51 in the test suite. I'll need to check if the removed 51 were false positives or not...