Fail closed on conditional SafeDI scanning#297
Open
adam-zethraeus wants to merge 1 commit into
Open
Conversation
SafeDITool previously walked every SwiftSyntax conditional-compilation branch while scanning for SafeDI declarations. That made inactive declarations and members visible to code generation, and it also meant unknown target-specific conditions could silently change the dependency graph without an explicit tool input. Add a conditional-compilation evaluator for the scanner that selects active #if/#elseif/#else clauses, supports boolean literals plus !, &&, ||, and parentheses, and treats custom flags as active only when supplied through the new active-compilation-conditions input. If an unevaluable condition guards a SafeDI declaration or relevant instantiable member, parsing now fails closed with an actionable error instead of guessing. Thread active compilation conditions through scan, generate, inline output-directory scanning, manifests, and scan caches. Cached scan output now records the conditions used to create it and is invalidated when generate runs with a different condition set. Existing manifests remain decodable with an empty condition list by default. Keep macro diagnostics on their previous all-branches syntax walk while making SafeDITool-owned visitors opt into fail-closed evaluation. This avoids making macro validation reject conditionally declared custom mocks while still protecting generation from inactive or unknown scanner branches. Cover the behavior with core visitor tests for inactive branches, active supplied flags, #elseif/operator evaluation, and unevaluable guarded declarations, plus tool-level tests for failing on unknown guarded roots and generating when the active condition is supplied. Verification performed before this amend: swift test --traits sourceBuild, ./CLI/lint.sh, and git diff --check HEAD~1 HEAD.
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (98.81%) is below the target coverage (100.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
===========================================
- Coverage 100.00% 98.81% -1.19%
===========================================
Files 41 43 +2
Lines 7040 7332 +292
===========================================
+ Hits 7040 7245 +205
- Misses 0 87 +87
🚀 New features to boost your workflow:
|
dfed
reviewed
May 27, 2026
Owner
dfed
left a comment
dfed
left a comment
There was a problem hiding this comment.
Very nifty! I've wanted this for awhile, but haven't carved out the time to get it over the line. Very much a fan of this capability. There are a few things we'll need before this gets approved (beyond handling the inline feedback):
- patch test coverage must be at 100%. the test suite should prove that this works.
- The SafeDITool plugins (SPM + Xcode, as well as the
safedi.bzland TuistSafeDI.swiftplugins) should be updated to gather this information and pass it to the tool commands. This capability is not terribly helpful unless the vended plugins actually pass this data into the into the tool. - ideally we'd take a
mockOnly: truein our example projects and put it behind#if DEBUGto prove the system works end-to-end.
Happy to answer any questions you have. Also happy to chat over in iOS Folks Slack if that's easier.
Comment on lines
+25
to
+34
| private enum CodingKeys: String, CodingKey { | ||
| case dependencyTreeGeneration | ||
| case mockGeneration | ||
| case configurationFilePaths | ||
| case mockConfigurationOutputFilePath | ||
| case additionalMocksToGenerate | ||
| case additionalInputFiles | ||
| case activeCompilationConditions | ||
| } | ||
|
|
Owner
There was a problem hiding this comment.
we can remove. build artifacts are hermetic per SafeDI version. no need for custom decoding. breaking changes to the format are fine.
Suggested change
| private enum CodingKeys: String, CodingKey { | |
| case dependencyTreeGeneration | |
| case mockGeneration | |
| case configurationFilePaths | |
| case mockConfigurationOutputFilePath | |
| case additionalMocksToGenerate | |
| case additionalInputFiles | |
| case activeCompilationConditions | |
| } |
Comment on lines
+97
to
+109
| } | ||
|
|
||
| public init(from decoder: Decoder) throws { | ||
| let container = try decoder.container(keyedBy: CodingKeys.self) | ||
| self.init( | ||
| dependencyTreeGeneration: try container.decode([InputOutputMap].self, forKey: .dependencyTreeGeneration), | ||
| mockGeneration: try container.decodeIfPresent([InputOutputMap].self, forKey: .mockGeneration) ?? [], | ||
| configurationFilePaths: try container.decodeIfPresent([String].self, forKey: .configurationFilePaths) ?? [], | ||
| mockConfigurationOutputFilePath: try container.decodeIfPresent(String.self, forKey: .mockConfigurationOutputFilePath), | ||
| additionalMocksToGenerate: try container.decodeIfPresent([String].self, forKey: .additionalMocksToGenerate) ?? [], | ||
| additionalInputFiles: try container.decodeIfPresent([String].self, forKey: .additionalInputFiles) ?? [], | ||
| activeCompilationConditions: try container.decodeIfPresent([String].self, forKey: .activeCompilationConditions) ?? [], | ||
| ) |
Owner
There was a problem hiding this comment.
same as above
Suggested change
| } | |
| public init(from decoder: Decoder) throws { | |
| let container = try decoder.container(keyedBy: CodingKeys.self) | |
| self.init( | |
| dependencyTreeGeneration: try container.decode([InputOutputMap].self, forKey: .dependencyTreeGeneration), | |
| mockGeneration: try container.decodeIfPresent([InputOutputMap].self, forKey: .mockGeneration) ?? [], | |
| configurationFilePaths: try container.decodeIfPresent([String].self, forKey: .configurationFilePaths) ?? [], | |
| mockConfigurationOutputFilePath: try container.decodeIfPresent(String.self, forKey: .mockConfigurationOutputFilePath), | |
| additionalMocksToGenerate: try container.decodeIfPresent([String].self, forKey: .additionalMocksToGenerate) ?? [], | |
| additionalInputFiles: try container.decodeIfPresent([String].self, forKey: .additionalInputFiles) ?? [], | |
| activeCompilationConditions: try container.decodeIfPresent([String].self, forKey: .activeCompilationConditions) ?? [], | |
| ) |
Comment on lines
+29
to
+42
| for clause in node.clauses { | ||
| guard let condition = clause.condition else { | ||
| return .active(clause) | ||
| } | ||
|
|
||
| switch evaluate(condition) { | ||
| case let .known(isActive): | ||
| if isActive { | ||
| return .active(clause) | ||
| } | ||
| case let .unknown(error): | ||
| return .unevaluable(error) | ||
| } | ||
| } |
Owner
There was a problem hiding this comment.
can we use a first(where: clause to avoid early returns from if and switch statements? we only early return from guard in this repo
Comment on lines
+47
to
+97
| if let boolLiteral = BooleanLiteralExprSyntax(condition) { | ||
| return .known(boolLiteral.literal.tokenKind == .keyword(.true)) | ||
| } | ||
|
|
||
| if let identifier = DeclReferenceExprSyntax(condition) { | ||
| let name = identifier.baseName.text | ||
| if activeCompilationConditions.contains(name) { | ||
| return .known(true) | ||
| } | ||
| // Missing custom flags are unknown, not false. The scanner can be | ||
| // invoked outside the compiler command that ultimately defines `-D` | ||
| // flags, so silently treating an absent flag as inactive would hide | ||
| // guarded SafeDI declarations from generation. | ||
| return .unknown(.init(condition: name, suggestedActiveCondition: name)) | ||
| } | ||
|
|
||
| if let prefixOperator = PrefixOperatorExprSyntax(condition) { | ||
| guard prefixOperator.operator.text == "!" else { | ||
| return .unknown(.init(condition: condition.trimmedDescription)) | ||
| } | ||
| return evaluate(prefixOperator.expression).negated | ||
| } | ||
|
|
||
| if let sequence = SequenceExprSyntax(condition) { | ||
| return evaluateSequence(Array(sequence.elements), fallbackDescription: condition.trimmedDescription) | ||
| } | ||
|
|
||
| if let infixOperator = InfixOperatorExprSyntax(condition), | ||
| let binaryOperator = BinaryOperatorExprSyntax(infixOperator.operator) | ||
| { | ||
| let left = evaluate(infixOperator.leftOperand) | ||
| let right = evaluate(infixOperator.rightOperand) | ||
| switch binaryOperator.operator.text { | ||
| case "&&": | ||
| return left.and(right) | ||
| case "||": | ||
| return left.or(right) | ||
| default: | ||
| return .unknown(.init(condition: condition.trimmedDescription)) | ||
| } | ||
| } | ||
|
|
||
| if let tuple = TupleExprSyntax(condition), | ||
| tuple.elements.count == 1, | ||
| let element = tuple.elements.first | ||
| { | ||
| return evaluate(element.expression) | ||
| } | ||
|
|
||
| return .unknown(.init(condition: condition.trimmedDescription)) | ||
| } |
Owner
There was a problem hiding this comment.
free-handed this so may not be right (hopefully tests will catch if I screwed up!), but let's restructure to avoid early returns:
Suggested change
| if let boolLiteral = BooleanLiteralExprSyntax(condition) { | |
| return .known(boolLiteral.literal.tokenKind == .keyword(.true)) | |
| } | |
| if let identifier = DeclReferenceExprSyntax(condition) { | |
| let name = identifier.baseName.text | |
| if activeCompilationConditions.contains(name) { | |
| return .known(true) | |
| } | |
| // Missing custom flags are unknown, not false. The scanner can be | |
| // invoked outside the compiler command that ultimately defines `-D` | |
| // flags, so silently treating an absent flag as inactive would hide | |
| // guarded SafeDI declarations from generation. | |
| return .unknown(.init(condition: name, suggestedActiveCondition: name)) | |
| } | |
| if let prefixOperator = PrefixOperatorExprSyntax(condition) { | |
| guard prefixOperator.operator.text == "!" else { | |
| return .unknown(.init(condition: condition.trimmedDescription)) | |
| } | |
| return evaluate(prefixOperator.expression).negated | |
| } | |
| if let sequence = SequenceExprSyntax(condition) { | |
| return evaluateSequence(Array(sequence.elements), fallbackDescription: condition.trimmedDescription) | |
| } | |
| if let infixOperator = InfixOperatorExprSyntax(condition), | |
| let binaryOperator = BinaryOperatorExprSyntax(infixOperator.operator) | |
| { | |
| let left = evaluate(infixOperator.leftOperand) | |
| let right = evaluate(infixOperator.rightOperand) | |
| switch binaryOperator.operator.text { | |
| case "&&": | |
| return left.and(right) | |
| case "||": | |
| return left.or(right) | |
| default: | |
| return .unknown(.init(condition: condition.trimmedDescription)) | |
| } | |
| } | |
| if let tuple = TupleExprSyntax(condition), | |
| tuple.elements.count == 1, | |
| let element = tuple.elements.first | |
| { | |
| return evaluate(element.expression) | |
| } | |
| return .unknown(.init(condition: condition.trimmedDescription)) | |
| } | |
| if let boolLiteral = BooleanLiteralExprSyntax(condition) { | |
| return .known(boolLiteral.literal.tokenKind == .keyword(.true)) | |
| } else if let identifier = DeclReferenceExprSyntax(condition) { | |
| let name = identifier.baseName.text | |
| if activeCompilationConditions.contains(name) { | |
| return .known(true) | |
| } else { | |
| // Missing custom flags are unknown, not false. The scanner can be | |
| // invoked outside the compiler command that ultimately defines `-D` | |
| // flags, so silently treating an absent flag as inactive would hide | |
| // guarded SafeDI declarations from generation. | |
| return .unknown(.init(condition: name, suggestedActiveCondition: name)) | |
| } | |
| } else if let prefixOperator = PrefixOperatorExprSyntax(condition) { | |
| if prefixOperator.operator.text == "!" { | |
| return evaluate(prefixOperator.expression).negated | |
| } else { | |
| return .unknown(.init(condition: condition.trimmedDescription)) | |
| } | |
| } else if let sequence = SequenceExprSyntax(condition) { | |
| return evaluateSequence(Array(sequence.elements), fallbackDescription: condition.trimmedDescription) | |
| } else if let infixOperator = InfixOperatorExprSyntax(condition), | |
| let binaryOperator = BinaryOperatorExprSyntax(infixOperator.operator) | |
| { | |
| let left = evaluate(infixOperator.leftOperand) | |
| let right = evaluate(infixOperator.rightOperand) | |
| switch binaryOperator.operator.text { | |
| case "&&": | |
| return left.and(right) | |
| case "||": | |
| return left.or(right) | |
| default: | |
| return .unknown(.init(condition: condition.trimmedDescription)) | |
| } | |
| } else if let tuple = TupleExprSyntax(condition), | |
| tuple.elements.count == 1, | |
| let element = tuple.elements.first | |
| { | |
| return evaluate(element.expression) | |
| } else { | |
| return .unknown(.init(condition: condition.trimmedDescription)) | |
| } | |
| } |
|
|
||
| enum ConditionalCompilationClauseSelection { | ||
| case active(IfConfigClauseSyntax?) | ||
| case unevaluable(UnevaluableConditionalCompilationConditionError) |
Owner
There was a problem hiding this comment.
lazy q: is this actually reachable? and do we have tests that cover it?
Comment on lines
+188
to
+199
| func or(_ other: Self) -> Self { | ||
| switch (self, other) { | ||
| case (.known(true), _), (_, .known(true)): | ||
| .known(true) | ||
| case (.known(false), let other): | ||
| other | ||
| case (let selfEvaluation, .known(false)): | ||
| selfEvaluation | ||
| case (.unknown, _): | ||
| self | ||
| } | ||
| } |
Comment on lines
+166
to
+198
| var negated: Self { | ||
| switch self { | ||
| case let .known(value): | ||
| .known(!value) | ||
| case .unknown: | ||
| self | ||
| } | ||
| } | ||
|
|
||
| func and(_ other: Self) -> Self { | ||
| switch (self, other) { | ||
| case (.known(false), _), (_, .known(false)): | ||
| .known(false) | ||
| case (.known(true), let other): | ||
| other | ||
| case (let selfEvaluation, .known(true)): | ||
| selfEvaluation | ||
| case (.unknown, _): | ||
| self | ||
| } | ||
| } | ||
|
|
||
| func or(_ other: Self) -> Self { | ||
| switch (self, other) { | ||
| case (.known(true), _), (_, .known(true)): | ||
| .known(true) | ||
| case (.known(false), let other): | ||
| other | ||
| case (let selfEvaluation, .known(false)): | ||
| selfEvaluation | ||
| case (.unknown, _): | ||
| self | ||
| } |
Owner
There was a problem hiding this comment.
this is inspired. I really like how you structured this!
Comment on lines
+256
to
+257
| } | ||
| return containsSafeDIDeclaration ? .skipChildren : .visitChildren |
Owner
There was a problem hiding this comment.
here and below let's add else branches:
Suggested change
| } | |
| return containsSafeDIDeclaration ? .skipChildren : .visitChildren | |
| } else { | |
| return containsSafeDIDeclaration ? .skipChildren : .visitChildren | |
| } |
|
|
||
| var containsInstantiableBodySyntax = false | ||
|
|
||
| private let customMockName: String? |
Owner
There was a problem hiding this comment.
private below internal please per repo convention
Comment on lines
+202
to
+217
| extension SyntaxVisitor { | ||
| func walkIfConfigElements(_ elements: IfConfigClauseSyntax.Elements) { | ||
| switch elements { | ||
| case let .statements(statements): | ||
| walk(statements) | ||
| case let .switchCases(switchCases): | ||
| walk(switchCases) | ||
| case let .decls(decls): | ||
| walk(decls) | ||
| case let .postfixExpression(expression): | ||
| walk(expression) | ||
| case let .attributes(attributes): | ||
| walk(attributes) | ||
| } | ||
| } | ||
| } |
Owner
There was a problem hiding this comment.
help me understand why we need this? can't we just visitChildren rather than explicitly walking the elements within the ifConfig manually?
Author
|
Thanks for the feedback! |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
SafeDITool walks every branch of a SwiftSyntax conditional-compilation while scanning for SafeDI declarations — potentially leaving inactive declarations and members visible to code generation and allowing unknown target-specific conditions to silently affect the dependency graph.
This PR closes the loophole by adding a conditional-compilation evaluator for the scanner. It selects active #if/#elseif/#else clauses, supports boolean literals plus !, &&, ||, and parentheses, and treats custom flags as active only when supplied through the new active-compilation-conditions input.
If an unevaluable condition guards a SafeDI declaration or relevant instantiable member, parsing fails closed with an actionable error.
Disclosure:
Thanks!