Skip to content

Commit

Permalink
Simplify the thunk names we generate for test functions. (#750)
Browse files Browse the repository at this point in the history
Parameterized test functions with the same name but different argument
types currently get the same generated names from swift-syntax, so we do
some additional decorating to ensure uniqueness. This results in very
long symbol names that `swift-demangle` has trouble with.

This PR changes the decorating formula. Previously, we would include a
copy of the test function's signature (stripped of whitespace, Unicode,
and non-identifier-friendly ASCII) and, if the identifier contained
non-ASCII characters, the CRC32 of the identifier for further
uniqueness. This change drops the copy of the identifier name and always
includes the CRC32. Collisions are only possible for fully-qualified
test function names that are _identical_, and I naïvely judge the risk
of those collisions to be sufficiently low that we can make this change.

For `ZipTests.allElementsEqual😀(i:j:)` we currently generate a thunk
named:

```
$s12TestingTests03ZipB0V0022allElementsEqual_oxFJo4TestfMp_43funcallElementsEqual__i_Int_j_Int__5bcb7b35fMu_
```

This change would change it to:

```
$s12TestingTests03ZipB0V0022allElementsEqual_oxFJo4TestfMp_9Z5bcb7b35fMu_
```

Which demangles to:

```
$s12TestingTests03ZipB0V0022allElementsEqual_oxFJo4TestfMp_9Z5bcb7b35fMu_ ---> unique name #1 of Z5bcb7b35 in peer macro @test expansion #1 of allElementsEqual😀 in TestingTests.ZipTests
```

The benefit of the change is that the generated names are shorter and
easier to read when expanding a macro, and play better with the
demangler. There may also be some benefit on Windows where the linker
has a 65KB symbol name cap, although we're not exporting these symbols
so probably not.

### Checklist:

- [x] Code and documentation should follow the style of the [Style
Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md).
- [x] If public symbols are renamed or modified, DocC references should
be updated.
  • Loading branch information
grynspan authored Oct 7, 2024
1 parent 987fed7 commit 7dd3d27
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 50 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -62,44 +62,18 @@ extension MacroExpansionContext {
.tokens(viewMode: .fixedUp)
.map(\.textWithoutBackticks)
.joined()
let crcValue = crc32(identifierCharacters.utf8)
let suffix = String(crcValue, radix: 16, uppercase: false)

// Strip out any characters in the function's signature that won't play well
// in a generated symbol name.
let identifier = String(
identifierCharacters.map { character in
if character.isLetter || character.isWholeNumber {
return character
}
return "_"
}
)

// If there is a non-ASCII character in the identifier, we might be
// stripping it out above because we are only looking for letters and
// digits. If so, add in a hash of the identifier to improve entropy and
// reduce the risk of a collision.
//
// For example, the following function names will produce identical unique
// names without this mutation:
//
// @Test(arguments: [0]) func A(🙃: Int) {}
// @Test(arguments: [0]) func A(🙂: Int) {}
//
// Note the check here is not the same as the one above: punctuation like
// "(" should be replaced, but should not cause a hash to be emitted since
// it does not contribute any entropy to the makeUniqueName() algorithm.
//
// The intent here is not to produce a cryptographically strong hash, but to
// disambiguate between superficially similar function names. A collision
// may still occur, but we only need it to be _unlikely_. CRC-32 is good
// enough for our purposes.
if !identifierCharacters.allSatisfy(\.isASCII) {
let crcValue = crc32(identifierCharacters.utf8)
let suffix = String(crcValue, radix: 16, uppercase: false)
return makeUniqueName("\(prefix)\(identifier)_\(suffix)")
// If the caller did not specify a prefix and the CRC32 value starts with a
// digit, include a single-character prefix to ensure that Swift's name
// demangling still works correctly.
var prefix = prefix
if prefix.isEmpty, let firstSuffixCharacter = suffix.first, firstSuffixCharacter.isWholeNumber {
prefix = "Z"
}

return makeUniqueName("\(prefix)\(identifier)")
return makeUniqueName("\(prefix)\(suffix)")
}
}

Expand Down
22 changes: 7 additions & 15 deletions Tests/TestingMacrosTests/UniqueIdentifierTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -27,50 +27,35 @@ struct UniqueIdentifierTests {
return BasicMacroExpansionContext().makeUniqueName(thunking: functionDecl).text
}

@Test("Thunk identifiers contain a function's name")
func thunkNameContainsFunctionName() throws {
let uniqueName = try makeUniqueName("func someDistinctFunctionName() async throws")
#expect(uniqueName.contains("someDistinctFunctionName"))
}

@Test("Thunk identifiers do not contain backticks")
func noBackticks() throws {
let uniqueName = try makeUniqueName("func `someDistinctFunctionName`() async throws")
#expect(uniqueName.contains("someDistinctFunctionName"))

#expect(!uniqueName.contains("`"))
}

@Test("Thunk identifiers do not contain arbitrary Unicode")
func noArbitraryUnicode() throws {
let uniqueName = try makeUniqueName("func someDistinctFunction🌮Name🐔() async throws")
#expect(uniqueName.contains("someDistinctFunction"))

#expect(!uniqueName.contains("🌮"))
#expect(!uniqueName.contains("🐔"))
#expect(uniqueName.contains("Name"))
}

@Test("Argument types influence generated identifiers")
func argumentTypes() throws {
let uniqueNameWithInt = try makeUniqueName("func someDistinctFunctionName(i: Int) async throws")
#expect(uniqueNameWithInt.contains("someDistinctFunctionName"))
let uniqueNameWithUInt = try makeUniqueName("func someDistinctFunctionName(i: UInt) async throws")
#expect(uniqueNameWithUInt.contains("someDistinctFunctionName"))

#expect(uniqueNameWithInt != uniqueNameWithUInt)
}

@Test("Effects influence generated identifiers")
func effects() throws {
let uniqueName = try makeUniqueName("func someDistinctFunctionName()")
#expect(uniqueName.contains("someDistinctFunctionName"))
let uniqueNameAsync = try makeUniqueName("func someDistinctFunctionName() async")
#expect(uniqueNameAsync.contains("someDistinctFunctionName"))
let uniqueNameThrows = try makeUniqueName("func someDistinctFunctionName() throws")
#expect(uniqueNameThrows.contains("someDistinctFunctionName"))
let uniqueNameAsyncThrows = try makeUniqueName("func someDistinctFunctionName() async throws")
#expect(uniqueNameAsyncThrows.contains("someDistinctFunctionName"))

#expect(uniqueName != uniqueNameAsync)
#expect(uniqueName != uniqueNameThrows)
Expand All @@ -89,4 +74,11 @@ struct UniqueIdentifierTests {
#expect(uniqueName1 != uniqueName3)
#expect(uniqueName2 != uniqueName3)
}

@Test("Body does not influence generated identifiers")
func body() throws {
let uniqueName1 = try makeUniqueName("func f() { abc() }")
let uniqueName2 = try makeUniqueName("func f() { def() }")
#expect(uniqueName1 == uniqueName2)
}
}

0 comments on commit 7dd3d27

Please sign in to comment.