Skip to content

Emit a diagnostic if a display name string is empty #1256

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

Merged
merged 7 commits into from
Aug 13, 2025

Conversation

stmontgomery
Copy link
Contributor

@stmontgomery stmontgomery commented Aug 8, 2025

This introduces an error diagnostic emitted from the testing library's @Test and @Suite macros if the display name string literal is empty. For example:

@Test("") // 🛑 Attribute 'Test' specifies an empty display name for this function (from macro 'Test')
func example() {
  ...
}

Fixes rdar://157603034

Motivation:

It's not recommended for the display name string to be empty since it can cause confusing test results.

Checklist:

  • Code and documentation should follow the style of the Style Guide.
  • If public symbols are renamed or modified, DocC references should be updated.

@stmontgomery stmontgomery added this to the Swift 6.x (main) milestone Aug 8, 2025
@stmontgomery stmontgomery self-assigned this Aug 8, 2025
@stmontgomery stmontgomery added enhancement New feature or request traits Issues and PRs related to the trait subsystem or built-in traits macros 🔭 Related to Swift macros such as @Test or #expect raw-identifiers 🧅 Support for raw identifiers labels Aug 8, 2025
@stmontgomery
Copy link
Contributor Author

@swift-ci please test

@stmontgomery
Copy link
Contributor Author

@swift-ci please test Linux

1 similar comment
@stmontgomery
Copy link
Contributor Author

@swift-ci please test Linux

@grynspan
Copy link
Contributor

grynspan commented Aug 8, 2025

Should we give it a fix-it to remove the display name? Also, have you read the compiler diagnostic style guide? It might help you find the right voice for the message.

@stmontgomery
Copy link
Contributor Author

@swift-ci please test

@stmontgomery
Copy link
Contributor Author

@swift-ci please test Linux

@stmontgomery
Copy link
Contributor Author

Should we give it a fix-it to remove the display name?

Added a fix-it to remove the display name argument!

Also, have you read the compiler diagnostic style guide? It might help you find the right voice for the message.

I've now read the guide. Among its recommendations I saw:

…a particular diagnostic should be a warning if the intent of the code is clear and it won't immediately result in a crash.

Based on that, I feel like the severity of this should be a warning and not an error. In terms of the phrasing of the diagnostic message I'm still open to suggestions. I didn't see much in the style guide that would contradict my current phrasing but I could also imagine saying "Display name string is empty" rather than saying it "should not" be empty.

@stmontgomery
Copy link
Contributor Author

@swift-ci test

@stmontgomery
Copy link
Contributor Author

@swift-ci test

@stmontgomery stmontgomery requested a review from grynspan August 12, 2025 16:44
@stmontgomery
Copy link
Contributor Author

@swift-ci test Windows

@stmontgomery
Copy link
Contributor Author

@swift-ci test

@stmontgomery stmontgomery requested a review from grynspan August 13, 2025 21:22
@stmontgomery stmontgomery changed the title Emit a warning if a display name string is empty Emit a diagnostic if a display name string is empty Aug 13, 2025
@stmontgomery
Copy link
Contributor Author

@swift-ci test

@stmontgomery stmontgomery merged commit 75fde1d into swiftlang:main Aug 13, 2025
3 checks passed
@stmontgomery stmontgomery deleted the empty-display-string branch August 13, 2025 22:55
jerryjrchen added a commit to jerryjrchen/swift-testing that referenced this pull request Aug 14, 2025
No functional change: just adds new test cases for the change in swiftlang#1256

* Moved the tests previously added to `apiMisuseErrors` ->
  `apiMisuseErrorsIncludingFixIts`, and include the expected fixits

  It would probably be ok to leave them in `apiMisuseErrors` as well,
  but I think it would be kinda redundant since we also test the
  expected message along with the fixits already.
jerryjrchen added a commit that referenced this pull request Aug 14, 2025
No functional change: adds new test cases for the change in #1256

Moved the tests previously added to `apiMisuseErrors` ->
`apiMisuseErrorsIncludingFixIts`, and include the expected fixits

It would probably be ok to leave them in `apiMisuseErrors` as well, but
I think it would be kinda redundant since we also test the expected
message along with the fixits already.

### 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request macros 🔭 Related to Swift macros such as @Test or #expect raw-identifiers 🧅 Support for raw identifiers traits Issues and PRs related to the trait subsystem or built-in traits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants