flow icon indicating copy to clipboard operation
flow copied to clipboard

FLIP for purity/mutability analysis and restriction in function conditions

Open dsainati1 opened this issue 2 years ago • 9 comments

Implementation of https://github.com/onflow/cadence/issues/1806 (further discussion can be found there).

Description

This FLIP proposes to add a new syntax and semantic analysis to determine which functions in Cadence have side effects, and which are pure. This analysis would allow us to then restrict or limit the contexts in which side effects can occur (see the "Motivation" section of the FLIP for a non-exhaustive list of issues and features where this would be useful).

A work-in-progress implementation of this FLIP for demonstration and proof-of-concept purposes can be found here: https://github.com/onflow/cadence/pull/1818


For contributor use:

  • [X] Targeted PR against master branch
  • [X] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • [X] Updated relevant documentation
  • [X] Re-reviewed Files changed in the Github PR explorer
  • [X] Added appropriate labels

dsainati1 avatar Jul 15 '22 19:07 dsainati1

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
flow-docs ✅ Ready (Inspect) Visit Preview Aug 16, 2022 at 6:21PM (UTC)

vercel[bot] avatar Jul 15 '22 19:07 vercel[bot]

I like this FLIP!

bjartek avatar Jul 21 '22 22:07 bjartek

I second @SupunS 's proposal, I have really big doubts about the gain for developer here.

As an average developer what I will gain from this ?

Cannot this be implemented invisible to user with runtime error ? I don't see any mention of that

All of the issues in the Motivation section do have alternative solutions; if we elected not to proceed with this proposal we could pursue those instead.

I think that section is for explaining those alternatives, not confirming of their existence.

I suggest, if we can do purity analysis ( we should be able to as we cannot trust developer marking pure) , I prefer runtime error when purity is needed. It will be a rare case, and panic has strong feedback loop when developing. I think this over complicates life of the developer as-is.

Swift takes the opposite approach, having a mutating keyword that functions must use in order to be allowed to mutate program state, essentially implying that functions are pure by default. Given the large amount of existing code in Cadence however, adoption will be easier by default if impurity is the default assumption.

If we will somehow do this, I think Swift has the best approach for user friendliness, it is pretty easy to declare that 'you will modify something here' than declare 'you will not modify something here' .

bluesign avatar Jul 22 '22 08:07 bluesign

As an average developer what I will gain from this ?

This will actually make it simpler, not harder, to think about your code. Reasoning about how and when mutation occurs in code is famously difficult because in most cases it requires knowing exactly how functions are implemented, even those that you didn't write. For a developer to able to look at a function declaration at a glance, see the pure keyword, and know with certainty that calling this function will not have any unexpected side effects they need to account for, will reduce the amount of mental overhead necessary to write composable code.

Cannot this be implemented invisible to user with runtime error ? I don't see any mention of that I suggest, if we can do purity analysis ( we should be able to as we cannot trust developer marking pure) , I prefer runtime error when purity is needed. It will be a rare case, and panic has strong feedback loop when developing. I think this over complicates life of the developer as-is.

I disagree pretty strongly that a runtime error is preferable to a static error in almost all cases. It's better for correctness, as a static error guarantee that a potential mistake is caught, while a runtime error can slip through testing if it only occurs with a specific set of inputs. It's also better for speed and ease of development: iterating on a static error is simple because the type checker will tell you exactly where it occurs and how to fix it, while a runtime error can require hours of debugging to even diagnose the source. A classic example of this tradeoff is in optional types (like Cadence and TypeScript have) where by adding an extra bit of complexity to the language in making nil-able values have a different type to normal values, we can prevent an entire category of runtime errors from even occurring and help developers more easily reason about where nil can occur in their code.

There is also the consideration that adding new runtime errors to existing contracts is significantly harder to deal with for developers. Adding a new static error is simple, because developers can fix the code that is reported as broken and redeploy their code. If we instead add a new runtime error though, developers may not even know that their code's behavior may change in a specific set of circumstances, and only encounter a critical error weeks or months after the Flow upgrade. It is more developer friendly for contracts to break early so they can be fixed in advance of a spork than for contracts that seem to be correct to suddenly break in production.

Swift takes the opposite approach, having a mutating keyword that functions must use in order to be allowed to mutate program state, essentially implying that functions are pure by default. Given the large amount of existing code in Cadence however, adoption will be easier by default if impurity is the default assumption. If we will somehow do this, I think Swift has the best approach for user friendliness, it is pretty easy to declare that 'you will modify something here' than declare 'you will not modify something here' .

I am not against making pure the default and requiring users to declare functions to be mutating; I think you may be right that the end result may be better for the language. My primary concern with making pure be the default, however, is that this will dramatically increase the amount of code that would be broken by this change, which could make it challenging for users to adopt.

dsainati1 avatar Jul 22 '22 14:07 dsainati1

For a developer to able to look at a function declaration at a glance, see the pure keyword, and know with certainty that calling this function will not have any unexpected side effects they need to account for.

Unless the owner of that contract changed that to impure ( which will break all my contract, as checker will fail ) Not broken when it is used. ( runtime error case )

I mean not only my contract, every collection containing a single of my NFT probably, every contract importing my contract.

Adding a new static error is simple, because developers can fix the code that is reported as broken and redeploy their code.

yeah but it will also break running contracts ( like Cadence last breaking change did with optional references )

Oh I mean maybe off-topic, but on optionals subject, please check transactions and contracts on mainnet. You will understand what I mean, proper optional usage is really really low. Panic is everywhere. Composable stuff is not possible at all.

bluesign avatar Jul 22 '22 15:07 bluesign

After some internal discussion, we identified one potential alternative approach to this that would be significantly more lightweight than the idea in the main FLIP.

This version would only be designed to address the function pre/post condition use case, and would feature no inference. Instead, all functions would be impure by default, and only by annotating a function as pure would purity be possible. This would only be required for functions that are designed to be called from pre/post conditions, which is unlikely to be a large number of use cases, and also unlikely to result in long function dependency chains.

This lightweight of a feature would probably not be suitable for any heavy weight use cases like mutability of fields or references, but could be a good solution specifically for the function pre/post condition use case.

dsainati1 avatar Jul 28 '22 18:07 dsainati1

I like this lightweight version more. The biggest concern I had with the original proposal was the UX/composability. But by limiting the purity requirement to only pre/post conditions, and by making it explicit (no inference), I think this would have a minimum (almost zero) impact on UX/composability.

This would only be required for functions that are designed to be called from pre/post conditions, which is unlikely to be a large number of use cases, and also unlikely to result in long function dependency chains.

Fully agree with this.

In future, if we ever decide that purity is needed for other parts of the language (e.g: original proposal), then this would be a good foundation for that.

SupunS avatar Jul 28 '22 20:07 SupunS

I guess the "Breaking Change" label can be removed, as the latest version of the proposal is just an addition?

Or is changing conditions to only allow calls of pure functions part of this proposal? If so, it should be maybe made more explicit, or could be a follow-up proposal.

turbolent avatar Aug 16 '22 17:08 turbolent

I guess the "Breaking Change" label can be removed, as the latest version of the proposal is just an addition?

Or is changing conditions to only allow calls of pure functions part of this proposal? If so, it should be maybe made more explicit, or could be a follow-up proposal.

Yea, with the limited scope of this now it makes sense to roll these two into one.

dsainati1 avatar Aug 16 '22 18:08 dsainati1