[Type checker] Refactor override checking #17733
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a separable path; give it its own path and isolate the state changes to TypeChecker::resolveOverriddenDecl(). NFC.
Break the gigantic swift::checkOverrides() into a few separable pieces encapsulated in a new class, OverrideMatcher: * Setup: establish whether we can do matching at all * Matching: check whether we have a match based on a set of checking rules * Checking: check that the selected override is semantically correct * Search: apply various matching rules to try to find the overridden declaration While here, limit the dependencies of override checking somewhat. Specifically, don’t ask for the type of the declaration until we’ve found a potential overridden declaration to check against, and even then only do it when the “simple” checks fail.
…-C interop. For reasons lost to history and not interesting enough to uncover, the “override” computation performed a check of the overriding declaration’s Objective-C selector against the potentially-overridden declaration’s Objective-C selector. This introduced a cycle dependency between the “is exposed to Objective-C” computation and the “overridden declarations” computation, because @objc is inherited from an overridden declaration. We’ll detect the same errors via other, existing diagnostics, so prune out the Objective-C selector checks from the override path.
ValueDecl::getOverriddenDecl(s)() does this for us automagically.
resolveOverriddenDecl() is meant to be used to provide access to the overridden declarations, e.g., from another source file. Only resolve when the declaration has an explicit ‘override’ modifier or is an initializer (for which there are cases where ‘override’ is not required). This does not yet reduce override checking across source files, because checkOverrides() is still called during validateDeclForNameLookup(), and we’re still doing too much work to compute overrides.
…bit. Because, you know, I’d like it to do what it says it does.
|
@swift-ci please smoke test and merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Break the gigantic
swift::checkOverrides()into a few separable piecesencapsulated in a new class, OverrideMatcher:
While here, limit the dependencies of override checking somewhat.
Specifically, don’t ask for the type of the declaration until we’ve found
a potential overridden declaration to check against, and even then only do
it when the “simple” checks fail.