Skip to content

Implement experimental @abi attribute #76878

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 15 commits into from
Dec 20, 2024

Conversation

beccadax
Copy link
Contributor

@beccadax beccadax commented Oct 4, 2024

This is the beginning of an implementation of the @abi attribute as recently pitched. It is intended to subsume the use of @_silgen_name in ABI compatibility hacks, providing a tool for handling API replacements and enhancements that is both more capable and usable by mere mortals.

This PR also replaces all standard library uses of @_silgen_name with a mangled Swift name with @abi uses. This is about a quarter of all standard library @_silgen_name uses (the remaining uses are to import or export runtime functions or compiler intrinsics by simple known names). (Note: Standard library adoption will be deferred until the type checking rules are more stable.)

There's a lot left to do here, but one of the most important things is making @abi on functions suppressible by providing a fallback to @_silgen_name in the module interface.

SwiftSyntax PR: swiftlang/swift-syntax#2882

// and even though this is @AEIC, the symbol name would conflict.
@_silgen_name("$s11Distributed0A5ActorPAAE20whenLocalTypedThrowsyqd__Sgqd__xYiYaYbqd_0_YKXEYaqd_0_YKs8SendableRd__s5ErrorRd_0_r0_lF")
@abi(
public nonisolated func __api_whenLocal<T: Sendable, E>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably this would be nicer if it stayed using the explicit "explaining why this method is different" name like we did in the silgen name:

Suggested change
public nonisolated func __api_whenLocal<T: Sendable, E>(
public nonisolated func whenLocalTypedThrows<T: Sendable, E>(

Copy link
Contributor Author

@beccadax beccadax Oct 8, 2024

Choose a reason for hiding this comment

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

There's actually three different variants. Here's what I think I'll do here:

  @abi(
    // We need to @abi here because the signature is the same as
    // `__separately_compiled_typed_throws_whenLocal(_:)`, and even though this
    // is @AEIC, the symbol name would conflict.
    public nonisolated func __typed_throws_whenLocal<T: Sendable, E>(
      _ body: @Sendable (isolated Self) async throws(E) -> T
    ) async throws(E) -> T?
  )
  @_alwaysEmitIntoClient
  public nonisolated func whenLocal<T: Sendable, E>(
    _ body: @Sendable (isolated Self) async throws(E) -> T
  ) async throws(E) -> T? { /* snip */ }

  // ABI: This is a workaround when in Swift 6 this method was introduced
  // in order to support typed-throws, but missed to add @_aeic.
  // In practice, this method should not ever be used by anyone, ever.
  @abi(
    public nonisolated func whenLocal<T: Sendable, E>(
      _ body: @Sendable (isolated Self) async throws(E) -> T
    ) async throws(E) -> T?
  )
  @usableFromInline
  nonisolated func __separately_compiled_typed_throws_whenLocal<T: Sendable, E>(
    _ body: @Sendable (isolated Self) async throws(E) -> T
  ) async throws(E) -> T? { /* snip */ }

  // ABI: Historical whenLocal, rethrows was changed to typed throws `throws(E)`
  @abi(
    public nonisolated func whenLocal<T: Sendable>(
      _ body: @Sendable (isolated Self) async throws -> T
    ) async rethrows -> T?
  )
  @usableFromInline
  nonisolated func __rethrows_whenLocal<T: Sendable>(
    _ body: @Sendable (isolated Self) async throws -> T
  ) async rethrows -> T? { /* snip */ }

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good, thank you @beccadax! I'm really enjoying this way to spell these

Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

Thank you for working on this -- the core stdlib parts look good to me. (Great, in fact!)

@_spi(SwiftStdlibLegacyABI) @available(swift, obsoleted: 1)
@usableFromInline
internal func ?? <T>(
internal func _legacy_abi_optionalNilCoalescingOperator <T>(
Copy link
Member

Choose a reason for hiding this comment

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

Ooh, nice catch

@beccadax beccadax force-pushed the abi-changed-your-name branch 4 times, most recently from f3a443a to 6c8eebd Compare October 9, 2024 21:03
@beccadax
Copy link
Contributor Author

beccadax commented Oct 9, 2024

@swift-ci please test

@beccadax
Copy link
Contributor Author

beccadax commented Oct 9, 2024

I think the implementation here is getting towards something mergeable, if not fully usable, but I expect the feature to continue to evolve, including in some ways that will break source compatibility with the current version. (For example, I may ban access control keywords in an @abi attribute and instead inherit them from the attribute it's attached to.) For that reason, I'm planning to move the standard library adoption into its own PR before this comes out of draft and gets merged. The stdlib PR will continue to evolve as the compiler implementation improves.

@nkcsgexi nkcsgexi self-requested a review October 9, 2024 22:57
@beccadax
Copy link
Contributor Author

The macOS test failure, at least, was caused by SwiftSyntax not being able to parse the new syntax, so I'm adding support for that.

@beccadax beccadax force-pushed the abi-changed-your-name branch 2 times, most recently from 7b715b7 to 446ecbc Compare October 16, 2024 00:38
beccadax added a commit to beccadax/swift-syntax that referenced this pull request Oct 16, 2024
@beccadax
Copy link
Contributor Author

With swiftlang/swift-syntax#2882

@swift-ci please test

beccadax added a commit to beccadax/swift-syntax that referenced this pull request Oct 28, 2024
@beccadax beccadax force-pushed the abi-changed-your-name branch 2 times, most recently from d332e60 to a007f36 Compare October 29, 2024 18:47
@beccadax
Copy link
Contributor Author

With swiftlang/swift-syntax#2882

@swift-ci please test

@@ -0,0 +1,154 @@
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=KEYWORD2 | %FileCheck %s -check-prefixes=KEYWORD2,KEYWORD2_DISABLED
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that you can use %batch-code-completion here instead. It has the benefit of not needing the many RUN lines + can re-use the ast context. Thanks for adding these by the way, it's very much appreciated!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to modify the tool with a feature that could add suffixes to the check prefixes, but this definitely cleaned things up quite a bit!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nice, thank you! CC @rintaro

beccadax added a commit to beccadax/swift-syntax that referenced this pull request Nov 5, 2024
@beccadax beccadax force-pushed the abi-changed-your-name branch 2 times, most recently from 99ddda0 to 7a96bdd Compare November 7, 2024 20:32
@beccadax beccadax force-pushed the abi-changed-your-name branch 2 times, most recently from 7fd234b to c2cd7b7 Compare December 6, 2024 01:11
@beccadax
Copy link
Contributor Author

beccadax commented Dec 6, 2024

@swift-ci please test

@beccadax beccadax marked this pull request as ready for review December 6, 2024 01:12
@beccadax beccadax force-pushed the abi-changed-your-name branch from 4c63dd8 to 6c8adfb Compare December 9, 2024 20:07
Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

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

Parser and ASTGen related changes LGTM!

@beccadax beccadax force-pushed the abi-changed-your-name branch 2 times, most recently from 1293a50 to 3483a6c Compare December 13, 2024 02:20
@beccadax
Copy link
Contributor Author

@swift-ci test

@beccadax beccadax force-pushed the abi-changed-your-name branch 2 times, most recently from da21faf to 1fd92db Compare December 13, 2024 20:58
@beccadax beccadax force-pushed the abi-changed-your-name branch from 1fd92db to 287e0bc Compare December 13, 2024 22:23

public:
void visitABIAttr(ABIAttr *attr) {
Decl *AD = attr->abiDecl;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewers: In this PR, I'm mostly just trying to make sure I can find the right decls to check against one another. The exact rules I'm enforcing are extremely rough and incomplete placeholders; properly defining them will be the subject of my next PR on this feature.

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

Sema changes look good to me!

/// `ABIAttr::abiDecl` to the declaration the `ABIAttr` is attached to.
///
/// \seeAlso \c recordABIAttr()
llvm::DenseMap<Decl *, Decl *> ABIDeclCounterparts;
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered turning this into a cached request instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be difficult to compute lazily because there's no way (other than this table) to walk up from an ABI-only declaration to its counterpart; you have to start from the counterpart and then walk down to the ABI-only decl. The most reliable way I've found to do that is to eagerly populate a table while parsing or deserializing. I suppose I could still create a dummy request whose evaluate() function returns the default output and then make recordABIAttr() manually cache non-default outputs, but I'm not sure if that would really buy us anything over what I'm doing here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am suggesting it because we have a similar pattern with other requests that are cached manually after parse and deserialization, the primary advantage is use a common mechanism and storage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 413c673.

Changes dump output used in one test; otherwise NFC.
The `@differentiable` and `@derivative` attributes need a parent pointer. Move the code to populate it from Parser to AST so it can be more easily shared between the parsers.

Done in preparation for similar code to be added for `@abi`.
This attribute will allow you to specify an alternate version of the declaration used for mangling. It will allow minor adjustments to be made to declarations so long as they’re still compatible at the calling convention level, such as refining isolation or sendability, renaming without breaking ABI, etc.

The attribute is behind the experimental feature flag `ABIAttribute`.
The new `DECL_ATTR_FEATURE_REQUIREMENT` macro in DeclAttr.def can be used to declare that an attribute should only be available when a related language feature is enabled.

Effects:

• `#if hasAttribute(someAttr)` will return `false` unless the required feature is enabled.
• Code completion will not include the attribute unless the required feature is enabled.
• `TypeChecker::checkDeclAttributes()` diagnoses non-implicit uses of the attribute.

Add this mechanism and use it to tie @abi to the ABIAttribute feature. Also design tests for it.
Sema now type-checks the alternate ABI-providing decls inside of @abi attributes.

Making this work—particularly, making redeclaration checking work—required making name lookup aware of ABI decls. Name lookup now evaluates both API-providing and ABI-providing declarations. In most cases, it will filter ABI-only decls out unless a specific flag is passed, in which case it will filter API-only decls out instead. Calls that simply retrieve a list of declarations, like `IterableDeclContext::getMembers()` and friends, typically only return API-providing decls; you have to access the ABI-providing ones through those.

As part of that work, I have also added some basic compiler interfaces for working with the API-providing and ABI-providing variants. `ABIRole` encodes whether a declaration provides only API, only ABI, or both, and `ABIRoleInfo` combines that with a pointer to the counterpart providing the other role (for a declaration that provides both, that’ll just be a pointer to `this`).

Decl checking of behavior specific to @abi will come in a future commit.

Note that this probably doesn’t properly exercise some of the new code (ASTScope::lookupEnclosingABIAttributeScope(), for instance); I expect that to happen only once we can rename types using an @abi attribute, since that will create distinguishable behavior differences when resolving TypeReprs in other @abi attributes.
Check for:

• Matching decl kinds
• Matching PBD shapes (does every VarDecl on both sides have a counterpart?)
• Matching function effects
• Matching function arity (roughly)
What’s implemented now is actually *far* more thorough than what the surface syntax can currently express, mainly because I can’t apply @abi to nominal types yet.
Use `Decl::attachParsedAttrs()` instead of `Decl::setAttrs()` to attach attributes to a declaration in ASTGen. This causes the common attribute-setup logic there to be run.

NFC in this commit because none of the attributes that have special setup logic are currently implemented in ASTGen. Prepares to add support for `@abi` in a future commit.
@beccadax beccadax force-pushed the abi-changed-your-name branch from 287e0bc to 413c673 Compare December 20, 2024 05:22
@beccadax
Copy link
Contributor Author

@swift-ci please test

@beccadax beccadax merged commit a6df4ef into swiftlang:main Dec 20, 2024
5 checks passed
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.

7 participants