Skip to content

[concurrency][Part 2] Fix async objc protocol conformance bugs #35719

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.

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

Conversation

kavon
Copy link
Member

@kavon kavon commented Feb 2, 2021

A continuation of #35662

TODOs:

  • Fix unusual note that claims a protocol's requirements are not satisfied (rdar://73641790). The conformance diagnostic should not be encountered at all.
    • Make an error also be emitted alongside this missing witness note.
    • Determine whether an error should have been encountered at all.
  • Fix a conformance serialization issue that might be related to the unusual note (rdar://73326224).

@kavon kavon force-pushed the objc-async-conformance-bugs-2-electric-boogaloo branch from d54b0b8 to 26090a1 Compare February 4, 2021 00:02
@kavon kavon marked this pull request as ready for review February 4, 2021 00:06
@kavon kavon force-pushed the objc-async-conformance-bugs-2-electric-boogaloo branch from 26090a1 to 472013b Compare February 4, 2021 00:47
@kavon
Copy link
Member Author

kavon commented Feb 4, 2021

@swift-ci please clean smoke test

@kavon kavon marked this pull request as draft February 4, 2021 01:42
@kavon kavon force-pushed the objc-async-conformance-bugs-2-electric-boogaloo branch from 472013b to c8d1f53 Compare February 5, 2021 02:24
@kavon
Copy link
Member Author

kavon commented Feb 5, 2021

@swift-ci please clean smoke test

@kavon kavon force-pushed the objc-async-conformance-bugs-2-electric-boogaloo branch from c8d1f53 to 9f60f3a Compare February 5, 2021 22:57
@kavon
Copy link
Member Author

kavon commented Feb 5, 2021

@swift-ci please smoke test

@kavon kavon force-pushed the objc-async-conformance-bugs-2-electric-boogaloo branch from 9f60f3a to 9db771b Compare February 6, 2021 00:40
@kavon
Copy link
Member Author

kavon commented Feb 6, 2021

@swift-ci please smoke test and merge

@kavon kavon force-pushed the objc-async-conformance-bugs-2-electric-boogaloo branch from 9db771b to 388abdd Compare February 6, 2021 00:45
@kavon
Copy link
Member Author

kavon commented Feb 6, 2021

@swift-ci please smoke test and merge

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

Definitely an improvement. I think there are other places where we have a check like this new duplicateAsyncObjCRequiremetHasBeenWitnesses? The other callers to getObjCRequirements? Should those call this new method instead?

@kavon kavon force-pushed the objc-async-conformance-bugs-2-electric-boogaloo branch from 388abdd to adf6780 Compare February 6, 2021 01:13
This really only happens when the ClangImporter imports an ObjC
protocol that has an async-looking method, which yields two
requirements in the protocol. Only one of these requirements
will be witnessed.

fixes rdar://73326224
@kavon kavon force-pushed the objc-async-conformance-bugs-2-electric-boogaloo branch from adf6780 to 739617c Compare February 9, 2021 01:51
@kavon
Copy link
Member Author

kavon commented Feb 9, 2021

Okay I've refactored it use a search that relies on a predicate, because the use-sites have different requirements. For example, NormalConformance::hasWitness will just check for existing knowledge of witnesses, whereas NormalConformance::getWitness will actually look for witnesses. This should pass all tests now too.

@swift-ci please test and merge

@kavon kavon marked this pull request as ready for review February 9, 2021 01:57
@kavon
Copy link
Member Author

kavon commented Feb 9, 2021

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit ffeb1d5 into swiftlang:main Feb 9, 2021
kavon added a commit to kavon/swift that referenced this pull request Feb 9, 2021
This adds regression test coverage for a
situation related to rdar://73326085, but
was just recently fixed in:

swiftlang#35719
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.

3 participants