Skip to content
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

@abi checking #79466

Merged
merged 22 commits into from
Mar 26, 2025
Merged

@abi checking #79466

merged 22 commits into from
Mar 26, 2025

Conversation

beccadax
Copy link
Contributor

@beccadax beccadax commented Feb 18, 2025

This PR adds logic to confirm that the ABI-only declaration in an @abi attribute is compatible with its counterpart—that is, that they are similar enough to one another that callees who expect the ABI-only decl will be able to successfully call the API decl. For instance, it will diagnose differences in the arity or generic signature, but also many more subtle differences.

It attempts to allow any differences that the checker knows are immaterial to call compatibility, such as:

  • Different declaration names, argument labels, etc.
  • Tuples with different labels but otherwise the same element type
  • Array<T> instead of T... in parameters
  • Uses of marker protocols or sending
  • Parameter conventions that are mangled differently but represent the same memory management behavior

However, especially for type differences, these are effectively opt-in—there may be situations where an ABI-only decl is compatible with its API counterpart, but the compiler can't detect that and rejects it.

The compiler will also diagnose if there's anything unnecessary in the @abi declaration. This includes some very common things, such as default arguments, access control modifiers, and @available attributes. The compiler will offer fix-its to remove them.

This PR doesn't actually adopt the feature yet. Standard library adoption has been cherry-picked atop this branch in #79937.


Reviewers: This is a large PR, but mostly owing to the fact that it specifies and tests behaviors for tons of attributes. I have attempted to split it into manageable commits, but this was done after the fact, so there's a little bit of wonkiness in the tests (expected-errors with diagnostics we haven't added yet, commits that make certain attributes irrelevant but don't actually ban them). It should all be tested in the end.

One issue I'm particularly worried about is whether I've forgotten any aspects of a declaration that need to be checked. Types will tend to fail safe, and there are mechanisms to make sure all attributes have a specified behavior, but it's totally possible for me to e.g. forget to check the failability of initializers (this actually happened, though I caught it myself) and thereby accidentally allow an ABI incompatibility to slip through.

@beccadax beccadax force-pushed the abi-inspected-your-appearance branch 6 times, most recently from a4c1514 to 5da1fb4 Compare February 25, 2025 05:54
@beccadax beccadax force-pushed the abi-inspected-your-appearance branch 5 times, most recently from afc7fd8 to de90dec Compare March 5, 2025 01:05
@beccadax beccadax force-pushed the abi-inspected-your-appearance branch 3 times, most recently from 1d6d7f7 to fc819d7 Compare March 12, 2025 03:04
@beccadax
Copy link
Contributor Author

@swift-ci please test

@beccadax beccadax force-pushed the abi-inspected-your-appearance branch from 2c2349d to 82a897e Compare March 12, 2025 21:11
@beccadax beccadax force-pushed the abi-inspected-your-appearance branch 2 times, most recently from 5ca85f5 to 96739a9 Compare March 13, 2025 00:40
@beccadax
Copy link
Contributor Author

@swift-ci please test

@beccadax beccadax force-pushed the abi-inspected-your-appearance branch from 96739a9 to 0ebcb3d Compare March 13, 2025 19:44
@beccadax
Copy link
Contributor Author

The test failures on 96739a9 will require minor tweaks (mostly to tests), but this is basically ready to review.

@beccadax beccadax marked this pull request as ready for review March 13, 2025 19:49
beccadax added 21 commits March 26, 2025 10:47
Allows code to get this for any AbstractStorageDecl.
Make this logic accessible outside of the mangler.
Create a helper method which looks for an `@objc` attribute with an explicit name and returns it, and adopt it in various existing places.
Adds assertions in various places where properties that can vary between ABI-only decls and their counterparts—particularly function and parameter attributes—are handled in SILGen, ensuring that we don’t accidentally end up processing ABI-only decls there.
This commit compares the decl inside the `@abi` attribute to the decl it’s attached to, diagnosing ABI-incompatible differences. It does not yet cover attributes, which are a large undertaking.
ABI-only declarations now query their API counterpart for things like `isObjC()`, their ObjC name, dynamic status, etc. This means that `@objc` and friends can simply be omitted from an `@abi` attribute.

No tests in this commit since attribute checking hasn’t landed yet.
ABI-only declarations now inherit access control modifiers like `public` or `private(set)`, as well as `@usableFromInline` and `@_spi`, from their API counterpart. This means these attributes and modifiers don’t need to be specified in an `@abi` attribute.

Very few tests because we aren’t yet enforcing the absence of these attributes.
ABI-only declarations now inherit `@available`, `@backDeployed`, etc. from their ABI counterpart. This will make it unnecessary to specify these attributes in `@abi`. Also some changes to make sure we suggest inserting `@available` in the correct place.

No tests because the enforcement is not yet in.
ABI-only decls will now inherit `override` from their API counterpart; the keyword is unnecessary and `getOverriddenDecls()` will get the ABI counterparts of the decls the API counterpart overrides.

No tests because we don’t have the enforcement mechanism in yet.
It’s unnecessary, shouldn’t be serialized into module interfaces, and Swift doesn’t know how to compute it for an ABI-only decl since it doesn’t have accessors or an initial value.

No tests because enforcement isn’t in yet.
Adds a new method to `DeclAttribute` which can compare two attributes and see if they would be “equivalent” in light of the given decl. “Equivalent” here means that they would have the same effect on the declaration; for instance, two attrs with different source locations can be equivalent, and two attributes with the same arguments in a different order are equivalent if the ordering of those argumetns is not semantically equivalent.

This capability is not yet used in this commit, but in a future commit `@abi` will check if certain attributes are equivalent or not.
This PR adds a set of DeclAttr.def flags for specifying how a given attribute interacts with `@abi`, and declares a behavior for each existing attribute. Future attributes will be *required* to declare an `@abi` behavior lest they fail a static assert.

Note that the behavior is not actually enforced in this commit—it is merely specified here.
This commit compares the attributes on the decl inside the `@abi` attribute to those in the decl it’s attached to, diagnosing ABI-incompatible differences. It also rejects many attributes that don’t need to be specified in the `@abi` attribute, such as ObjC-ness, access control, or ABI-neutral traits like `@discardableResult`, so developers know to remove them.
And make sure we reject it on `deinit`s and accessors.
When printing an `@abi` attribute’s decl, we now filter out any attrs that are not valid in that position. Fixes a broken test.
Specify whether the type with the problem is a result type, parameter type (and which parameter), etc.
A same-type constraint in an enclosing `where` clause will eliminate a generic parameter’s ABI impact. Teach `ABIDeclChecker::checkType()` about this so it can handle a known-unsupported case for `Swift.Result.init(catching:)`.
Remove a diagnostic that isn’t occurring anymore.
@beccadax beccadax force-pushed the abi-inspected-your-appearance branch from a7f4a0c to 6759ad5 Compare March 26, 2025 17:48
@beccadax
Copy link
Contributor Author

@swift-ci please smoke test

@beccadax beccadax enabled auto-merge March 26, 2025 18:05
@beccadax beccadax merged commit a5a84f1 into swiftlang:main Mar 26, 2025
3 checks passed
@nkcsgexi
Copy link
Contributor

🎉

// REQUIRES: swift_feature_StrictMemorySafety
// REQUIRES: swift_feature_SymbolLinkageMarkers

import _Differentiation
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a REQUIRES: differentiable_programming if one wants to import _Differentiation.


import _Differentiation

import Distributed
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a REQUIRES: distributed for this import Distributed as well.

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