-
Notifications
You must be signed in to change notification settings - Fork 10.5k
ABI checker: diagnose the missing of @available attributes for added ABIs #25104
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 checker: diagnose the missing of @available attributes for added ABIs #25104
Conversation
…ns in error messages External type declarations are synthesized to incorporate members in extensions to types of external modules. In diagnostics, we should use 'extension' instead of 'struct/class' for these decls to avoid confusion.
@swift-ci please smoke test |
// Diagnose the missing of @available attributes. | ||
// Decls with @_alwaysEmitIntoClient aren't required to have an | ||
// @available attribute. | ||
if (D->getIntroducingVersion().empty() && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swift availability doesn't count as OS availability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will Fix!
…ABIs New ABIs should have an @available attribute to describe the introducing version. This patch teaches the tool to diagnose its missing. Decls with @_AlwaysEmitIntoClient are excluded from the blaming lists since they are essentially available all the time. rdar://51089418
7b5e9f2
to
f55d3ad
Compare
@swift-ci please smoke test |
For frameworks not shipping with OSs, the users should specify this flag to avoid diagnosing some changes, such as the missing of OS availability attributes.
@swift-ci please smoke test |
…are enabled We started to diagnose new APIs such as the missing of @available attributes. This diverges the expected change lists for builds with/without assertions enabled because some stdlib symbols are included only when asserts are enabled. We start to require asserts here to make sure the test doesn't fail on no-assertion settings.
@swift-ci please smoke test |
@@ -1,4 +1,5 @@ | |||
// REQUIRES: OS=macosx | |||
// REQUIRES: asserts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems very dangerous. Can we have a with-asserts and a without-asserts variant, and say that the dump has to match one of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like a good idea. I'll add another test requires no asserts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be easiest to keep them in sync if we assume the with-asserts variant is a superset. Then it can just use the cat
of the two files as the expected changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just split the expected changes to two lists: one without asserts and the other the additional blames when asserts are enabled. The asserts-enabled test will diff against the combination of both.
@swift-ci please smoke test |
4f25102
to
90f8498
Compare
90f8498
to
de1718e
Compare
@swift-ci please smoke test |
New ABIs should have an @available attribute to describe the introducing version. This patch teaches the tool to diagnose its missing.
Decls with @_AlwaysEmitIntoClient are excluded from the blaming lists since they are essentially available all the time.
Thanks for the inputs from @lorentey and @jrose-apple !
rdar://51089418