Disallow test/suite declarations in protocols. #333
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change is similar to #332 except that the diagnostic is applied when a test or suite is declared inside a protocol rather than inside a non-final class. Without this change, the compiler will fail to build a test target with such a test _anyway_, but the diagnostics produced will refer to the emitted macro expansion and are opaque. For example, given the declaration: ```swift protocol P { @test func f() } ``` The compiler produces these diagnostics which don't really explain the problem very well: > 🛑 'private' modifier cannot be used in protocols > 🛑 Protocol methods must not have bodies > 🛑 Type '$s12TestingTests1PP1f4TestfMp_37__🟠$test_container__function__funcf__fMu_' cannot be nested in protocol 'P' > 🛑 Use of protocol 'P' as a type must be written 'any P' > 🛑 Cannot find '$s12TestingTests1PP1f4TestfMp_7funcf__fMu0_' in scope This is because the swift-testing macro target is unaware that the enclosing lexical context is a protocol, so it tries to emit code that would only make sense inside a concrete type. With this change, you instead get: > 🛑 Attribute 'Test' cannot be applied to a function within protocol 'P' I also took the liberty of improving the diagnostic emitted if a test/suite is declared inside a closure. > [!NOTE] > As with #332, this change only takes effect when swift-syntax-600 is used.
grynspan
requested review from
stmontgomery,
dennisweissmann,
briancroom,
SeanROlszewski and
suzannaratcliff
as code owners
|
@swift-ci please test |
briancroom
approved these changes
Apr 6, 2024
dennisweissmann
approved these changes
Apr 6, 2024
grynspan
added a commit
that referenced
this pull request
Apr 6, 2024
This PR uses the swift-syntax-600 `lexicalContext` property to enforce the compile-time requirement that tags declared with `@Tag` must be declared in an extension to `Tag` or in a type declared inside `Tag`. Fix-its are provided for a few scenarios. The new diagnostics are: ```swift @tag var x: Tag // 🛑 Attribute 'Tag' cannot be applied to a global variable // Declare in an extension to 'Tag' [Fix] // Remove attribute 'Tag' [Fix] extension String { @tag var x: Tag // 🛑 Attribute 'Tag' cannot be applied to a property except in an extension to 'Tag' } extension Tag { @tag var x: Tag // 🛑 Attribute 'Tag' cannot be applied to an instance property // Add 'static' [Fix] } extension Tag { @tag var x: String // 🛑 Attribute 'Tag' cannot be applied to a property of type 'String' // Change type to 'Tag' [Fix] // Remove attribute 'Tag' [Fix] } ``` As with #332 and #333, these problems are only diagnosed when using swift-syntax-600.
2 tasks
grynspan
added a commit
that referenced
this pull request
Apr 8, 2024
This PR uses the swift-syntax-600 `lexicalContext` property to enforce the compile-time requirement that tags declared with `@Tag` must be declared in an extension to `Tag` or in a type declared inside `Tag`. Fix-its are provided for a few scenarios. The new diagnostics are: ```swift @tag var x: Tag // 🛑 Attribute 'Tag' cannot be applied to a global variable // Declare in an extension to 'Tag' [Fix] // Remove attribute 'Tag' [Fix] extension String { @tag var x: Tag // 🛑 Attribute 'Tag' cannot be applied to a property except in an extension to 'Tag' } extension Tag { @tag var x: Tag // 🛑 Attribute 'Tag' cannot be applied to an instance property // Add 'static' [Fix] } extension Tag { @tag var x: String // 🛑 Attribute 'Tag' cannot be applied to a property of type 'String' // Change type to 'Tag' [Fix] // Remove attribute 'Tag' [Fix] } ``` As with #332 and #333, these problems are only diagnosed when using swift-syntax-600. ### 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.
grynspan
added a commit
that referenced
this pull request
Apr 8, 2024
This PR applies all suite diagnostics\* to every decl group (declaring type or
extension) in the lexical context during `@Suite` and `@Test` expansion. For
example:
```swift
struct S1<T> {
@suite struct S2 { // 🛑 Attribute 'Suite' cannot be applied to a generic structure
@test func f() {} // 🛑 Attribute 'Test' cannot be applied to a generic function
}
}
```
This PR is follow-up to #332 and #333.
\* Except for diagnostics specific to the `@Suite` attribute itself.
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change is similar to #332 except that the diagnostic is applied when a test or suite is declared inside a protocol rather than inside a non-final class.
Without this change, the compiler will fail to build a test target with such a test anyway, but the diagnostics produced will refer to the emitted macro expansion and are opaque. For example, given the declaration:
The compiler produces these diagnostics which don't really explain the problem very well:
This is because the swift-testing macro target is unaware that the enclosing lexical context is a protocol, so it tries to emit code that would only make sense inside a concrete type. With this change, you instead get:
I also took the liberty of improving the diagnostic emitted if a test/suite is declared inside a closure.
Note
As with #332, this change only takes effect when swift-syntax-600 is used.
Checklist: