Skip to content

Conversation

Xazax-hun
Copy link
Contributor

This PR is another attempt at landing #76903. The changes compared to the original PR:

  • Instead of increasing the size of SILDeclRef, store the necessary type information in a side channel using withClosureTypeInfo.
  • Rely on SGFContext to get the right ClangType
  • Extend BridgingConversion with an AbstractionPattern to store the original clang type.
  • The PR above introduced a crash during indexing system modules that references foreign types coming from modules imported as implementation only. These entities are implementation details so they do not need to be included during serialization. This PR adds a test and adds logic to exclude such clang types in the serialization process.

rdar://131321096&141786724

@Xazax-hun
Copy link
Contributor Author

This is a slightly different approach to fix the same bits as #82486. The serialization related changes are the same and already got PR reviews for that in #82486.

Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

I'm a little worried about the serialization changes. Are we relying on preserving Clang function types more than we were before?

@Xazax-hun Xazax-hun force-pushed the const-ref-crash-take-4 branch from 56ddae6 to 13b96a1 Compare August 21, 2025 09:07
@Xazax-hun
Copy link
Contributor Author

Are we relying on preserving Clang function types more than we were before?

Yes, but we need this information to properly do type lowering for closure arguments, so I do not see a way around this without a major redesign. What are your concerns?

@rjmccall
Copy link
Contributor

Are we relying on preserving Clang function types more than we were before?

Yes, but we need this information to properly do type lowering for closure arguments, so I do not see a way around this without a major redesign. What are your concerns?

Just that IIRC serialization was the major thing blocking us from putting Clang types into the AST in the first place, so if we're serializing them in more places, that gives me pause. I wouldn't have expected this technical approach to require that.

…ce structs

This PR is another attempt at landing swiftlang#76903. The changes compared to
the original PR:
* Instead of increasing the size of SILDeclRef, store the necessary type
  information in a side channel using withClosureTypeInfo.
* Rely on SGFContext to get the right ClangType
* Extend BridgingConversion with an AbstractionPattern to store the
  original clang type.
* The PR above introduced a crash during indexing system modules that
  references foreign types coming from modules imported as
  implementation only. These entities are implementation details so they
  do not need to be included during serialization. This PR adds a test
  and adds logic to exclude such clang types in the serialization
  process.

rdar://131321096&141786724
@Xazax-hun Xazax-hun force-pushed the const-ref-crash-take-4 branch from 13b96a1 to f267492 Compare September 9, 2025 11:08
@Xazax-hun
Copy link
Contributor Author

@swift-ci please test

Copy link
Member

@kavon kavon left a comment

Choose a reason for hiding this comment

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

LGTM

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