Skip to content

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

Merged
stmontgomery merged 7 commits into
swiftlang:mainfrom
stmontgomery:empty-display-string
Aug 13, 2025
Merged

Emit a diagnostic if a display name string is empty#1256
stmontgomery merged 7 commits into
swiftlang:mainfrom
stmontgomery:empty-display-string

Conversation

@stmontgomery

@stmontgomery stmontgomery commented Aug 8, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

stmontgomery commented Aug 8, 2025

Copy link
Copy Markdown
Contributor Author

@swift-ci please test

stmontgomery
stmontgomery commented Aug 8, 2025
View reviewed changes
Comment thread Sources/TestingMacros/Support/DiagnosticMessage.swift Outdated
@stmontgomery

stmontgomery commented Aug 8, 2025

Copy link
Copy Markdown
Contributor Author

@swift-ci please test Linux

1 similar comment
@stmontgomery

stmontgomery commented Aug 8, 2025

Copy link
Copy Markdown
Contributor Author

@swift-ci please test Linux

@grynspan

grynspan commented Aug 8, 2025

Copy link
Copy Markdown
Contributor

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.

grynspan
grynspan requested changes Aug 8, 2025
View reviewed changes
Comment thread Sources/TestingMacros/Support/Additions/StringLiteralExprSyntaxAdditions.swift Outdated
@stmontgomery

stmontgomery commented Aug 9, 2025

Copy link
Copy Markdown
Contributor Author

@swift-ci please test

@stmontgomery

stmontgomery commented Aug 10, 2025

Copy link
Copy Markdown
Contributor Author

@swift-ci please test Linux

grynspan
grynspan reviewed Aug 11, 2025
View reviewed changes
Comment thread Sources/TestingMacros/Support/DiagnosticMessage.swift
@stmontgomery

stmontgomery commented Aug 12, 2025

Copy link
Copy Markdown
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

stmontgomery commented Aug 12, 2025

Copy link
Copy Markdown
Contributor Author

@swift-ci test

@stmontgomery

stmontgomery commented Aug 12, 2025

Copy link
Copy Markdown
Contributor Author

@swift-ci test

@stmontgomery stmontgomery requested a review from grynspan August 12, 2025 16:44
@stmontgomery

stmontgomery commented Aug 12, 2025

Copy link
Copy Markdown
Contributor Author

@swift-ci test Windows

grynspan
grynspan reviewed Aug 12, 2025
View reviewed changes
Comment thread Sources/TestingMacros/Support/DiagnosticMessage.swift Outdated
grynspan
grynspan reviewed Aug 12, 2025
View reviewed changes
Comment thread Sources/TestingMacros/Support/DiagnosticMessage.swift Outdated
@stmontgomery

stmontgomery commented Aug 12, 2025

Copy link
Copy Markdown
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

stmontgomery commented Aug 13, 2025

Copy link
Copy Markdown
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
@jerryjrchen
Test that the fixups are generated if display name is empty
afb7a48 
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 jerryjrchen mentioned this pull request Aug 14, 2025
Merged
2 tasks
jerryjrchen added a commit that referenced this pull request Aug 14, 2025
@jerryjrchen
Test that the fixups are generated if display name is empty (#1267)
9faef9b 
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

Reviewers

@grynspan grynspan grynspan approved these changes

@briancroom briancroom Awaiting requested review from briancroom briancroom is a code owner

@suzannaratcliff suzannaratcliff Awaiting requested review from suzannaratcliff

@jerryjrchen jerryjrchen Awaiting requested review from jerryjrchen jerryjrchen is a code owner

Assignees

@stmontgomery stmontgomery

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

Milestone

Swift 6.3.0

Development

Successfully merging this pull request may close these issues.

2 participants

@stmontgomery @grynspan