[SR-13503] [Sema] Special InsertExplicitCall for nominal callable types #34006

LucianoPAlmeida · 2020-09-20T20:32:45Z

Detect if a type mismatch we the source type is a callable nominal and has a callAsFunction that produces the destination type, so we can suggest making it an explicit call to it.

Resolves SR-13503.

include/swift/AST/DiagnosticsSema.def

xedin

The approach makes sense to me, but I think we need to use existing functionality to form implicit call and match result. I have left a couple of comments about this inline.

lib/Sema/CSSimplify.cpp

include/swift/AST/TypeCheckRequests.h

LucianoPAlmeida · 2020-10-13T09:57:05Z

@swift-ci Please smoke test

LucianoPAlmeida · 2020-10-13T11:44:04Z

@swift-ci Please smoke test

LucianoPAlmeida · 2020-10-14T10:13:43Z

@swift-ci Please smoke test

include/swift/Sema/ConstraintLocator.h

include/swift/Sema/ConstraintLocatorPathElts.def

include/swift/Sema/CSFix.h

xedin · 2020-10-16T20:38:57Z

lib/Sema/CSSimplify.cpp

Let's rephrase this a bit:

If this is a call formed by a fix to attempt explicit call of a callable type to see if that would match expected contextual type, let's fail if some of the expected arguments are missing.

But I also don't fully understand why do we want to do that? I think the idea where would be to suggest adding a call and missing arguments, no? I guess we'd just need to adjust a fix for synthesized arguments to handle implicit calls to support that...

I think the idea where would be to suggest adding a call and missing arguments, no?

Not really sure, that would mean a fix for a declaration like func callAsFunction(a: Int) -> Int { 0 } would be (a: <#editor placeholder#>)?

I guess we'd just need to adjust a fix for synthesized arguments to handle implicit calls to support that...

I didn't get it, have to take a look into this synthesized fix first :)

bq. Not really sure, that would mean a fix for a declaration like func callAsFunction(a: Int) -> Int { 0 } would be (a: <#editor placeholder#>)?

Yeah, that's exactly how I see it.

Ahh, I thought about this at the beginning, but as I was looking at the existing InsertExplicitCall fix for function types and since it didn't have this, I figure it would be something more simple to maintain this behavior here. Do you think is worth introducing this here for both?
Also, in the beginning, I was trying to think from the user perspective that to insert a () call in value mismatch is more likely to be an expected fix than a call with parameters, so the more generic mismatch fix would fit better in this 'with params' case? I'm not sure, just trying to think if insert explicit call would be a better diagnostic in this case :)

This is going to be tricky because we'd effectively have to combine (or coalesce) missing arguments with missing call fix to make it work. I think we'd need some test cases to make sure that if this happens diagnostic logic wouldn't crash since there is no call :)

Yeah, I was also thinking about some other cases where we should give a good thought about like

struct S { func callAsFunction(a: Int) -> Int { 0 } func callAsFunction(b: String) -> Int { 0 } }

What we should do in a mismatchS and Int like suggest one or another doesn't feel right...
So offer two fix-its? In that case, diagnostic wording should probably change in those situations.

I think in this case, if there are no other options, diagnostic should just say that call is missing and let the developer disambiguate...

xedin · 2020-10-16T20:41:27Z

lib/Sema/CSSimplify.cpp

It might not help but I suspect it also hurts something if we have to do this explicitly? :)

Hum, I have to remember exactly which is the case for this ... but I'll take a more detailed look

lib/Sema/CSSimplify.cpp

xedin · 2020-10-16T21:09:47Z

lib/Sema/CSSimplify.cpp

If I understand it correctly, all of the checks for isForImplicitApplyCallableValue sprinkled around the code are necessary because there is no way to come back to a generic "fix" for a type mismatch?... If that's the same then the solution would be to add one more option to the overload set for explicit call which would just register that generic fix and do nothing else - that way we don't need any special handling for call overloads which didn't match...

Humm, that would be to add a fixed choice constraint as the last constraint before addOverloadSet?

Yeah, we'd probably need to do a lookup as part of a fix and adjust the overload set to have that "fallback" choice.

Hum, when trying to do this I was struggling because as my understanding we would have to Constraint::createFixedChoice but didn't know what would be the "fallback" overload choice?

do a lookup as part of a fix

Sorry, not sure I understand this part, that means we have to do a lookup in order to create the fallback fixed choice constraint?

As I see it the logic here would be to:

Use performMemberLookup to form a member set, add a "fallback" choice like "don't do anything but apply a fix (e.g. ContextualMismatch one)

Construct an ApplicableFunction constraint to imitate presence of a missing call.

This way if any of the choices found by lookup produce other fixes they'd be considered a worse solution vs. the "fallback".

Ahh great, thank you @xedin :)
I will get back to this soon

Hey @xedin :)
While getting back to this I've found this behavior SR-13844, so I'm thinking before moving on with this I should take a look at that problem. WDYT?
I'm just asking because I've seen that there is a TODO of some refactors needed in simplifyDynamicCallableApplication and I'm trying to organize between some SRs/PRs I've opened.

@LucianoPAlmeida I think it would make sense to refactor some of the code around that area before going back to this, it might make it easier.

Right, thanks @xedin!
I'll take a look at that then

include/swift/Sema/ConstraintLocator.h

include/swift/Sema/ConstraintLocatorPathElts.def

lib/Sema/CSDiagnostics.h

lib/Sema/CSSimplify.cpp

lib/Sema/ConstraintSystem.cpp

…meter that handles access control

…cess control

…we attempting to verify if a value can be called to produce expected type

…ctly produce mismatch diagnostics for parameterized overload

… callable value can be explicit called to produce type

… for callable values

LucianoPAlmeida · 2021-05-13T02:07:53Z

Just organizing some old PRs, so closing this. It's linked to the ticket for future reference =]

LucianoPAlmeida requested review from hamishknight, hborla and xedin September 20, 2020 20:32

LucianoPAlmeida force-pushed the SR-13503-callasfunction-explicit-call branch from 01e36b6 to 9cac752 Compare September 20, 2020 21:52

xwu reviewed Sep 21, 2020

View reviewed changes

include/swift/AST/DiagnosticsSema.def Outdated Show resolved Hide resolved

xedin requested changes Sep 21, 2020

View reviewed changes

lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved

include/swift/AST/TypeCheckRequests.h Outdated Show resolved Hide resolved

LucianoPAlmeida force-pushed the SR-13503-callasfunction-explicit-call branch 5 times, most recently from 274fa09 to a6bb7a0 Compare September 24, 2020 01:34

LucianoPAlmeida changed the base branch from master to main September 25, 2020 00:02

LucianoPAlmeida force-pushed the SR-13503-callasfunction-explicit-call branch 2 times, most recently from 51b2702 to 8f73343 Compare September 29, 2020 02:16

LucianoPAlmeida force-pushed the SR-13503-callasfunction-explicit-call branch 2 times, most recently from 4bcc9d2 to 203dbe6 Compare October 11, 2020 00:36

LucianoPAlmeida force-pushed the SR-13503-callasfunction-explicit-call branch from 07f0d2f to a83c10b Compare October 13, 2020 11:43

LucianoPAlmeida force-pushed the SR-13503-callasfunction-explicit-call branch 3 times, most recently from d92b4b5 to e52ecc9 Compare October 14, 2020 10:11

LucianoPAlmeida requested a review from xedin October 14, 2020 10:18

LucianoPAlmeida force-pushed the SR-13503-callasfunction-explicit-call branch 2 times, most recently from 9e22659 to df9c27d Compare October 15, 2020 03:30

xwu reviewed Oct 16, 2020

View reviewed changes

include/swift/Sema/ConstraintLocator.h Outdated Show resolved Hide resolved

LucianoPAlmeida force-pushed the SR-13503-callasfunction-explicit-call branch from a968b59 to 84b7e61 Compare October 16, 2020 13:10

xedin requested changes Oct 16, 2020

View reviewed changes

LucianoPAlmeida force-pushed the SR-13503-callasfunction-explicit-call branch from 84b7e61 to 236b8bc Compare October 17, 2020 02:31

LucianoPAlmeida force-pushed the SR-13503-callasfunction-explicit-call branch from f5f8b8a to 49e89bc Compare October 17, 2020 03:41

xwu reviewed Oct 17, 2020

View reviewed changes

pasje approved these changes Oct 19, 2020

View reviewed changes

LucianoPAlmeida force-pushed the SR-13503-callasfunction-explicit-call branch from 7ca544e to 29a798f Compare October 20, 2020 03:18

LucianoPAlmeida force-pushed the SR-13503-callasfunction-explicit-call branch 2 times, most recently from f366e50 to 897046b Compare November 7, 2020 05:09

LucianoPAlmeida force-pushed the SR-13503-callasfunction-explicit-call branch from 897046b to 3e24dee Compare November 9, 2020 23:49

LucianoPAlmeida added 8 commits November 10, 2020 00:18

[AST] Tailored message for diagnose explicit call on callable value

1a30294

[TypeChecker] Adapting IsCallableNominalTypeRequest to receive a para…

7d9e763

…meter that handles access control

[Type] An extra parameter in TypeBase::isCallableNominal to handle ac…

c0a585e

…cess control

[ConstraintLocator] ImplicitCallableValue locator path to track that …

3f18aa9

…we attempting to verify if a value can be called to produce expected type

[tests] Adding a few more edge cases tests for SR-13503

29a4a28

[Sema] Handle innsert explicit call diag for callable values to corre…

14fb15e

…ctly produce mismatch diagnostics for parameterized overload

[Sema] Better naming locator path that indicates we are checking if a…

6affac4

… callable value can be explicit called to produce type

[Sema] Adapt and reuse InsertCall fix instead of creating another one…

2504f50

… for callable values

LucianoPAlmeida force-pushed the SR-13503-callasfunction-explicit-call branch 2 times, most recently from f70c96b to 32f0449 Compare November 11, 2020 02:00

Fixing typos and comments

640fbe6

LucianoPAlmeida force-pushed the SR-13503-callasfunction-explicit-call branch from 32f0449 to 640fbe6 Compare November 12, 2020 01:06

LucianoPAlmeida closed this May 13, 2021

LucianoPAlmeida mentioned this pull request Sep 20, 2020

[SR-13503] InsertExplicitCall fix not created for callable values #55945

Open

[SR-13503] [Sema] Special InsertExplicitCall for nominal callable types #34006

[SR-13503] [Sema] Special InsertExplicitCall for nominal callable types #34006

Uh oh!

Conversation

LucianoPAlmeida commented Sep 20, 2020

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LucianoPAlmeida commented Oct 13, 2020

LucianoPAlmeida commented Oct 13, 2020

LucianoPAlmeida commented Oct 14, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LucianoPAlmeida commented May 13, 2021

Labels

4 participants