MVar: add "tryRead" fixes #732

[kubum]

Motivation:

I found an open good first issue #732 while reading about MVar. This branch contains an attempt to implement it.

Modification:

• add tryRead: F[Option[A]] similar to Control-Concurrent-MVar#tryRead
• add a test and a line into the docs

I wonder does it do what is it intended to do? I am happy to amend it if I've missed something, suggestions are welcome. Thank you!

[djspiewak]

The implementation looks fantastic here! Thank you! The problem (and the reason the build failed) is binary compatibility. I should have thought of that earlier, but basically it stems from the fact that MVar is a trait which can be implemented by third-parties. I wonder if there's any way around this?

[rossabaker]

Deprecate and rename MVar? Problem is, in addition to Cats-Effect, it's already familiar as MVar to Haskell and Monix folks. And I don't have a suggestion for a better name.

[kubum]

@djspiewak Thanks for looking into it! I haven't checked the implementation of MVar in Monix. Surprisingly, it already has tryRead implemented.

If we want to leave the current trait untouched, we can create a new trait. Should the interface closely match Control-Concurrent-MVar? If so, we need to add more features such as swap.

What do you think?

[djspiewak]

We could in theory define a new trait, MVar2 or something like that, which has tryRead and swap and other things that the current one is missing and inherit from MVar. This would mean that the current MVar constructors would continue to be binary compatible but would actually produce MVar2 (a subtype). Implementations like Monix would also be able to seamlessly swap to MVar2 without breaking their own compatibility guarantees. In CE3 (when next we break compatibility), we can squish them back together.

I'm not sure if there is another meaningful approach here. The name thing is kind of annoying, and maybe we can come up with something better than MVar2 as a name, but I'm not sure what else to do other than just punting on adding functionality here. But in truth, I don't see how punting on adding the functionality is any better than adding MVar2, since anyone who doesn't need the function will just ignore it and treat it like MVar anyway.

[rossabaker]

💯 to Daniel's last point about MVar2 being no worse. I think we should do it that way and explain ourselves in the MVar doc.

[djspiewak]

I think we should do it that way and explain ourselves in the MVar doc.

Agreed. @kubum would you like to take a crack at restructuring this change to use the MVar2 suggestion? We should probably also take this opportunity to provide the rest of the missing functionality, such as swap.

[kubum]

Agreed. @kubum would you like to take a crack at restructuring this change to use the MVar2 suggestion? We should probably also take this opportunity to provide the rest of the missing functionality, such as swap.

Sounds good to me, thank you! I'll do it and ping you in the discussion for another review if you wouldn't mind.

[djspiewak]

Sounds good to me, thank you! I'll do it and ping you in the discussion for another review if you wouldn't mind.

Sounds perfect!

[kubum]

@djspiewak thanks! I updated the PR. Could let me know is it something you had in mind? I couldn't add @deprecated to MVar, because it is in use when extending MVar2 and it fails the build.

[rossabaker]

The silencer plugin can help with that deprecation issue.

[kubum]

@rossabaker thanks for the link! I wasn't sure if adding a dependency was appropriate. Just pushed a separate commit with the silencer. Have a look, and please let me know what you think. I am happy to keep it or remove it.

I added silencer under a separate libraryDependencies declaration to separate from the important dependencies. However, maybe it is unnecessary.

[rossabaker]

Yes, I think silencer is an appropriate addition, and the only way we can get the experience we want without turning off fatal warnings or breaking binary compatibility.

It looks like we also need to silence some references in tests.

[kubum]

@rossabaker Well spotted, I just updated the PR. What do you think? Thank you!

[rossabaker]

Thanks! The code looks good to me. Docs need a little more mopup.

I milestoned this as 2.2.0 for proper semver, but the version number is negotiable.

If we still want to add swap, we should do it here, or be sure it gets done before we release again, or else we'll have an MVar3 and be doing this dance again. Are any other methods missing?

[kubum]

Thanks! The code looks good to me. Docs need a little more mopup.

I milestoned this as 2.2.0 for proper semver, but the version number is negotiable.

If we still want to add swap, we should do it here, or be sure it gets done before we release again, or else we'll have an MVar3 and be doing this dance again. Are any other methods missing?

