Skip to content

Commit 4c85b1a

Browse files
authored
Require that .bug() identifiers be parseable as URLs. (#347)
This PR constrains the valid values of `.bug()` identifiers. They must now be valid URLs or, as a fallback, valid unsigned integers. Arbitrary strings were never intended to be supported. ### 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.
1 parent 3811d5b commit 4c85b1a

File tree

8 files changed

+230
-63
lines changed

8 files changed

+230
-63
lines changed

Sources/Testing/Support/Environment.swift

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,9 @@ enum Environment {
9999
}
100100
}
101101
return result
102+
#else
103+
#warning("Platform-specific implementation missing: environment variables unavailable")
104+
return [:]
102105
#endif
103106
}
104107

Sources/Testing/Testing.docc/AssociatingBugs.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,9 @@ func engineWorks() async {
3939
```
4040

4141
The bug identifier can be specified as an integer or as a string; if it is
42-
specified as a string and matches certain formats, the testing library is able
43-
to infer additional information about it. For more information on the formats
44-
recognized by the testing library, see <doc:BugIdentifiers>.
42+
specified as a string, it must be parseable as an unsigned integer or as a URL.
43+
For more information on the formats recognized by the testing library, see
44+
<doc:BugIdentifiers>.
4545

4646
## Specifying the relationship between a bug and a test
4747

Sources/Testing/Testing.docc/BugIdentifiers.md

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -28,30 +28,35 @@ formats are associated with some common bug-tracking systems.
2828
- If the bug identifier can be parsed as a URL according to
2929
[RFC 3986](https://www.ietf.org/rfc/rfc3986.txt), it is assumed to represent
3030
an issue tracked at that URL.
31-
- All other bug identifiers, including those specified as integers rather than
32-
strings, are assumed to refer to bugs in an unspecified bug-tracking system.
31+
- If the bug identifier begins with `"FB"` and the rest of it can be parsed as
32+
an unsigned integer, it is assumed to represent a bug filed with the
33+
[Apple Feedback Assistant](https://feedbackassistant.apple.com).
34+
- If the bug identifier can be parsed as an unsigned integer, it is assumed to
35+
represent an issue with that numeric identifier in an unspecified bug-tracking
36+
system.
37+
- All other bug identifiers are considered invalid and will cause the compiler
38+
to generate an error at compile time.
3339

3440
<!--
3541
Possible additional formats we could recognize (which would require special
3642
handling to detect:
3743
38-
- If the bug identifier begins with `"FB"`, it is assumed to represent a bug
39-
filed with the [Apple Feedback Assistant](https://feedbackassistant.apple.com).
4044
- If the bug identifier begins with `"#"` and can be parsed as a positive
4145
integer, it is assumed to represent a [GitHub](https://github.com) issue in
4246
the same repository as the test.
4347
-->
4448

4549
## Examples
4650

47-
| Trait Function | Inferred Bug-Tracking System |
48-
|-|-|
49-
| `.bug(12345)` | None |
50-
| `.bug("12345")` | None |
51-
| `.bug("rdar:12345")` | Apple Radar |
52-
| `.bug("https://github.com/apple/swift/pull/12345")` | [GitHub Issues for the Swift project](https://github.com/apple/swift/issues) |
53-
| `.bug("https://bugs.webkit.org/show_bug.cgi?id=12345")` | [WebKit Bugzilla](https://bugs.webkit.org/) |
51+
| Trait Function | Valid | Inferred Bug-Tracking System |
52+
|-|:-:|-|
53+
| `.bug(12345)` | Yes | None |
54+
| `.bug("12345")` | Yes | None |
55+
| `.bug("Things don't work")` | **No** | None |
56+
| `.bug("rdar:12345")` | Yes | Apple Radar |
57+
| `.bug("https://github.com/apple/swift/pull/12345")` | Yes | [GitHub Issues for the Swift project](https://github.com/apple/swift/issues) |
58+
| `.bug("https://bugs.webkit.org/show_bug.cgi?id=12345")` | Yes | [WebKit Bugzilla](https://bugs.webkit.org/) |
59+
| `.bug("FB12345")` | Yes | Apple Feedback Assistant | <!-- SEE ALSO: rdar://104582015 -->
5460
<!--
55-
| `.bug("FB12345")` | Apple Feedback Assistant | // SEE ALSO: rdar://104582015
56-
| `.bug("#12345")` | GitHub Issues for the current repository (if hosted there) |
61+
| `.bug("#12345")` | Yes | GitHub Issues for the current repository (if hosted there) |
5762
-->
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
//
2+
// This source file is part of the Swift.org open source project
3+
//
4+
// Copyright (c) 2024 Apple Inc. and the Swift project authors
5+
// Licensed under Apache License v2.0 with Runtime Library Exception
6+
//
7+
// See https://swift.org/LICENSE.txt for license information
8+
// See https://swift.org/CONTRIBUTORS.txt for Swift project authors
9+
//
10+
11+
#if compiler(>=5.11)
12+
import SwiftSyntax
13+
#else
14+
public import SwiftSyntax
15+
#endif
16+
17+
extension EditorPlaceholderExprSyntax {
18+
/// Initialize an instance of this type with the given placeholder string.
19+
///
20+
/// - Parameters:
21+
/// - placeholder: The placeholder string, not including surrounding angle
22+
/// brackets or pound characters.
23+
init(_ placeholder: String) {
24+
self.init(placeholder: .identifier("<# \(placeholder) #" + ">"))
25+
}
26+
}

Sources/TestingMacros/Support/AttributeDiscovery.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ struct AttributeInfo {
154154
if let declaration = declaration.asProtocol((any WithAttributesSyntax).self) {
155155
traits += createAvailabilityTraitExprs(for: declaration, in: context)
156156
}
157-
diagnoseIssuesWithTags(in: traits, addedTo: attribute, in: context)
157+
diagnoseIssuesWithTraits(in: traits, addedTo: attribute, in: context)
158158

159159
// Use the start of the test attribute's name as the canonical source
160160
// location of the test.

Sources/TestingMacros/Support/DiagnosticMessage+Diagnosing.swift

Lines changed: 109 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,10 @@ public import SwiftSyntaxMacros
2323
/// - traitExprs: An array of trait expressions to examine.
2424
/// - attribute: The `@Test` or `@Suite` attribute.
2525
/// - context: The macro context in which the expression is being parsed.
26-
func diagnoseIssuesWithTags(in traitExprs: [ExprSyntax], addedTo attribute: AttributeSyntax, in context: some MacroExpansionContext) {
27-
// Find tags that are in an unsupported format (only .member and "literal"
28-
// are allowed.)
26+
func diagnoseIssuesWithTraits(in traitExprs: [ExprSyntax], addedTo attribute: AttributeSyntax, in context: some MacroExpansionContext) {
2927
for traitExpr in traitExprs {
30-
// At this time, we are only looking for .tags() traits in this function.
28+
// At this time, we are only looking for .tags() and .bug() traits in this
29+
// function.
3130
guard let functionCallExpr = traitExpr.as(FunctionCallExprSyntax.self),
3231
let calledExpr = functionCallExpr.calledExpression.as(MemberAccessExprSyntax.self) else {
3332
continue
@@ -36,58 +35,122 @@ func diagnoseIssuesWithTags(in traitExprs: [ExprSyntax], addedTo attribute: Attr
3635
// Check for .tags() traits.
3736
switch calledExpr.tokens(viewMode: .fixedUp).map(\.textWithoutBackticks).joined() {
3837
case ".tags", "Tag.List.tags", "Testing.Tag.List.tags":
39-
for tagExpr in functionCallExpr.arguments.lazy.map(\.expression) {
40-
if tagExpr.is(StringLiteralExprSyntax.self) {
41-
// String literals are supported tags.
42-
} else if let tagExpr = tagExpr.as(MemberAccessExprSyntax.self) {
43-
let joinedTokens = tagExpr.tokens(viewMode: .fixedUp).map(\.textWithoutBackticks).joined()
44-
if joinedTokens.hasPrefix(".") || joinedTokens.hasPrefix("Tag.") || joinedTokens.hasPrefix("Testing.Tag.") {
45-
// These prefixes are all allowed as they specify a member access
46-
// into the Tag type.
47-
} else {
48-
context.diagnose(.tagExprNotSupported(tagExpr, in: attribute))
49-
continue
50-
}
38+
_diagnoseIssuesWithTagsTrait(functionCallExpr, addedTo: attribute, in: context)
39+
case ".bug", "Bug.bug", "Testing.Bug.bug":
40+
_diagnoseIssuesWithBugTrait(functionCallExpr, addedTo: attribute, in: context)
41+
default:
42+
// This is not a trait we can parse.
43+
break
44+
}
45+
}
46+
}
5147

52-
// Walk all base expressions and make sure they are exclusively member
53-
// access expressions.
54-
func checkForValidDeclReferenceExpr(_ declReferenceExpr: DeclReferenceExprSyntax) {
55-
// This is the name of a type or symbol. If there are argument names
56-
// (unexpected in this context), it's a function reference and is
57-
// unsupported.
58-
if declReferenceExpr.argumentNames != nil {
59-
context.diagnose(.tagExprNotSupported(tagExpr, in: attribute))
60-
}
61-
}
62-
func checkForValidBaseExpr(_ baseExpr: ExprSyntax) {
63-
if let baseExpr = baseExpr.as(MemberAccessExprSyntax.self) {
64-
checkForValidDeclReferenceExpr(baseExpr.declName)
65-
if let baseBaseExpr = baseExpr.base {
66-
checkForValidBaseExpr(baseBaseExpr)
67-
}
68-
} else if let baseExpr = baseExpr.as(DeclReferenceExprSyntax.self) {
69-
checkForValidDeclReferenceExpr(baseExpr)
70-
} else {
71-
// The base expression was some other kind of expression and is
72-
// not supported.
73-
context.diagnose(.tagExprNotSupported(tagExpr, in: attribute))
74-
}
75-
}
76-
if let baseExpr = tagExpr.base {
77-
checkForValidBaseExpr(baseExpr)
48+
/// Diagnose issues with a `.tags()` trait in a parsed attribute.
49+
///
50+
/// - Parameters:
51+
/// - traitExpr: The `.tags()` expression.
52+
/// - attribute: The `@Test` or `@Suite` attribute.
53+
/// - context: The macro context in which the expression is being parsed.
54+
private func _diagnoseIssuesWithTagsTrait(_ traitExpr: FunctionCallExprSyntax, addedTo attribute: AttributeSyntax, in context: some MacroExpansionContext) {
55+
// Find tags that are in an unsupported format (only .member and "literal"
56+
// are allowed.)
57+
for tagExpr in traitExpr.arguments.lazy.map(\.expression) {
58+
if tagExpr.is(StringLiteralExprSyntax.self) {
59+
// String literals are supported tags.
60+
} else if let tagExpr = tagExpr.as(MemberAccessExprSyntax.self) {
61+
let joinedTokens = tagExpr.tokens(viewMode: .fixedUp).map(\.textWithoutBackticks).joined()
62+
if joinedTokens.hasPrefix(".") || joinedTokens.hasPrefix("Tag.") || joinedTokens.hasPrefix("Testing.Tag.") {
63+
// These prefixes are all allowed as they specify a member access
64+
// into the Tag type.
65+
} else {
66+
context.diagnose(.tagExprNotSupported(tagExpr, in: attribute))
67+
continue
68+
}
69+
70+
// Walk all base expressions and make sure they are exclusively member
71+
// access expressions.
72+
func checkForValidDeclReferenceExpr(_ declReferenceExpr: DeclReferenceExprSyntax) {
73+
// This is the name of a type or symbol. If there are argument names
74+
// (unexpected in this context), it's a function reference and is
75+
// unsupported.
76+
if declReferenceExpr.argumentNames != nil {
77+
context.diagnose(.tagExprNotSupported(tagExpr, in: attribute))
78+
}
79+
}
80+
func checkForValidBaseExpr(_ baseExpr: ExprSyntax) {
81+
if let baseExpr = baseExpr.as(MemberAccessExprSyntax.self) {
82+
checkForValidDeclReferenceExpr(baseExpr.declName)
83+
if let baseBaseExpr = baseExpr.base {
84+
checkForValidBaseExpr(baseBaseExpr)
7885
}
86+
} else if let baseExpr = baseExpr.as(DeclReferenceExprSyntax.self) {
87+
checkForValidDeclReferenceExpr(baseExpr)
7988
} else {
80-
// This tag is not of a supported expression type.
89+
// The base expression was some other kind of expression and is
90+
// not supported.
8191
context.diagnose(.tagExprNotSupported(tagExpr, in: attribute))
8292
}
8393
}
84-
default:
85-
// This is not a tag list (as far as we know.)
86-
break
94+
if let baseExpr = tagExpr.base {
95+
checkForValidBaseExpr(baseExpr)
96+
}
97+
} else {
98+
// This tag is not of a supported expression type.
99+
context.diagnose(.tagExprNotSupported(tagExpr, in: attribute))
87100
}
88101
}
89102
}
90103

104+
/// Diagnose issues with a `.bug()` trait in a parsed attribute.
105+
///
106+
/// - Parameters:
107+
/// - traitExpr: The `.bug()` expression.
108+
/// - attribute: The `@Test` or `@Suite` attribute.
109+
/// - context: The macro context in which the expression is being parsed.
110+
private func _diagnoseIssuesWithBugTrait(_ traitExpr: FunctionCallExprSyntax, addedTo attribute: AttributeSyntax, in context: some MacroExpansionContext) {
111+
// If the first argument to the .bug() trait is unlabelled and a string
112+
// literal, check that it can be parsed as a URL or at least as an integer.
113+
guard let arg = traitExpr.arguments.first.map(Argument.init),
114+
arg.label == nil,
115+
let stringLiteralExpr = arg.expression.as(StringLiteralExprSyntax.self),
116+
let urlString = stringLiteralExpr.representedLiteralValue else {
117+
return
118+
}
119+
120+
if UInt64(urlString) != nil {
121+
// The entire URL string can be parsed as an integer, so allow it. Although
122+
// the testing library prefers valid URLs here, some bug-tracking systems
123+
// might not provide URLs, or might provide excessively long URLs, so we
124+
// allow numeric identifiers as a fallback.
125+
return
126+
}
127+
128+
if urlString.count > 3 && urlString.starts(with: "FB") && UInt64(urlString.dropFirst(2)) != nil {
129+
// The string appears to be of the form "FBnnn...". Such strings are used by
130+
// Apple to indicate issues filed using Feedback Assistant. Although we
131+
// don't want to special-case every possible bug-tracking system out there,
132+
// Feedback Assistant is very important to Apple so we're making an
133+
// exception for it.
134+
return
135+
}
136+
137+
// We could use libcurl, libxml, or Windows' InternetCrackUrlW() to actually
138+
// parse the string and ensure it is a valid URL, however we could get
139+
// different results on different platforms. See the branch
140+
// jgrynspan/type-check-bug-identifiers-with-libcurl for an implementation.
141+
// Instead, we apply a very basic sniff test above. We intentionally don't
142+
// use a regular expression here.
143+
144+
let isURLStringValid = urlString.allSatisfy(\.isASCII)
145+
&& !urlString.contains(where: \.isWhitespace)
146+
&& urlString.contains(":")
147+
if !isURLStringValid {
148+
context.diagnose(.urlExprNotValid(stringLiteralExpr, in: traitExpr, in: attribute))
149+
}
150+
}
151+
152+
// MARK: -
153+
91154
/// Diagnose issues with a synthesized suite (one without an `@Suite` attribute)
92155
/// containing a declaration.
93156
///

Sources/TestingMacros/Support/DiagnosticMessage.swift

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -556,6 +556,38 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
556556
)
557557
}
558558

559+
/// Create a diagnostic message stating that the URL string passed to a trait
560+
/// is not a valid URL.
561+
///
562+
/// - Parameters:
563+
/// - urlExpr: The unsupported URL string.
564+
/// - traitExpr: The trait expression containing `urlExpr`.
565+
/// - attribute: The `@Test` or `@Suite` attribute.
566+
///
567+
/// - Returns: A diagnostic message.
568+
static func urlExprNotValid(_ urlExpr: StringLiteralExprSyntax, in traitExpr: FunctionCallExprSyntax, in attribute: AttributeSyntax) -> Self {
569+
// We do not currently expect anything other than "[...].bug()" here, so
570+
// force-cast to MemberAccessExprSyntax to get the name of the trait.
571+
let traitName = traitExpr.calledExpression.cast(MemberAccessExprSyntax.self).declName
572+
let urlString = urlExpr.representedLiteralValue!
573+
574+
return Self(
575+
syntax: Syntax(urlExpr),
576+
message: #"URL "\#(urlString)" is invalid and cannot be used with trait '\#(traitName.trimmed)' in attribute \#(_macroName(attribute))"#,
577+
severity: .error,
578+
fixIts: [
579+
FixIt(
580+
message: MacroExpansionFixItMessage(#"Replace "\#(urlString)" with URL"#),
581+
changes: [.replace(oldNode: Syntax(urlExpr), newNode: Syntax(EditorPlaceholderExprSyntax("url")))]
582+
),
583+
FixIt(
584+
message: MacroExpansionFixItMessage("Remove trait '\(traitName.trimmed)'"),
585+
changes: [.replace(oldNode: Syntax(traitExpr), newNode: Syntax("" as ExprSyntax))]
586+
),
587+
]
588+
)
589+
}
590+
559591
/// Create a diagnostic messages stating that the expression passed to
560592
/// `#require()` is ambiguous.
561593
///

Tests/TestingMacrosTests/TestDeclarationMacroTests.swift

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,4 +370,42 @@ struct TestDeclarationMacroTests {
370370
#expect(diagnostic.message == "Tag '\(tagExpr)' cannot be used with attribute 'Test'; pass a member of 'Tag' or a string literal instead")
371371
}
372372
}
373+
374+
@Test("Valid bug identifiers are allowed",
375+
arguments: [
376+
#"@Test(.bug(12345)) func f() {}"#,
377+
#"@Test(.bug("12345")) func f() {}"#,
378+
#"@Test(.bug("mailto:[email protected]")) func f() {}"#,
379+
#"@Test(.bug("rdar:12345")) func f() {}"#,
380+
#"@Test(.bug("rdar://12345")) func f() {}"#,
381+
#"@Test(.bug("FB12345")) func f() {}"#,
382+
#"@Test(.bug("https://github.com/apple/swift-testing/issues/12345")) func f() {}"#,
383+
#"@Test(Bug.bug("https://github.com/apple/swift-testing/issues/12345")) func f() {}"#,
384+
#"@Test(Testing.Bug.bug("https://github.com/apple/swift-testing/issues/12345")) func f() {}"#,
385+
#"@Test(Bug.bug("https://github.com/apple/swift-testing/issues/12345", "here's what happened...")) func f() {}"#,
386+
]
387+
)
388+
func validBugIdentifiers(input: String) throws {
389+
let (_, diagnostics) = try parse(input)
390+
391+
#expect(diagnostics.isEmpty)
392+
}
393+
394+
@Test("Invalid bug identifiers are detected",
395+
arguments: [
396+
"12345 ", "here's what happened...", "🌯", "mailto: [email protected]",
397+
"FB", "FBabc", "FB1",
398+
]
399+
)
400+
func invalidBugIdentifiers(id: String) throws {
401+
let input = #"@Test(.bug("\#(id)")) func f() {}"#
402+
let (_, diagnostics) = try parse(input)
403+
404+
#expect(diagnostics.count > 0)
405+
for diagnostic in diagnostics {
406+
#expect(diagnostic.diagMessage.severity == .error)
407+
#expect(diagnostic.message == #"URL "\#(id)" is invalid and cannot be used with trait 'bug' in attribute 'Test'"#)
408+
}
409+
}
410+
373411
}

0 commit comments

Comments
 (0)