Skip to content

[5.5][Runtime] Fix infinite recursion in protocol conformance checking on class Sub: Super<Sub>. #38592

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

mikeash
Copy link
Contributor

@mikeash mikeash commented Jul 22, 2021

Cherry-pick #38515 to 5.5.

Conformance checks now walk the superclass chain in two stages: stage 1 only walks superclasses that have already been instantiated. When there's a negative result and there's an uninstantiated superclass, then stage 2 will walk the uninstantiated superclasses.

The infinite recursion would occur in this scenario:

class Super<T: P> {}
class Sub: Super<Sub>, P {}

Instantiating the metadata for Super requires looking up the conformance for Sub: P. Conformance checking for Sub would instantiate the metadata for Super to check for a Super: P conformance.

The compiler does not allow the conformance to come from a superclass in this situation. This does not compile:

class Super<T: P>: P {}
class Sub: Super<Sub> {}

Therefore it's not necessary to look at Super when finding the conformance for Sub: P in this particular case. The trick is knowing when to skip Super.

We do need to instantiate Super in the general case, otherwise we can get false negatives. This was addressed in a80fe85, which walks the full superclass chain during conformance checks, even if the superclass has not yet been instantiated. Unfortunately, that causes this infinite recursion.

This fix modifies that fix to make superclass instantiation conditional. The result is the ability to choose between the old algorithm (which skipped uninstantiated superclasses, albeit somewhat by accident) and the new one. A small wrapper then runs the check with the old algorithm, and then only if the old algorithm fails and there is an uninstantiated superclass, it runs with the new one.

Uninstantiated superclasses are uncommon and transient (you only run into this while metadata is in the process of being constructed) so 99.9999% of the time we'll just run the first stage and be done, and performance should remain the same as before.

rdar://80532245

…g on class Sub: Super<Sub>.

Conformance checks now walk the superclass chain in two stages: stage 1 only walks superclasses that have already been instantiated. When there's a negative result and there's an uninstantiated superclass, then stage 2 will walk the uninstantiated superclasses.

The infinite recursion would occur in this scenario:

    class Super<T: P> {}
    class Sub: Super<Sub>, P {}

Instantiating the metadata for Super requires looking up the conformance for Sub: P. Conformance checking for Sub would instantiate the metadata for Super to check for a Super: P conformance.

The compiler does not allow the conformance to come from a superclass in this situation. This does not compile:

    class Super<T: P>: P {}
    class Sub: Super<Sub> {}

Therefore it's not necessary to look at Super when finding the conformance for Sub: P in this particular case. The trick is knowing when to skip Super.

We do need to instantiate Super in the general case, otherwise we can get false negatives. This was addressed in a80fe85, which walks the full superclass chain during conformance checks, even if the superclass has not yet been instantiated. Unfortunately, that causes this infinite recursion.

This fix modifies that fix to make superclass instantiation conditional. The result is the ability to choose between the old algorithm (which skipped uninstantiated superclasses, albeit somewhat by accident) and the new one. A small wrapper then runs the check with the old algorithm, and then only if the old algorithm fails and there is an uninstantiated superclass, it runs with the new one.

Uninstantiated superclasses are uncommon and transient (you only run into this while metadata is in the process of being constructed) so 99.9999% of the time we'll just run the first stage and be done, and performance should remain the same as before.

rdar://80532245
(cherry picked from commit b37252d)
@mikeash mikeash requested review from rjmccall, al45tair and tbkka July 22, 2021 23:50
@mikeash mikeash requested a review from a team as a code owner July 22, 2021 23:50
@mikeash
Copy link
Contributor Author

mikeash commented Jul 22, 2021

@swift-ci please test

@DougGregor DougGregor merged commit f3e29e3 into swiftlang:release/5.5 Jul 23, 2021
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.

2 participants