Skip to content

Add a Recursion Breaker to Opaque Type Substitution #40971

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

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

CodaFi
Copy link
Contributor

@CodaFi CodaFi commented Jan 22, 2022

The current check in MiscDiagnostics can only handle local 2-cycles as in:

func foo() -> some P { return bar() }
func bar() -> some P { return foo() }

Larger cycles, such as the 3-cycle present in the validation test in this patch cannot be detected without resolving all substitutions up front. Therefore, we take the tact that we can at least prevent the compiler from crashing. At runtime, instead, type resolution will blow out the stack.

The check here is simple: Plumb through a set of opaque type decls that the resolution machinery has already seen, and if we encounter a recursive opaque type substitution bail with the original opaque type.

rdar://87121502

@CodaFi CodaFi requested a review from jckarter January 22, 2022 02:09
@CodaFi
Copy link
Contributor Author

CodaFi commented Jan 22, 2022

@swift-ci smoke test

@CodaFi CodaFi force-pushed the hitting-the-stacks branch from 10cfe59 to 33168de Compare January 23, 2022 19:28
@CodaFi
Copy link
Contributor Author

CodaFi commented Jan 24, 2022

@swift-ci smoke test

@CodaFi
Copy link
Contributor Author

CodaFi commented Jan 25, 2022

Docker's busted

@swift-ci smoke test Linux platform

The current check in MiscDiagnostics can only handle local 2-cycles as in:

func foo() -> some P { return bar() }
func bar() -> some P { return foo() }

Larger cycles, such as the 3-cycle present in the validation test in this patch cannot be detected without resolving all substitutions up front. Therefore, we take the tact that we can at least prevent the compiler from crashing. At runtime, instead, type resolution will blow out the stack.

The check here is simple: Plumb through a set of opaque type decls that the resolution machinery has already seen, and if we encounter a recursive opaque type substitution bail with the original opaque type.

rdar://87121502
@CodaFi CodaFi force-pushed the hitting-the-stacks branch from 33168de to ad94cbd Compare January 25, 2022 21:01
@CodaFi
Copy link
Contributor Author

CodaFi commented Jan 25, 2022

@swift-ci smoke test

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.

Nice catch!

@CodaFi CodaFi merged commit 3165ea5 into swiftlang:main Jan 26, 2022
@CodaFi CodaFi deleted the hitting-the-stacks branch January 26, 2022 02:18
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