Repository navigation
Conversation
c1d2b1d to
20991fc
Compare
20991fc to
14d2cd8
Compare
14d2cd8 to
a3cdc86
Compare
a3cdc86 to
65001f0
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The shared return-node refactor affects interprocedural flow and type inference, with unresolved return-classification errors requiring correction and human validation.
1 open issue
What changed in this PR
Adds Swift shorthand-return support to Unified’s data-flow analysis and type inference, using QL-side return detection rather than extractor desugaring.
Changes:
- Recognizes implicit returns from single-expression Swift bodies.
- Introduces canonical return nodes shared by data flow and model sinks.
- Adds shorthand-return data-flow and contextual-type regression cases.
| File | Description |
|---|---|
| unified/ql/test/library-tests/type-inference/type-inference.expected | Records shorthand-return type inference results. |
| unified/ql/test/library-tests/type-inference/pattern_matching.swift | Adds contextual typing for a shorthand enum return. |
| unified/ql/test/library-tests/dataflow/test.expected | Records the new shorthand-return flow. |
| unified/ql/test/library-tests/dataflow/short-return.swift | Adds a shorthand-return flow test. |
| unified/ql/lib/codeql/unified/internal/typeinference/TypeInference.qll | Uses returned expressions as inference inputs. |
| unified/ql/lib/codeql/unified/internal/mad/LegacyMaD.qll | Uses canonical return nodes for model sinks. |
| unified/ql/lib/codeql/unified/internal/FacadeAst.qll | Adds getter and setter AST refinements. |
| unified/ql/lib/codeql/unified/internal/ExprPositionsPluginSwift.qll | Implements Swift implicit-return detection. |
| unified/ql/lib/codeql/unified/internal/ExprPositionsPlugin.qll | Adds the language-specific return hook. |
| unified/ql/lib/codeql/unified/internal/ExprPositions.qll | Unifies explicit and implicit return detection. |
| unified/ql/lib/codeql/unified/internal/dataflow/DataFlowNode.qll | Defines canonical return nodes and their metadata. |
| unified/ql/lib/codeql/unified/internal/dataflow/DataFlowInstantiation.qll | Integrates canonical returns and preserves explicit-return path detail. |
| unified/ql/lib/codeql/unified/internal/dataflow/DataFlowGraph.qll | Connects returned values to canonical return nodes. |
Review effort: Max
| callable instanceof FunctionDeclaration and | ||
| exists(Expr ret | ret = callable.getReturnType() | not ret.(Identifier).getValue() = "Void") | ||
| or | ||
| callable instanceof GetterDeclaration | ||
| or | ||
| callable instanceof FunctionExpr // We can't tell whether closures have a return value |
There was a problem hiding this comment.
I think we can accept this. It's a best-effort check and spurious tuples should be harmless in practice.
hvitved
left a comment
There was a problem hiding this comment.
LGTM, except we don't actually need changes in the type inference implementation.
| TLocalVariableRefNode(AstNode repr, LocalVariable var, VariableRefKind kind) { | ||
| performsVariableAccess(repr, var, kind, _) | ||
| } or | ||
| TReturnNode(DataFlowCallable callable) { |
There was a problem hiding this comment.
Do you mind if we keep it as DataFlowCallable for now? Using Callable is equally ugly as we have to exclude callables in type context.
| class Return extends ReturnExpr { | ||
| Expr getExpr() { result = this.getValue() } | ||
| class Return extends Expr { | ||
| Return() { isReturnedExpr(_, this) } |
There was a problem hiding this comment.
This bit is actually not needed, since we have n1 = n2.(Block).getLastStmt() in stepLanguageSpecific which already covers it.
There was a problem hiding this comment.
So type inference thinks the last statement in a functionbody is returned from the function? That's not actually true for Swift.
There was a problem hiding this comment.
Yes, that is how it is implemented right now.
There was a problem hiding this comment.
I think in practice it will be fine, since if the last statement is an expression that is not implicitly returned, then I think it has to be dead code?
There was a problem hiding this comment.
I agree it's probably fine for type inference since it only matters for contextual typing in functions that have a return type. I'd like to be a bit more precise about this in data flow as spurious returns can amplify spurious flow in cases where the call graph contains spurious call edges. Reverting the change in type inference.
Since I was looking into this I also went ahead added data flow out of singleton blocks and conditional expressions (where flow out of blocks can occur).
Implements data flow support for Swift's short-hand returns.
When the body of a function consists of a single expression that resolves to a value, that value is implicitly the return value of the function. For example,
is the same as
This is currently implemented on the QL side as the desugaring rule can't be implemented nicely with Yeast, but we might want to desugar it in the future, when Yeast offers better support for subtype checks.