Thanks for the review @rossabaker! I added "swap" with tests too. Please have a look and let me know what you think.

[alexandru]

Thanks for looking into it! I haven't checked the implementation of MVar in Monix. Surprisingly, it already has tryRead implemented.

@kubum yes, Monix's implementation has tryRead. This must have happened after we released Cats-Effect 1.0.0 and as you can see, adding new methods ain't easy.

In the next Cats-Effect I'm thinking that these shouldn't be abstract classes, because it prevents us from adding new methods. And there's not much value in maintaining different implementations in a project like Monix. I'm also thinking that these should be in a sub-project separate from the core exposing the type classes (e.g. cats-effect-data or something) but I digress.

[alexandru]

I don't like provided libs, because they end up in the generated pom.xml anyway. Would be cool to have a setup that filters these out.

I'm using something like this:

/**
  * Filter out dependencies from the generated `pom.xml`.
  *
  * E.g. to exclude Scoverage:
  * {{{
  *   filterOutDependencyFromGeneratedPomXml("groupId" -> "org\\.scoverage".r)
  * }}}
  *
  * Or to exclude based on both `groupId` and `artifactId`:
  * {{{
  *   filterOutDependencyFromGeneratedPomXml("groupId" -> "io\\.estatico".r, "artifactId" -> "newtype".r)
  * }}}
  */
def filterOutDependencyFromGeneratedPomXml(conditions: (String, Regex)*) = {
  def shouldExclude(e: Elem) =
    e.label == "dependency" && {
      conditions.forall { case (key, regex) =>
        e.child.exists(child => child.label == key && regex.findFirstIn(child.text).isDefined)
      }
    }

  if (conditions.isEmpty) Nil else {
    Seq(
      pomPostProcess := { (node: xml.Node) =>
        new RuleTransformer(new RewriteRule {
          override def transform(node: xml.Node): Seq[xml.Node] = node match {
            case e: Elem if shouldExclude(e) => Nil
            case _ => Seq(node)
          }
        }).transform(node).head
      },
    )
  }
}

[kubum]

Ah, I didn't know "provided" still keeps it in the pom.xml. Thank you, this snipper is amazing. I added it to a separate file and called it to exclude both: "scoverage" and "silencer". What do you think?

[alexandru]

👍

[djspiewak]

I think we already have something a bit like this with the CompileOnly scope, or something like that. I can't remember. We used it for simulacrum.

[rossabaker]

Simulacrum is still Provided in cats, with a similar pom-stripping trick to this. Maybe @djspiewak is thinking of the @compileTimeOnly annotation?

[alexandru]

We want to provide good ScalaDocs. I wonder what we can do here, because people are going to be looking at the description of MVar2, which has this sentence that has technical implementation details and with no link to MVar. And then if they'll take a look at the ScalaDoc for MVar, they'll see it as being deprecated.

[kubum]

Good point. It seems there are two ways:

• Reference both. The downside of it is that tools (IDE helpers) won't give the relevant docs immediately.
• Copy docs from MVar to MVar2. The downside is maintaining the two copies.

Are there something else? What do you think?

[alexandru]

You can define reusable documentation via @define in ScalaDoc.

And definitions get inherited, so if you @define something on MVar, it should then be available in MVar2. And a trick would be to do something like ...

/** 
  * @define mvarDescription An MVar is a mutable location that can be empty or 
  *         contain a value, asynchronously blocking reads when empty and 
  *         blocking writes when full.
  * 
  *         Use-cases:
  *          - bla bla ...
  */
private[concurrent] trait MVarDocs extends Any {}

/** 
  * $mvarDescription
  */
abstract class MVar[F[_], A] extends MVarDocs

/** 
  * $mvarDescription
  */
abstract class MVar2[F[_], A] extends MVar[F, A]

It's just a template value, so you can add your warning before or after it.

[kubum]

Ah, it is a great suggestion, thank you. I just added it.

I have tried to avoid the redundant trait, but scaladocs kept crashing with expansion errors.

[djspiewak]

Ping @rossabaker? I defer to you on all of this. :-)

[rossabaker]

