From 3ea1c31640d77318b951e099912b12bb33e5f0b4 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Tue, 20 Aug 2024 11:56:23 -0400 Subject: [PATCH] Warn when passing a non-optional value to `try #require(T?)`. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR adds an overload of `try #require()` that warns the developer if they pass a non-optional, non-`Bool` value For example, this code: ```swift let x = 0 let y = try #require(x) ``` Will produce the diagnostic: > ⚠️ '#require(_:_:)' is redundant because 'x' never equals 'nil' --- .../Expectations/Expectation+Macro.swift | 28 +++++++++++++++++++ Sources/TestingMacros/ConditionMacro.swift | 20 +++++++++++++ .../Support/DiagnosticMessage.swift | 20 +++++++++++++ Sources/TestingMacros/TestingMacrosMain.swift | 1 + .../ConditionMacroTests.swift | 13 +++++++++ .../TestSupport/Parse.swift | 1 + Tests/TestingTests/BacktraceTests.swift | 4 +-- Tests/TestingTests/IssueTests.swift | 2 +- Tests/TestingTests/MiscellaneousTests.swift | 10 +++---- 9 files changed, 90 insertions(+), 9 deletions(-) diff --git a/Sources/Testing/Expectations/Expectation+Macro.swift b/Sources/Testing/Expectations/Expectation+Macro.swift index dd4b8875d..65c78495c 100644 --- a/Sources/Testing/Expectations/Expectation+Macro.swift +++ b/Sources/Testing/Expectations/Expectation+Macro.swift @@ -101,6 +101,34 @@ public macro require( sourceLocation: SourceLocation = #_sourceLocation ) -> Bool = #externalMacro(module: "TestingMacros", type: "AmbiguousRequireMacro") +/// Unwrap an optional value or, if it is `nil`, fail and throw an error. +/// +/// - Parameters: +/// - optionalValue: The optional value to be unwrapped. +/// - comment: A comment describing the expectation. +/// - sourceLocation: The source location to which recorded expectations and +/// issues should be attributed. +/// +/// - Returns: The unwrapped value of `optionalValue`. +/// +/// - Throws: An instance of ``ExpectationFailedError`` if `optionalValue` is +/// `nil`. +/// +/// If `optionalValue` is `nil`, an ``Issue`` is recorded for the test that is +/// running in the current task and an instance of ``ExpectationFailedError`` is +/// thrown. +/// +/// This overload of ``require(_:_:sourceLocation:)-6w9oo`` is used when a +/// non-optional, non-`Bool` value is passed to `#require()`. It emits a warning +/// diagnostic indicating that the expectation is redundant. +@freestanding(expression) +@_documentation(visibility: private) +public macro require( + _ optionalValue: T, + _ comment: @autoclosure () -> Comment? = nil, + sourceLocation: SourceLocation = #_sourceLocation +) -> T = #externalMacro(module: "TestingMacros", type: "NonOptionalRequireMacro") + // MARK: - Matching errors by type /// Check that an expression always throws an error of a given type. diff --git a/Sources/TestingMacros/ConditionMacro.swift b/Sources/TestingMacros/ConditionMacro.swift index 690489702..ad1e9ffd3 100644 --- a/Sources/TestingMacros/ConditionMacro.swift +++ b/Sources/TestingMacros/ConditionMacro.swift @@ -293,6 +293,26 @@ public struct AmbiguousRequireMacro: RefinedConditionMacro { } } +/// A type describing the expansion of the `#require()` macro when it is passed +/// a non-optional, non-`Bool` value. +/// +/// This type is otherwise exactly equivalent to ``RequireMacro``. +public struct NonOptionalRequireMacro: RefinedConditionMacro { + public typealias Base = RequireMacro + + public static func expansion( + of macro: some FreestandingMacroExpansionSyntax, + in context: some MacroExpansionContext + ) throws -> ExprSyntax { + if let argument = macro.arguments.first { + context.diagnose(.nonOptionalRequireIsRedundant(argument.expression, in: macro)) + } + + // Perform the normal macro expansion for #require(). + return try RequireMacro.expansion(of: macro, in: context) + } +} + // MARK: - /// A syntax visitor that looks for uses of `#expect()` and `#require()` nested diff --git a/Sources/TestingMacros/Support/DiagnosticMessage.swift b/Sources/TestingMacros/Support/DiagnosticMessage.swift index 2096068d5..8fa2b5519 100644 --- a/Sources/TestingMacros/Support/DiagnosticMessage.swift +++ b/Sources/TestingMacros/Support/DiagnosticMessage.swift @@ -670,6 +670,26 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage { ) } + /// Create a diagnostic messages stating that the expression passed to + /// `#require()` is not optional and the macro is redundant. + /// + /// - Parameters: + /// - expr: The non-optional expression. + /// + /// - Returns: A diagnostic message. + static func nonOptionalRequireIsRedundant(_ expr: ExprSyntax, in macro: some FreestandingMacroExpansionSyntax) -> Self { + // We do not provide fix-its because we cannot see the leading "try" keyword + // so we can't provide a valid fix-it to remove the macro either. We can + // provide a fix-it to add "as Optional", but only providing that fix-it may + // confuse or mislead developers (and that's presumably usually the *wrong* + // fix-it to select anyway.) + Self( + syntax: Syntax(expr), + message: "\(_macroName(macro)) is redundant because '\(expr.trimmed)' never equals 'nil'", + severity: .warning + ) + } + /// Create a diagnostic message stating that a condition macro nested inside /// an exit test will not record any diagnostics. /// diff --git a/Sources/TestingMacros/TestingMacrosMain.swift b/Sources/TestingMacros/TestingMacrosMain.swift index 1bad7bc8b..8603a2031 100644 --- a/Sources/TestingMacros/TestingMacrosMain.swift +++ b/Sources/TestingMacros/TestingMacrosMain.swift @@ -23,6 +23,7 @@ struct TestingMacrosMain: CompilerPlugin { ExpectMacro.self, RequireMacro.self, AmbiguousRequireMacro.self, + NonOptionalRequireMacro.self, ExitTestExpectMacro.self, ExitTestRequireMacro.self, TagMacro.self, diff --git a/Tests/TestingMacrosTests/ConditionMacroTests.swift b/Tests/TestingMacrosTests/ConditionMacroTests.swift index 4bdbd88bf..5f4869055 100644 --- a/Tests/TestingMacrosTests/ConditionMacroTests.swift +++ b/Tests/TestingMacrosTests/ConditionMacroTests.swift @@ -331,6 +331,19 @@ struct ConditionMacroTests { #expect(diagnostics.isEmpty) } + @Test("#require(non-optional value) produces a diagnostic", + arguments: [ + "#requireNonOptional(expression)", + ] + ) + func requireNonOptionalProducesDiagnostic(input: String) throws { + let (_, diagnostics) = try parse(input) + + let diagnostic = try #require(diagnostics.first) + #expect(diagnostic.diagMessage.severity == .warning) + #expect(diagnostic.message.contains("is redundant")) + } + #if !SWT_NO_EXIT_TESTS @Test("Expectation inside an exit test diagnoses", arguments: [ diff --git a/Tests/TestingMacrosTests/TestSupport/Parse.swift b/Tests/TestingMacrosTests/TestSupport/Parse.swift index 734c39fc0..4fcfb22c3 100644 --- a/Tests/TestingMacrosTests/TestSupport/Parse.swift +++ b/Tests/TestingMacrosTests/TestSupport/Parse.swift @@ -22,6 +22,7 @@ fileprivate let allMacros: [String: any Macro.Type] = [ "expect": ExpectMacro.self, "require": RequireMacro.self, "requireAmbiguous": AmbiguousRequireMacro.self, // different name needed only for unit testing + "requireNonOptional": NonOptionalRequireMacro.self, // different name needed only for unit testing "expectExitTest": ExitTestRequireMacro.self, // different name needed only for unit testing "requireExitTest": ExitTestRequireMacro.self, // different name needed only for unit testing "Suite": SuiteDeclarationMacro.self, diff --git a/Tests/TestingTests/BacktraceTests.swift b/Tests/TestingTests/BacktraceTests.swift index 522865e30..f5a2c497c 100644 --- a/Tests/TestingTests/BacktraceTests.swift +++ b/Tests/TestingTests/BacktraceTests.swift @@ -34,8 +34,8 @@ struct BacktraceTests { } @Test("Backtrace.current() is populated") - func currentBacktrace() throws { - let backtrace = try #require(Backtrace.current()) + func currentBacktrace() { + let backtrace = Backtrace.current() #expect(!backtrace.addresses.isEmpty) } diff --git a/Tests/TestingTests/IssueTests.swift b/Tests/TestingTests/IssueTests.swift index fbb9ce28c..e0618e344 100644 --- a/Tests/TestingTests/IssueTests.swift +++ b/Tests/TestingTests/IssueTests.swift @@ -113,7 +113,7 @@ final class IssueTests: XCTestCase { await Test { let x: String? = nil - _ = try #require(x ?? "hello") + _ = try #require(x ?? ("hello" as String?)) }.run(configuration: configuration) } diff --git a/Tests/TestingTests/MiscellaneousTests.swift b/Tests/TestingTests/MiscellaneousTests.swift index f77b698d6..1a6130cd1 100644 --- a/Tests/TestingTests/MiscellaneousTests.swift +++ b/Tests/TestingTests/MiscellaneousTests.swift @@ -369,9 +369,8 @@ struct MiscellaneousTests { #expect(firstParameter.index == 0) #expect(firstParameter.firstName == "i") #expect(firstParameter.secondName == nil) - let firstParameterTypeInfo = try #require(firstParameter.typeInfo) - #expect(firstParameterTypeInfo.fullyQualifiedName == "Swift.Int") - #expect(firstParameterTypeInfo.unqualifiedName == "Int") + #expect(firstParameter.typeInfo.fullyQualifiedName == "Swift.Int") + #expect(firstParameter.typeInfo.unqualifiedName == "Int") } catch {} do { @@ -386,9 +385,8 @@ struct MiscellaneousTests { #expect(secondParameter.index == 1) #expect(secondParameter.firstName == "j") #expect(secondParameter.secondName == "k") - let secondParameterTypeInfo = try #require(secondParameter.typeInfo) - #expect(secondParameterTypeInfo.fullyQualifiedName == "Swift.String") - #expect(secondParameterTypeInfo.unqualifiedName == "String") + #expect(secondParameter.typeInfo.fullyQualifiedName == "Swift.String") + #expect(secondParameter.typeInfo.unqualifiedName == "String") } catch {} }