Skip to content

Conversation

@LucianoPAlmeida
Copy link
Contributor

@LucianoPAlmeida LucianoPAlmeida commented May 9, 2020

Improving the diagnostics when the key path type can't be inferred by context. e.g. let _ : AnyKeyPath = \.x

cc @xedin @hborla @varungandhi-apple :)

@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-12745-diagnostics-keypath branch from 08466d9 to 8708374 Compare May 9, 2020 15:20
@varungandhi-apple
Copy link
Contributor

@swift-ci please test

@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-12745-diagnostics-keypath branch 2 times, most recently from 64981c1 to 8ba94f9 Compare May 10, 2020 20:47
@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-12745-diagnostics-keypath branch 2 times, most recently from d64bb42 to 5e881bc Compare May 10, 2020 22:51
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.

Thanks for looking into this, @LucianoPAlmeida!

Couple of thoughts:

  • I think we should split this into two separate PRs, one for contextual stuff and other one for keyPath: overload.

  • Regarding keyPath:

    • I think we can do better in this case and instead of saying maybe we could actually verify that keyPath: overload choice works (good that it doesn't have to be a declaration :)) and suggest adding keyPath: label.
  • Regarding root/value inference failure:

    • Is it any different from a regular inability to infer generic arguments?
    • I think we should add a special diagnostic about AnyKeyPath which state that it doesn't
      provide enough contextual information to allow root type to be inferred implicitly
      (because its root type is Any). That should help resolve at least some confusion here...
    • Could we suggest a fix-it in this case to specify <#Root#> before a leading dot ?

@LucianoPAlmeida
Copy link
Contributor Author

  • I think we should split this into two separate PRs, one for contextual stuff and other one for keyPath: overload.

Sure! Let's handle contextual first here then :)

  • I think we can do better in this case and instead of saying maybe we could actually verify that keyPath: overload choice works (good that it doesn't have to be a declaration :)) and
    suggest adding keyPath: label.

Noted :)

  • Is it any different from a regular inability to infer generic arguments?

I didn't thought about that, perhaps there is no difference...
but I think we can tailor the diagnostics for key paths since generic arguments is more general...?

  • I think we should add a special diagnostic about AnyKeyPath which state that it doesn't
    provide enough contextual information to allow root type to be inferred implicitly
    (because its root type is Any). That should help resolve at least some confusion here...
  • Could we suggest a fix-it, in this case, to specify <#Root#> before a leading dot ?

I'll work on those, thanks @xedin :)

@xedin
Copy link
Contributor

xedin commented May 11, 2020

I think we can tailor the diagnostics for key paths since generic arguments is more general.

I agree that we can tailor diagnostic messages but I think emission of the fix/diagnostic should be based on what we do for generic parameters.

@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-12745-diagnostics-keypath branch 3 times, most recently from ce42050 to 34f40a8 Compare May 11, 2020 21:12
@LucianoPAlmeida LucianoPAlmeida requested a review from xedin May 11, 2020 21:13
@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-12745-diagnostics-keypath branch from 34f40a8 to 16f1fd2 Compare May 11, 2020 21:56
@artemcm
Copy link
Contributor

artemcm commented May 11, 2020

I was accidentally concurrently looking into this issue. From what I gather, this PR is now resolving the more general issue with key path types not being inferred from context, but not the issue of the keyPath: label overload?

@LucianoPAlmeida
Copy link
Contributor Author

I was accidentally concurrently looking into this issue. From what I gather, this PR is now resolving the more general issue with key path types not being inferred from context, but not the issue of the keyPath: label overload?

Initially, it was trying to handle both, but I just remove the commit since @xedin suggested split in two PRs, will do a follow up as soon as this one lands :)

@artemcm
Copy link
Contributor

artemcm commented May 11, 2020

@LucianoPAlmeida, I can't see the removed commits, but please take a peak at #31716, I imagine it is something similar?

@varungandhi-apple
Copy link
Contributor

I'm a bit confused as to what's going to happen here? Are we merging Artem's PR or this one? I can't comment on the implementation but I think the diagnostic looks good. 👍

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 87083743c6cff0d750ef59744b6bb5d3b8862f57

@LucianoPAlmeida
Copy link
Contributor Author

I'm a bit confused as to what's going to happen here? Are we merging Artem's PR or this one? I can't comment on the implementation but I think the diagnostic looks good. 👍

@varungandhi-apple We decided to split this PR into two parts contextual inference issues, and subscript fixes as a follow-up. So we let this one only to solve contextual issues and planning a follow up for the keyPath label... but coincidentally the second part is already handled by Artem's PR :)

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 87083743c6cff0d750ef59744b6bb5d3b8862f57

@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-12745-diagnostics-keypath branch from 16f1fd2 to 9240366 Compare May 12, 2020 17:35
@LucianoPAlmeida
Copy link
Contributor Author

I think Artem's covers PR the whole scope of the SR, so I removed the test cases related and will let this be only contextual stuff :)

@LucianoPAlmeida LucianoPAlmeida changed the title [SR-12745] Improve keypath diagnostic when cannot be inferred by context [Diagnostics] Improve keypath diagnostic when cannot be inferred by context May 12, 2020
@xedin
Copy link
Contributor

xedin commented May 12, 2020

@LucianoPAlmeida I think it's from context instead of by context but I might be wrong...

@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-12745-diagnostics-keypath branch from 9240366 to 7c19460 Compare May 12, 2020 20:58
@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-12745-diagnostics-keypath branch 2 times, most recently from 85e1eed to 9b422b0 Compare May 13, 2020 01:11
@LucianoPAlmeida LucianoPAlmeida force-pushed the SR-12745-diagnostics-keypath branch from 9b422b0 to 9f5c113 Compare May 13, 2020 01:26
@xedin
Copy link
Contributor

xedin commented May 13, 2020

@swift-ci please smoke test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants