Skip to content

[Sema][SR-15053] Ignore ambiguity from conformance in some situations #39220

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged

Conversation

LucianoPAlmeida
Copy link
Contributor

The problem here for this additional extra diagnostic

func fooAAA<T : Numeric>(_ a: T, _ b: T) -> T {
    // this results in two compile errors, first is correct, second should not be:
    // Binary operator '/' cannot be applied to operands of type 'T' and 'Int'    <=== this error is correct
    // Binary operator '+' cannot be applied to two 'T' operands    <=== !!! but this should not be an error!!!
    (a + b) / 2
}

Is that for some overloads attempted for / operator given the type bound to T some additional conformance fixes are recorded on + arguments which then latter arrive in diagnose overload ambiguity and end up producing the extra diagnostic. The initial idea was to try not record the conformance fixes in case operator locator is already anchored in a argument mismatch fix, but this didn't work, sometimes at the point conformance constraint is being simplified and fix is being recorded we may or may not have argument mismatch recorded being that because there is no mismatch for the current overload being attempted or because given the ordering of simplification of constraints arg mismatch fix is yet to be recorded.

So this PR I took the approach of detecting this at the ambiguity diagnostic. But this PR is more to get your thoughts and suggestions on maybe better approaches in this case :)

Resolves SR-15053.

@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-15053-overload-diags branch 2 times, most recently from d8e0dd4 to e205fb4 Compare September 10, 2021 02:42
@xedin
Copy link
Contributor

xedin commented Sep 11, 2021

This is on my radar, just need to think about what to suggest here :)

@LucianoPAlmeida
Copy link
Contributor Author

This is on my radar, just need to think about what to suggest here :)

Sure, thank you!

I was also thinking in doing something not related overloads, but to generic parameters, because it may be possible to detect if a generic parameter is already involved in fixes before recording conformance mismatch, but I would have to get this a try and also then we may hit the same problem as we previously discussed here #34451 (comment)

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might want to consider a different approach to diagnosing failures in chained expressions. Currently it's all about arguments, but I think in cases like (a + b) / 2 it should actually be about viability of subsequent links and protocol conformance fixes.

If you look at the debug output you'd see that e.g. for /(Float, Float) and +<T: AdditiveArithmetic>(_: Self, Self) -> Self there is going to be a record of an argument failure even when result type of + is inferred to be T and contextual mismatch with function result. That local result would just be repeated for each of the overloads of /.

I think we need to figure out whether all the conformance fixes even make sense for operators and if they do what impact should they have e.g. when / is <Self: SIMD, Self.Scalar : FixedWidthInteger>(Self, Self.Scalar) -> Self solver would record a conformance failure (originating from contextual type T and make Self.Scalar a placeholder and in combination with +<Self: AdditiveArithmetic>(Self, Self) -> Self solution ends up with a score of 3 which frankly seems too low.

There is a good solution in there already - argument conversion and contextual mismatch with result but it's just buried under a mass of all these conformance failures with a low impact...

@LucianoPAlmeida
Copy link
Contributor Author

Thank you for the suggestions @xedin! I'll take a deeper look and experiment a bit and get back with something as soon as possible

@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-15053-overload-diags branch 3 times, most recently from 83eea7d to fdc3fbb Compare October 10, 2021 22:39
@LucianoPAlmeida
Copy link
Contributor Author

@xedin Let me know if that is the direction you had in mind :)
Have been experimenting a bit in how much the score impact should be increased and this seems to give a good balance, more than that I saw some regressions on some other diagnostics... so maybe I have to do a better job understanding those cases.

@LucianoPAlmeida
Copy link
Contributor Author

@swift-ci Please smoke test MacOS Platform

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that this is what we want, left a minor comment inline.

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@LucianoPAlmeida
Copy link
Contributor Author

@swift-ci Please smoke test

@LucianoPAlmeida
Copy link
Contributor Author

@swift-ci Please test Windows Platform

1 similar comment
@LucianoPAlmeida
Copy link
Contributor Author

@swift-ci Please test Windows Platform

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants