Reorganize Swift compiler tests for C/ObjC/C++ interop

With upcoming work on interoperability between Swift and C++, we anticipate that we will need to change a lot of existing interop tests (to validate that C features continue to work when we switch the language mode to C++ in ClangImporter, to separate existing interop tests into pure C tests and tests that depend on Objective-C), and we will add many new tests for C++ interop itself. Therefore, we would like to reorganize the existing interop tests to move closer to achieving the goals we list below.

@scentini has created a pull request that sketched out the idea, but during the code review we understood that a more detailed writeup and discussion is probably needed.

What do people think? @Douglas_Gregor @Slava_Pestov @John_McCall @Michael_Gottesman @codafi

Goals for interop tests

We would like Swift tests for C, Objective-C, and C++ interoperability to be a reliable indicator of interop quality while being easy to work with. Specifically:

  • It should be easy to understand which C, Objective-C, and C++ features work.
  • It should be easy to understand which features are covered by tests.
  • It should be difficult to miss adding a certain kind of tests for a newly implemented interop feature. For example, it should be obvious that it is customary to have tests for the type checker, SILGen, IRGen, and execution tests, but a pull request is only adding type checker tests.
    • Where we currently have such testing gaps, they should be obvious.
  • Tests for a given interop feature should be easy to find.
  • All inputs for a given test should be easy to find.
  • It should be obvious which input files are shared between tests.
  • It should be easy to share input files between different kinds of tests for one interop feature.
  • It should be difficult to accidentally share input files across unrelated tests.
  • Tests should be easy to change. Developers should be able to change tests confidently. Changing one test should not subtly break unrelated tests.

Current difficulties

We believe the way the interop tests are organized right now makes it difficult to achieve the goals listed above.

  • Tests for a given C interop feature (for example, interop with anonymous enums) are located in multiple directories (test/ClangImporter, test/Sema, test/SILGen, test/IRGen, test/Interpreter etc.)
    • These directories contain lots of tests for features other than interop, so finding interop tests is non-trivial. Some interop tests are named starting with objc_, but it is not a hard rule.
    • Multiple tests for the same interop feature are often named differently across multiple directories.
    • Tests for an interop feature that located in different directories don’t reuse input files (for example, C headers).
    • It is difficult to understand where the gaps in testing are, and where coverage is inconsistent.
    • Interop features are often tested inconsistently. For a given interop feature we often have tests for one component of the compiler, but not for other components. It does not immediately look wrong though, because to notice it you have to look in multiple directories.
    • Tests are often organized around how or when the feature was implemented, or how a bug was fixed, not around the aspect being tested.
  • Tests are located in unintuitive places, for example, tests for IRGen for interop exist under both test/ClangImporter and test/IRGen.
  • Test directories contain too many files in a flat list (test/ClangImporter -- 190, test/SILGen -- 419, test/IRGen -- 457).
    • Therefore, finding interop tests (or any other type of tests!) is difficult.
  • Large test directories each have exactly one “Inputs” directory.
    • Most Swift tests don’t use input files, they are contained in one .swift or .sil file. Interop tests are different from most other tests: each interop test uses one or more C header files. Therefore, the issues with input files affect interop tests the most severely.
    • Loosely-related and unrelated interop tests tend to reuse the input files (C headers). Often they use completely disjoint parts of those headers.
    • Due to reuse, headers tend to grow bigger and bigger, as more tests use them and need to add “just one more thing”.
    • The “Inputs” directory is accessible to all tests, making it difficult to understand which tests use which input files.
    • Changing files in the “Inputs” directory is scary because the scope of their reuse is unclear. It is much easier to just append stuff at the bottom.
    • Due to reuse of input files in new and creative ways, their incidental parts over time become load-bearing. This makes input files and tests that depend on them more difficult to change.

Suggested layout for the test directory

All our suggestions are based on the observation that interop is a cross-cutting feature. It is not the case for many other features and aspects of the Swift compiler. Therefore, we think it makes sense to group tests differently.

For features isolated to a specific component, it makes sense to store tests in a directory related to that component. For example, a test that checks that the SIL optimizer transforms a specific code pattern should be located under test/SILOptimizer, because there is very little chance that there are type checker or IRGen concerns with that code pattern.

However, for interop, we think it makes more sense to group the tests around the C, Objective-C, and C++ features. Almost any interop feature has a non-trivial interaction with every compiler component (type checker, SILGen, IRGen, execution), and therefore should be validated at each stage.

Baseline suggestion: organize interop tests by language and feature

test/
  Interop/
    ObjC/
      class/
    Cxx/
      class/
      template/
      enum/
        Inputs/
          shared/
            CoreCooling.h    // A curated header intended for reuse. 
          anonymous-enums.h
          enum-class.h
          module.map
        anonymous-enums-typechecker.swift
        anonymous-enums-print-module-interface.swift
        anonymous-enums-silgen.swift
        anonymous-enums-irgen.sil
        anonymous-enums-execution.swift
        enum-class-typechecker.swift
        enum-class-print-module-interface.swift
        enum-class-...

Note that there is no test/Interop/C directory. From the interop point of view, C is a subset of C++, and all C headers should continue to be importable into Swift once we turn on C++ interop. Furthermore, at some point C++ interop will become on by default, and therefore, there will be no separate C interop as such. Objective-C interop is different because it is only available on select targets.

Advantages:

  • Tests that validate a given interop feature in different compiler components can reuse input files.
  • Reuse of input files is limited to logically related tests, assuming tests don’t reach out into unrelated directories. Tests within a focused directory, like test/Interop/Cxx/enum, can share input files, and it should not cause much trouble because they are related.
  • We can furthermore establish an “Inputs/shared” subdirectory for shared inputs and avoid sharing other inputs as a convention.
  • It is clear which platforms run which tests. All target platforms will run test/Interop/Cxx tests. Only platforms with Objective-C interop will run test/Interop/ObjC tests. We will not need to add “REQUIRES: objc_interop” tags.

Disadvantages:

  • Tests are no longer organized only by compiler component. The only reason we could think of why this is valuable today is described in the section below, along with a suggested alternative workflow ( Composable option: ninja check-swift-<compiler-component>).

Alternative: flatten Interop/ObjC and Interop/Cxx to InteropObjC and InteropCxx

test/
  InteropObjC/
    class/
  InteropCxx/
    enum/
    class/
    template/
      ...

This alternative creates fewer nested directories, but otherwise achieves the same goals, and has the same advantages/disadvantages as the baseline suggestion.

Alternative: group tests only by language feature, not by language

test/
  Interop/
    enum/           // ObjC and C++ enums are similar enough
                    // to go into one directory.
    template/       // Templates are obviously C++-only.
    cxx-class/      // Disambiguate C++ and ObjC classes.
    objc-class/

This alternative has the same advantages/disadvantages as the baseline suggestion, except that we still have to annotate every Objective-C interop test with “REQUIRES: objc_interop”.

Alternative: tests with inputs must be in a separate directory

To further limit unintentional sharing of input files and clarify which input files are used by which test, we could require that tests with inputs must be in a separate directory.

test/
  Interop/
    Cxx/
      SharedInputs/
        CoreCooling.h
      anonymous-enums/
        Inputs/
          anonymous-enums.h
          module.map
        anonymous-enums-typechecker.swift
        anonymous-enums-print-module-interface.swift
        anonymous-enums-silgen.swift
        anonymous-enums-irgen.sil
        anonymous-enums-execution.swift
      enum-class/
        Inputs/
          enum-class.h
          module.map
        enum-class-typechecker.swift
        enum-class-print-module-interface.swift
        enum-class-...

Composable option: test/ClangImporter instead of test/Interop

We could put the proposed directory structure under test/ClangImporter instead of test/Interop. We are suggesting to not do that because “ClangImporter” is a compiler component, and putting non-ClangImporter tests there (for example, type checker tests, SILGen, IRGen tests) can be confusing.

Composable option: ninja check-swift-<compiler-component>

Currently it is possible to run most tests for a given compiler component by pointing lit at a subdirectory like test/IRGen. It is a useful workflow for developers who iterate on the code in a particular component of the compiler. However, this workflow only runs most tests, not all of them; for example, the test/ClangImporter directory also contains IRGen and SILGen tests; execution tests indirectly test most components of the compiler and the standard library, etc. Therefore, this workflow is only useful as a smoke test while rapidly iterating on the code. One still needs to run the whole testsuite before merging any changes.

This proposal will weaken the lit test/IRGen workflow further by moving out interop tests into a different directory. If it is a concern, we could add ninja targets like check-swift-typechecker, check-swift-silgen, check-swift-irgen etc. that run only the tests that are named or tagged accordingly.

This option composes with any alternative described above.

3 Likes

What does it mean to switch the language mode to C++? Do you anticipate that some existing C APIs will import differently in C++ mode?

I mean switching the language mode (AKA dialect, AKA -std=) that we pass to Clang from C or Objective-C to some version of C++ or Objective-C++.

I do expect that some C constructs will produce a different AST when we change the language mode, for example, C structs used to be clang::RecordDecl, but they will become clang::CXXRecordDecl. They used to have no special members (constructors etc.), but they will have those generated implicitly in C++ mode. However, I expect such C constructs to continue to import to Swift just like before (to ensure source stability) -- but hope is not a strategy, hence we want to improve our testing coverage.

Also, some C APIs will absolutely import differently in C++ mode if they haven't taken steps to protect themselves, chiefly by wrapping themselves in extern "C". System headers have almost certainly done set; user library headers have probably done so; bridging headers have almost certainly not done so.

It would be a better world if C++ treated the global namespace as implicitly extern "C", but alas.

2 Likes

As a general matter, moving a bunch of tests around doesn't interest me very much, but if it interests you, feel free.

Splitting tests into their own directories to give them their own Inputs feels like a marginally acceptable solution to a non-problem.

Thanks for the responses! I suggest that we go with the baseline suggestion + flattening Interop/ObjC and Interop/Cxx to InteropObjC and InteropCxx.

test/
  InteropObjC/
    class/
  InteropCxx/
    class/
    template/
    enum/
        Inputs/
          shared/
            CoreCooling.h    // A curated header intended for reuse. 
          anonymous-enums.h
          enum-class.h
          module.map
        anonymous-enums-typechecker.swift
        anonymous-enums-print-module-interface.swift
        anonymous-enums-silgen.swift
        anonymous-enums-irgen.sil
        anonymous-enums-execution.swift
        enum-class-typechecker.swift
        enum-class-print-module-interface.swift
        enum-class-...

Echoing the discussion we had in the linked PR, I worry that the proposed structure is optimizing for a particular kind of compiler development that we don’t actually commit to in reality. Additionally, this breaks the workflows people have come to expect from lit’s (bizarre) directory overlay configuration system. I also don’t understand why ClangImporter isn’t a sufficient umbrella for these tests, regardless of what we choose.

A lot of the hairiness of the current suite is because authors

  1. Piggyback on prior unit tests
    (This has the occasional unfortunate consequence of turning them into ever-broadening integration tests.)
  2. Are installing regression tests for crashes/bugs/behavior observed in components downstream of the Clang Importer
    We shouldn’t ever actually be checking IR in ClangImporter, and if we are that seems... wrong

I agree with the general goal of a holistic feature-oriented approach to getting something as mammoth and byzantine as C++ interop up and running smoothly, I just wonder why we can’t improve the situation we have incrementally instead of folding the structure of the test suite then rebuilding our tooling and workflows on top.

Echoing the discussion we had in the linked PR, I worry that the proposed structure is optimizing for a particular kind of compiler development that we don’t actually commit to in reality.

We currently indeed follow a different organization for tests, and I am suggesting that we change it for interop tests, because doing so has benefits that the first post described ("Goals for interop tests") and because these tests are significantly different from tests for other features.

Additionally, this breaks the workflows people have come to expect from lit’s (bizarre) directory overlay configuration system.

If you are referring to the workflow of running only SILGen/IRGen/etc. tests, then:

(1) Those workflows already don't work reliably for the reasons I explained above (the tests are already disorganized, and you can't reliably run only tests for a certain component, you can only approximate it),

(2) If you make a change to, say, SILGen, you can't avoid running the whole testsuite eventually -- because we have, for example, execution tests, that can break due to a change in SILGen,

(3) We suggested an alternative workflow (ninja check-swift-<compiler-component>), which we can commit to supporting (and doing something meaningful), unlike running lit over a particular directory. We can also extend it to, for example, run the same tests for cross-targets -- something that is very difficult when one tries to run lit directly (one needs to set many command line options to test cross-targets).

I also don’t understand why ClangImporter isn’t a sufficient umbrella for these tests, regardless of what we choose.

ClangImporter is a component that translates Clang ASTs to Swift ASTs (swift.git/lib/ClangImporter). test/ClangImporter is a directory that is supposed to contain tests strictly for the ClangImporter component. Putting the proposed test organization into test/ClangImporter is therefore misleading, because we will have type checker, SILGen, IRGen, execution tests in a directory that is supposed to only test ClangImporter.

If you would be fine with the test organization that we proposed in the first post in this thread, except that "Interop" would be replaced by "ClangImporter", I'd take that. However, you are saying "We shouldn’t ever actually be checking IR in ClangImporter, and if we are that seems... wrong" -- that is exactly the reason why I think that test/ClangImporter is not the right directory for the test organization that we are suggesting.

I agree with the general goal of a holistic feature-oriented approach to getting something as mammoth and byzantine as C++ interop up and running smoothly, I just wonder why we can’t improve the situation we have incrementally instead of folding the structure of the test suite then rebuilding our tooling and workflows on top.

In the post that opens this thread I explained how existing test suite structure makes it difficult to achieve goals for interop tests that we see important ("Goals for interop tests" and "Current difficulties").

We can improve existing tests in place, and things will certainly become better, but I believe we would be still far from achieving the goals we listed above.

Do you have a different suggestion that will allow us to achieve the goals we listed above, at least partially?

1 Like

Dmitri and I spoke about this issue. I'd like to get this down publicly.

In short, the plan is to adopt the proposed baseline with Interop as a high-level component and reserve it for self-contained feature-oriented development as outlined in this proposal. ClangImporter's structure will be retained for integration tests.

This hybrid approach will allow the C++ interoperability folk to maintain an immediate understanding of the feature-completeness of their efforts, while not disrupting the core workflows of most compiler developers. To bridge the gap between the existing test suite's structure and the new nested suite's structure, additional tooling built on top of lit will be investigated so we don't have to add new CMake targets to e.g. run all the IRGen tests. If this becomes a significant issue, we can re-evaluate this decision.

Reserving ClangImporter for more complex integration tests also allows a future place where the importer itself, not just its observable behaviors, can be tested via dedicated infrastructure.

1 Like

This point was implemented in https://github.com/apple/swift/pull/30850.

Terms of Service

Privacy Policy

Cookie Policy