Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

[SR-14050] Add test for/remove locator check in matchFunctionRepresentations #56441

Closed
typesanitizer opened this issue Jan 15, 2021 · 5 comments
Closed

[SR-14050] Add test for/remove locator check in matchFunctionRepresentations #56441

typesanitizer opened this issue Jan 15, 2021 · 5 comments
Assignees
@LucianoPAlmeida
Labels
compiler The Swift compiler in itself task

Comments

@typesanitizer
Copy link

Previous ID SR-14050
Radar rdar://problem/73250601
Original Reporter @typesanitizer
Type Task
Status Closed
Resolution Done
Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Task
Assignee @LucianoPAlmeida
Priority Medium

md5: f53286905d7e83dd8b448b8cdb84257e

Issue Description:

There is the following code in CSSimplify's matchFunctionRepresentations.

  case ConstraintKind::Subtype: {
    auto last = locator.last();
    if (!(last && last->is<LocatorPathElt::FunctionArgument>()))
      return false;

It's not clear when this code path affects the behavior of the compiler, if you comment the three lines out, the test suite still continues to pass. If this check is actually useful, we should add a test case exercising it and maybe a comment indicating when it is used. Otherwise, we should delete the code.
@typesanitizer
Copy link
Author

cc luciano (JIRA User)

@typesanitizer
Copy link
Author

@swift-ci create

@LucianoPAlmeida
Copy link
Collaborator

I'll take a look on that =]

@LucianoPAlmeida
Copy link
Collaborator

Just forget everything above(I delete the comment because I made a mistake while looking into it), there are actually a lot of cases where the matching are done for Subtype, it just that all of then the representation is the same so it always allowed. But either way the only cases we can for sure validate early on type checking are argument function types, other cases should be as it is, allowed on typechecking and deferred to SIL because there compiler knows where it can diagnose or depending on the context the conversion is allowed because as mentioned by Slava, conversion is possible "we can emit a thunk that doesn’t capture context". So I think we maybe could maintain this check and just add an additional comment and it should be fine. I'll also try to come up with some test cases to exercise that. Sorry for the misunderstanding, I'll wait on your PR to land to avoid conflicts and look into this =]

@LucianoPAlmeida
Copy link
Collaborator

Seems like there is more into this SR-14063

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@LucianoPAlmeida LucianoPAlmeida

Labels
compiler The Swift compiler in itself task
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@typesanitizer @LucianoPAlmeida