This looks good to me. I've never used MVar, so I'm not the most qualified, but I note a few functions that are missing compared to the Haskell version:

• withMVar
• modifyMVar_
• modifyMVar

We don't use MVar in the method names, and might rename withMVar to use since with is a keyword.

The only reason I bring them up now is that adding them later would result in an MVar3. We could do it in a separate PR, but we should do it before 2.2.0 if we think we might want those.

[rossabaker]

Simulacrum is still Provided in cats, with a similar pom-stripping trick to this. Maybe @djspiewak is thinking of the @compileTimeOnly annotation?

[djspiewak]

Actually I was thinking of this: https://github.com/typelevel/cats-effect/blob/master/build.sbt#L28 So in other words, if you swap Provided for CompileTime, it should be neatly stripped from the pom.

[kubum]

Actually I was thinking of this: https://github.com/typelevel/cats-effect/blob/master/build.sbt#L28 So in other words, if you swap Provided for CompileTime, it should be neatly stripped from the pom.

Hi @djspiewak, I wonder if you know the answer, but when I swap Provided for CompileTime it refuses to run sbt ci:

sbt:root> ci
[info] Checking 2 Scala sources...
[success] Total time: 3 s, completed 22-Mar-2020 16:39:32
[success] Total time: 0 s, completed 22-Mar-2020 16:39:32
[info] Compiling 18 Scala sources to /Users/andreyfadeyev/Sites/github/cats-effect/core/js/target/scala-2.12/test-classes ...
[info] Compiling 19 Scala sources to /Users/andreyfadeyev/Sites/github/cats-effect/core/jvm/target/scala-2.12/test-classes ...
[error] `silencer-plugin` was enabled but the @silent annotation was not found on classpath - have you added `silencer-lib` as a library dependency?
[error] one error found
[error] `silencer-plugin` was enabled but the @silent annotation was not found on classpath - have you added `silencer-lib` as a library dependency?
[error] one error found
[error] (coreJVM / Test / compileIncremental) Compilation failed
[error] (coreJS / Test / compileIncremental) Compilation failed

but sbt compile works:

sbt:root> compile
[success] Total time: 0 s, completed 22-Mar-2020 16:39:07

Do you know a way to make CompileTime to work with the silencer?

[kubum]

Just updated the doc and rebased. I couldn't make CompileTime to work with the silencer, left the rest as it is. Please let me know what you think should be improved.

[rossabaker]

I had the same problem in http4s last night trying to use CompileTime on silencer.

[rossabaker]

When it's in CompileTime, it's not accessible to the tests. I made it work by also adding it to Test scope:

    libraryDependencies ++= Seq(
      compilerPlugin(("com.github.ghik" % "silencer-plugin" % SilencerVersion).cross(CrossVersion.full)),
      ("com.github.ghik" % "silencer-lib" % SilencerVersion % CompileTime).cross(CrossVersion.full),
      ("com.github.ghik" % "silencer-lib" % SilencerVersion % Test).cross(CrossVersion.full)
    ),

For whatever it's worth, SilencerVersion can now be 1.6.0.

@kubum has done great work on this, and I'd like to see us get it merged ASAP. I'll add a follow-up ticket to get the other relevant methods from MVar in before 2.2.0 so we don't get stuck with MVar3.

[kubum]

When it's in CompileTime, it's not accessible to the tests. I made it work by also adding it to Test scope:

    libraryDependencies ++= Seq(
      compilerPlugin(("com.github.ghik" % "silencer-plugin" % SilencerVersion).cross(CrossVersion.full)),
      ("com.github.ghik" % "silencer-lib" % SilencerVersion % CompileTime).cross(CrossVersion.full),
      ("com.github.ghik" % "silencer-lib" % SilencerVersion % Test).cross(CrossVersion.full)
    ),

For whatever it's worth, SilencerVersion can now be 1.6.0.

@rossabaker Your idea worked fantastic! I just removed the last commit with the pom filter, bumped silencer to 1.6.0 and followed the suggested way to include both in test and compile time. Thank you!

[rossabaker]

👍 on green.

[djspiewak]

Terribly sorry for missing this! I didn't notice the approval. 🎉

[djspiewak]

Thank you so much for following this through, @kubum!