[test] Reenable the stdlib ABI checker in PR tests

[lorentey]

The asserts test currently requires array_cow_checks, but that feature isn't enabled by default during PR tests.

rdar://88153292

[lorentey]

@swift-ci test

[lorentey]

Looking good:

PASS: Swift(macosx-x86_64) :: api-digester/stability-stdlib-abi-with-asserts.test (6508 of 15267)

[benrimmington]

Can -with-array-cow-checks.test and -without-asserts.test be checked by other CI build presets?

---

For the -without-asserts.test file:

• Does it also need an // UNSUPPORTED: array_cow_checks comment?

• Why does its // RUN: %api-digester comment have a -v flag?
  (The other tests don't use this flag.)

• The false positives might be solved by ABIChecker: obsoleted/unavailable ABI-public decls #33694.
  The code change simply moves an if statement in the ModuleAnalyzerNodes.cpp file.
  (I closed that PR because it also involves updating the baseline.)

[lorentey]

Can -with-array-cow-checks.test and -without-asserts.test be checked by other CI build presets?

We definitely have CI configurations that run the -without-asserts test -- e.g. oss-swift_tools-RA_stdlib-RD_test-simulator is one.

I don't know about -with-array-cow-checks; I added that test as a fallback in case that condition gets (re)enabled by default, to protect against the ABI checks getting accidentally disabled again when that happens.

The critical test is without-asserts -- that is the one that covers what actually ships in ABI stable releases. However, PR tests have assertions enabled, so they can only run one of the other two. This is still a good enough approximation in practice, as long as we always have one of these tests running.

For the -without-asserts.test file:

• Does it also need an // UNSUPPORTED: array_cow_checks comment?

I don't think so -- I don't expect the extra CoW checks to be enabled in no-assert builds of the stdlib, and if they do get enabled, the test failure will be a good signal.

• Why does its // RUN: %api-digester comment have a -v flag? (The other tests don't use this flag.)

Huh, good catch. It enables verbose output in the build logs. I don't think the difference matters, but it'd probably be a good idea to keep it consistent.

• The false positives might be solved by ABIChecker: obsoleted/unavailable ABI-public decls #33694.
  The code change simply moves an if statement in the [ModuleAnalyzerNodes.cpp] file.
  (I closed that PR because it also involves updating the baseline.)

Hm, I could be misunderstanding, but is ignoring unavailable APIs a good idea on the checkingABI() path? Unavailable (but public or @usableFromInline) declarations still generate exported symbols, and we definitely do want the ABI checker to raise the alarm if they ever get removed or modified.

(Heh, of course I was misunderstanding -- I somehow reversed the polarity of the diff while reading it. Yep, that change looks like the right move! One could even argue that unavailable-but-public declarations shouldn't be ignored even in source compat mode -- client modules are able to refer to them in declarations that are themselves unavailable:)

// module A
@available(*, unavailable)
public func secret() { print("Psst") }

// module B
import A
@available(*, unavailable)
func sneaky() {
  secret() // OK, results in strong linkage to A.secret() even though it's unavailable
}

False positives aren't that bad in this case -- it's usually easy enough to review them & add them to these exception lists. It's far more important that we don't have any false negatives; the consequences of shipping hidden ABI breaks would be terrible. So we need the ABI checker to have excellent sensitivity, but (within reason) we don't care that much about specificity.

[lorentey]

Weirdly, #40609 indicates that the -without-asserts tests are in fact running in the current configuration, despite array_cow_checks not being enabled, and the related symbols not getting emitted into the Swift module.

I don't know what's going on; perhaps it's a lit parsing issue with the original REQUIRES line below?

REQUIRES: swift_stdlib_asserts, array_cow_checks

That space after the comma is unusual.

[benrimmington]

Hm, I could be misunderstanding, but is ignoring unavailable APIs a good idea on the checkingABI() path?

No, it's not a good idea, but that's what is currently happening AFAIK.

The proposed change would only ignore obsoleted/unavailable declarations on the non-checkingABI() path.
(They are currently ignored on all paths.)

[benrimmington]

Yep, that change looks like the right move!

I can open another PR, but it's difficult for others to review, even if the baseline is changed over several commits.
Therefore, a trusted member of the project should make the changes.

That space after the comma is unusual.

Is the double-ampersand in REQUIRES: OS=macosx && CPU=x86_64 valid?
The other tests have two separate requirements.

If the baseline is only for macOS and x86_64, there's also an issue that Float16 isn't checked.

[lorentey]

Is the double-ampersand in REQUIRES: OS=macosx && CPU=x86_64 valid?
The other tests have two separate requirements.

Lit supports both variants, a && b and a, b -- and as far as I can tell they both seem to be working as expected.

I suspect array_cow_checks somehow gets turned on without lit reporting it at the start of testing; however, local testing seems to indicate otherwise. It's probably a really silly oversight somewhere, but I have no idea where just yet.

[lorentey]

Ah, the mystery is solved -- and it indeed was a very silly oversight. I accidentally committed a change to #40609 that turns array_cow_checks from a REQUIRED to an UNSUPPORTED feature. 🤦‍♂️ Builds of other PRs still have both tests disabled, as expected:

UNSUPPORTED: Swift(macosx-x86_64) :: api-digester/stability-stdlib-abi-with-asserts.test (6471 of 15284)
UNSUPPORTED: Swift(macosx-x86_64) :: api-digester/stability-stdlib-abi-without-asserts.test (6472 of 15284)

So all is well, this PR looks like the right move; the real problem was between my chair and keyboard, as usual. 🙈