Atom swap eff documentation

[timmi-on-rails]

Atoms swap implementation uses optimistic locking.
I have tested, that in case of concurrent swaps the effect returned by f is run multiple times.

Assuming this is supposed to happen, I find the summary should be more explicit:
If f may be called multiple times, then the returned effect will be called multiple times as well.
And an effect encapsulates side-effects by definition.
So instead of only hinting that f should be side-effect free, it should tell, that returned side-effects will be run multiple times.

I will contribute a PR, when the issue is clear.

//     Atomically updates the value by passing the old value to `f` and updating the
//     atom with the result. Note: `f` may be called multiple times, so it should be
//     free of side-effects.
public Eff<A> SwapEff(Func<A, Eff<A>> f)

Also, for me as an unexperienced effects user, the current behavior of Atom.SwapEff is not expected.
I would have made the following mistake:

var lastDeletedFile = Atom<Option<string>>(None);

static Eff<Unit> DeleteFile(string filename)
  => lastDeletedFile.SwapEff(_ => Eff(() =>
  {
     File.Delete(filename);  // assume this line throws, if the file does not exist
     return filename;
  })).Map(ignore);

Concurrent calls to DeleteFile will eventually fail, because the file has already been deleted.

Now that I know how Atom.SwapEff works, I would implement it so:

var lastDeletedFile = Atom<Option<string>>(None);

static Eff<Unit> DeleteFile(string filename)
  => Eff(() =>
  {
    File.Delete(filename);
    return unit;
  }).Bind(_ => lastDeletedFile.SwapEff(_ => SuccessEff(filename))).Map(ignore);

Well, actually there is no point in using SwapEff here, so I would simplify to:

var lastDeletedFile = Atom<Option<string>>(None);

static Eff<Unit> DeleteFile(string filename)
  => Eff(() =>
  {
    File.Delete(filename);
    return unit;
  }).Bind(_ => Eff(() => lastDeletedFile.Swap(_ => filename))).Map(ignore);

[louthy]

Assuming this is supposed to happen, I find the summary should be more explicit:

It is explicit throughout the documentatoin: [1] , [2], [3]. The assumption is nobody goes in to using Atom before at least reading up on what it's for. But there are warnings on Atom, in the API reference docs, and in the wiki.

Your point about side-effects in atoms is fair though. Not sure what I was thinking when I did that, I was probably thinking more about trying to have as much of the library work together as possible. I'll leave it for now, because maybe there was a point to this, but it doesn't smell right for sure.

[louthy]

Literally as I posted the above, I remembered. Atoms must, by defintion, have side-effects. They are changing the global state of the world. So, it makes sense that there's a declarative version of them that makes this explicit. They then work like IO Ref in Haskell (which allows for mutable state, but changes are performed within the IO monad). So it makes perfect sense to be within the Eff monad.

Probably warnings need to be more focussed around idempotency and commutability than about not containing side-effects.