Skip to content

[SILSymbolVisitor] Skip internal nominal type decls #65175

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zixu-w
Copy link
Contributor

@zixu-w zixu-w commented Apr 14, 2023

SILSymbolVisitor crashes with deserialization failures with opaque type in internal modules when adding conformances to public protocols.

Resolves rdar://107672461

`SILSymbolVisitor` crashes with deserialization failures with opaque
type in internal modules when adding conformances to public protocols.

Resolves rdar://107672461
@zixu-w
Copy link
Contributor Author

zixu-w commented Apr 14, 2023

Actually this is superseded by #64991 where the deserialization was fixed. The test failed earlier without the SILSymbolVisitor fix, but now passes on latest main. Double-checking

@zixu-w
Copy link
Contributor Author

zixu-w commented Apr 14, 2023

Yep. #64991 fixed it 🎉

@zixu-w zixu-w closed this Apr 14, 2023
@zixu-w
Copy link
Contributor Author

zixu-w commented Apr 14, 2023

Re-opening for planning reasons

@zixu-w zixu-w reopened this Apr 14, 2023
@zixu-w
Copy link
Contributor Author

zixu-w commented Apr 14, 2023

@swift-ci Please smoke test

Copy link
Contributor

@xymus xymus left a comment

Choose a reason for hiding this comment

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

It looks good!

@zixu-w
Copy link
Contributor Author

zixu-w commented Apr 14, 2023

@swift-ci Please smoke test

1 similar comment
@zixu-w
Copy link
Contributor Author

zixu-w commented Apr 14, 2023

@swift-ci Please smoke test

@zixu-w
Copy link
Contributor Author

zixu-w commented Apr 14, 2023

@swift-ci Please smoke test

1 similar comment
@zixu-w
Copy link
Contributor Author

zixu-w commented Apr 17, 2023

@swift-ci Please smoke test

@xymus
Copy link
Contributor

xymus commented Jun 1, 2023

@zixu-w Do you plan on merging this? I think it's still a worthwhile change on its own even if we avoided last crash in a different way.

@zixu-w
Copy link
Contributor Author

zixu-w commented Jun 1, 2023

@zixu-w Do you plan on merging this? I think it's still a worthwhile change on its own even if we avoided last crash in a different way.

IIRC this was blocked by another crash with this specific test case. Let me try the test again
@swift-ci Please smoke test

@zixu-w
Copy link
Contributor Author

zixu-w commented Jun 1, 2023

@swift-ci Please smoke test

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