Skip to content

Fixes the free variable problem on closure conversion#299

Merged
rodrigogribeiro merged 9 commits into
mainfrom
closure-conversion-fix
Jan 14, 2026
Merged

Fixes the free variable problem on closure conversion#299
rodrigogribeiro merged 9 commits into
mainfrom
closure-conversion-fix

Conversation

@rodrigogribeiro

Copy link
Copy Markdown
Collaborator
  • Fixes the free variable problem on closure conversion
  • Adds test cases.

@rodrigogribeiro rodrigogribeiro marked this pull request as ready for review January 6, 2026 12:51
@rodrigogribeiro rodrigogribeiro requested a review from mbenke January 6, 2026 12:52

@mbenke mbenke left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Free variable calculation for blocks should be corrected.

Crashing example (tries to add z to the closure in makeClosure; see also comments in code)

function addW (l: word, r: word) -> word {
     let rw : word;
     assembly {
	 rw := add(l,r);
     }
     return rw;
}


contract Bug {
    function main() -> word {
        return makeClosure(42);
    }

    function makeClosure(e : word) -> word {
        let f = lam (x : word) {
	    let y: word;
	    if (false) {
	      let z = 7;
	      y = z;
	    } else {
	      y = e;
	    }
	    return addW(e,x);  // this works
        };
        return f(1);
    }
}


instance Vars a => Vars [a] where
vars = foldr (union . vars) []
free = foldr (union . free) []

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This may be slightly misleading, for a statement list like [let y = 42; return y], the free will include y even though it's bound locally. It is compensated for in closureConversion but I think it merits at least a comment in the class definition.

This approach also may result in incorrect closure - see comment about If statement

Comment thread src/Solcore/Frontend/TypeInference/TcStmt.hs Outdated
Comment thread src/Solcore/Frontend/TypeInference/TcStmt.hs Outdated
Comment thread src/Solcore/Frontend/TypeInference/TcStmt.hs Outdated
free (StmtExp e) = free e
free (Return e) = free e
free (Match e eqns) = free e `union` free eqns
free (If e blk1 blk2) = free e `union` free blk1 `union` free blk2

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For let y:word;if b then { let z = 7; y := z; } else ... we have z in free and not in bound. Then closure conversion tries to close over z.

rodrigogribeiro and others added 4 commits January 10, 2026 09:31
Co-authored-by: Marcin Benke <marcin@benke.org>
Co-authored-by: Marcin Benke <marcin@benke.org>
Co-authored-by: Marcin Benke <marcin@benke.org>

@mbenke mbenke left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good now, thanks!

@rodrigogribeiro rodrigogribeiro merged commit 62432e5 into main Jan 14, 2026
2 checks passed
@rodrigogribeiro rodrigogribeiro deleted the closure-conversion-fix branch January 14, 2026 14:51
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