Skip to content

[swiftsrc2cpg] Implement desugaring for optional binding syntax#6006

Open
max-leuthaeuser wants to merge 5 commits into
masterfrom
max/fix-optional-binding-desugaring
Open

[swiftsrc2cpg] Implement desugaring for optional binding syntax#6006
max-leuthaeuser wants to merge 5 commits into
masterfrom
max/fix-optional-binding-desugaring

Conversation

@max-leuthaeuser
Copy link
Copy Markdown
Contributor

@max-leuthaeuser max-leuthaeuser commented May 26, 2026

Add support for optional binding constructs in if/while/guard statements:

If-let and While-let:

  • Single bindings: if let a = foo() { ... }
  • Shorthand syntax: if let a { ... } (checks existing variable)
  • Multiple bindings: if let a = foo(), let b = bar() { ... }
  • Mixed cases: if let a = foo(), let b { ... }
  • Tuple patterns: if let (a, b) = foo() { ... }
  • Mixed simple + tuple: if let a = foo(), let (b, c) = bar() { ... }
  • While loops: while let item = iterator.next() { ... }

Guard-let:

  • Single bindings: guard let a = foo() else { exit }
  • Shorthand syntax: guard let a else { exit } (checks existing variable)
  • Multiple bindings: guard let a = foo(), let b = bar() else { exit }
  • Mixed cases: guard let a = foo(), let b else { exit }
  • Guard with other conditions: guard let a = foo(), flag else { exit }

Desugaring strategy:

  • Condition block: Creates temp variables for simple optional bindings with initializers, evaluates expressions within nil checks with proper short-circuit semantics using && operator
  • Then/body block: Creates properly scoped locals, unwraps temp variables into final bindings
  • Tuple patterns: Excluded from condition desugaring, processed as-is in body block
  • Guard special handling: Implemented in astsForBlockElements (AstCreatorHelper.scala) because guard's then block includes all subsequent statements in the function, not just an explicit block

Desugaring examples:

if let a = foo() { body } desugars to:

let <tmp>0;
if { (<tmp>0 = foo()) != nil } {
  let a = <tmp>0
  body
}

if let a = foo(), let b = bar() { body } desugars to:

let <tmp>0;
let <tmp>1;
if { (<tmp>0 = foo()) != nil && (<tmp>1 = bar()) != nil } {
  let a = <tmp>0
  let b = <tmp>1
  body
}

This ensures proper short-circuit evaluation: bar() is only called if foo() returns non-nil.

if let a { body } desugars to:

if a != nil {
  body
}

guard let a = foo() else { exit } desugars to:

let <tmp>0;
if { (<tmp>0 = foo()) != nil } {
  let a = <tmp>0
  // ... rest of function
} else {
  exit
}

guard let a = foo(), let b else { exit } desugars to:

let <tmp>0;
if { (<tmp>0 = foo()) != nil && b != nil } {
  let a = <tmp>0
  // ... rest of function
} else {
  exit
}

This does no type "unwrapping" (i.e., Optional[T] -> T).

  1. by default (without types from the compiler): we do not have any types there anyway. That would only work if the user code explicitly uses type annotation which mostly just does not happen.
  2. with types from the compiler: we set type fullnames for identifiers and call fullnames for calls anyway (directly from what the compiler gives us). Hence, no need to create a synthetic unwrap call. Imho, that would not bring anything to the table w.r.t. data-flow.

@max-leuthaeuser max-leuthaeuser force-pushed the max/fix-optional-binding-desugaring branch 5 times, most recently from fb6adcf to 1bfc3f1 Compare May 27, 2026 09:34
@max-leuthaeuser max-leuthaeuser force-pushed the max/fix-optional-binding-desugaring branch from 1bfc3f1 to d8c72f5 Compare May 27, 2026 14:07
@max-leuthaeuser max-leuthaeuser requested a review from maltek May 28, 2026 05:16
*
* Condition: { <tmp>0 = foo(); <tmp>0 != nil }
*
* Then block: { let a = <tmp>0; let (b, c) = bar(); body }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is the tuple assignment desugared?

Copy link
Copy Markdown
Contributor Author

@max-leuthaeuser max-leuthaeuser May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not done with full coverage there. That needs an additional PR.
There is special tuple de-sugaring, but only in condition/pattern-matching contexts like if case, guard case, and switch, where tuple subjects get de-sugared into tmp variables plus .0, .1, etc. accesses. That de-sugaring is in astsForBindingTuplePattern / astsForBindingTupleExpr.

For optional tuple pattern bindings here we can reuse it mostly (store in tmp and do the nil comparison; locals and .0, .1, etc. accesses in then block).

@max-leuthaeuser max-leuthaeuser force-pushed the max/fix-optional-binding-desugaring branch from f5e2872 to 0a65640 Compare May 28, 2026 13:23
Copy link
Copy Markdown
Contributor

@maltek maltek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not happy with some of the tests. They are so fuzzy. E.g. AvailabilityQueryTests is using contains and not be empty all over the place instead of complete matches that'd tell us exactly what's there and what isn't there. And some of the queries in there are also structured similarly imprecise - e.g. we know that nilCheck has an assignment argument, but we don't know if it has any other arguments.

(It's not just this test though, some of the others are similar.)

@max-leuthaeuser
Copy link
Copy Markdown
Contributor Author

Ok, I'll overhaul them (tbh they were bad already before this PR).

@max-leuthaeuser
Copy link
Copy Markdown
Contributor Author

max-leuthaeuser commented May 28, 2026

I'm not happy with some of the tests. They are so fuzzy. E.g. AvailabilityQueryTests is using contains and not be empty all over the place

Are we talking about the same file? I can not find any "not be empty" assertion there.
There are a few "shouldBe empty" assertions but that's expected (testing for empty true/false blocks).

Do not yet re-review. I am still bug-hunting a local creation issue. Got it.

val List(condition) = guardIf.condition.l
condition.code shouldBe "i % 2 == 0"

val List(thenPrint, thenAssign) = guardIf.whenTrue.astChildren.isCall.l
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally, I think it's better to write such tests like this instead:

      val List(thenPrint: Call, thenAssign: Call) = guardIf.whenTrue.astChildren.l

That way, any potential children which are not calls aren't swept under the rug.

(Depending on the warning levels of the project, inside might be required.)

Copy link
Copy Markdown
Contributor Author

@max-leuthaeuser max-leuthaeuser May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, warning level do not allow this implicit type narrowing.
Rewriting everything to inside matchers is something thats needs to be addressed separately as the current test style with .isFoo traversals is scattered across the whole test base here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants