Skip to content

stdlib: Place @_semantics prespecialization requirement target under correct constraints #32324

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
Jul 1, 2020

Conversation

AnthonyLatsis
Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis commented Jun 11, 2020

subscript.read requires Bound: Strideable, Bound.Stride: SignedInteger.

The substitution map refactor in #31895 revealed this with a crash. With a bit of guidance, I am happy to add a regression test for master too, if warranted.

@CodaFi CodaFi requested a review from gottesmm June 12, 2020 22:21
@gottesmm
Copy link
Contributor

@CodaFi I think @atrick understands prespecialization the best now (and how -Onone support works).

@CodaFi CodaFi requested review from atrick and removed request for gottesmm June 13, 2020 05:08
@AnthonyLatsis
Copy link
Collaborator Author

@atrick ping

Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

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

This looks like a straightforward fix, so I approved it. I have to admit, I don't understand how the issue was exposed or what would break in the old version. The resulting symbol name should be the same either way.

@AnthonyLatsis
Copy link
Collaborator Author

AnthonyLatsis commented Jul 1, 2020

@atrick I should have linked the explanation, sorry. The issue was exposed because substitution maps have become part of the identity of a BoundGenericType in #31895, resulting in Assertion failed: (ReInfo.getSpecializedType() == SpecializedF->getLoweredFunctionType() && "Previously specialized function does not match expected type."). The difference in substitution maps causing the assertion failure is due to subscript.read and the corresponding prespecialization point having different conditional requirements. It looks like the prespecialized read coroutine is currently receiving pattern substitutions with an invalid Strideable conformance. I don't know of any real-world implications to this though; perhaps the prespecialization is just left unused.

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

@atrick
Copy link
Contributor

atrick commented Jul 1, 2020

OK thanks. Makes sense. This PR looks like it's just adding some test flags though: #31893 [silgen] Add an extra swift-version 5 run to initializer tests

@AnthonyLatsis
Copy link
Collaborator Author

This PR looks like it's just adding some test flags though

My bad, fixed the link.

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

@AnthonyLatsis AnthonyLatsis merged commit fe82a70 into swiftlang:master Jul 1, 2020
@AnthonyLatsis AnthonyLatsis deleted the prespec-oopsie branch July 1, 2020 09:22
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.

4 participants