Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revisiting "unfold" and renaming it to unfolding #1552

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

samuelchassot
Copy link
Collaborator

Solves #1550

@vkuncak
Copy link
Collaborator

vkuncak commented Aug 11, 2024

@samuelchassot test fails:

[info] - 219: imperative/valid/UnfoldOpaqueMutate  *** FAILED *** (18 milliseconds)

I am now not sure if we want the argument of unfold to be ghost. Maybe unfold should be opaque. In some cases, we may call an executable function and we may want to direct stainless to unfold it. Perhaps the most confusing aspect of unfold is that it's always returning unit. Perhaps we should have unfolding instead which is an identity function that does nothing but makes stainless unfold the call? How does our internal tree look like for unfold? We should:

  • check the semantics as it is implemented
  • confirm if this is what we want
  • document it
  • define unfold stainless library definition accordingly

@samuelchassot
Copy link
Collaborator Author

I am diving into the code and am documenting what is done.
I'll post my documentation here.

@samuelchassot
Copy link
Collaborator Author

samuelchassot commented Aug 12, 2024

Summary

  • unfolding is performed by a specific phase UnfoldOpaque

  • it unfolds FunctionInvocation by transforming it into a tree like:

    {
      assume(opaqueFn(v) == @DropVCs {
        val v: BigInt = v // Bindings arguments -> value in the context
        // BEGIN BODY opaqueFN
        val x: BigInt = v * 2
        x + 2
        // END BODY opaqueFN
      })
      ()
    }
  • This block ends with a Unit value

  • The transformation looks for a function invocation to unfold

    • it can be the argument to unfold directly
    • or in the body of a let binding whose variable is passed to the unfold function
      • IT HAS TO BE DIRECTLY IN THE BODY, if it is in other let bindings, it wouldn’t be unfolded because lifted already → it does not explore the entire tree to find a function invocation
  • If the body of the unfolded function contains another invocation it is NOT unfolded

  • There is a special case in the AntiAliasing phase, to avoid the body of the function

    • Usually, AntiAliasing is transforming a function call to return a tuple (return_value, mutatedVar1, mutatedVar2, ...) and where it is called, the 1st element is extracted and the mutated variables of scope are updated using the other elements.
      In this case, we don’t want this to happen, because ,otherwise, the ImperativeCodeElimination phase would hoist the call to the function and the UnfoldOpaque phase would not be able to see it.

      ⇒ this works only because unfold drops the result!!!

@samuelchassot
Copy link
Collaborator Author

The last point above makes going for a version returning something else than Unit more tricky to handle.
I think we can however change it to accept non-ghost argument.
And we can make it inline. @vkuncak @mario-bucev WDYT?

@samuelchassot
Copy link
Collaborator Author

(I have some examples of the trees transformation if you want to see)

@samuelchassot
Copy link
Collaborator Author

samuelchassot commented Aug 14, 2024

After discussion:

  • Keep argument as non-ghost -> more flexible
  • In the future, update so that unfold returns the return value and not Unit
  • Rename it to unfolding as the call actually happens.

@samuelchassot samuelchassot requested a review from vkuncak August 14, 2024 12:27
@vkuncak
Copy link
Collaborator

vkuncak commented Aug 14, 2024

We should also fix documentation in stainless/core/src/sphinx/library.rst

There is a mention of local inline there in the table. Does this work and how is it different from unfold?

@vkuncak vkuncak changed the title Add ghost annotation to unfold argument, to be able to call it outside of ghost env Revisiting "unfold" and renaming it to unfolding Aug 14, 2024
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