Do not extend AnyVal which triggers wrong QueryString/PathBindable

[mkurz]

I have no idea why in #7621 those UserId classes were made to extends AnyVal. Back when that pull request was merged, that didn't matter. Let's again look at the code of that pull request (and that code still is alive in master):

playframework/dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/routes-compiler-injected-routes-compilation/app/models/UserId.scala

Lines 12 to 27 in b2645bd

	object UserId {
	implicit object pathBindable
	extends PathBindable.Parsing[UserId](
	UserId.apply,
	_.id,
	(key: String, e: Exception) => "Cannot parse parameter %s as UserId: %s".format(key, e.getMessage)
	)
	implicit object queryStringBindable
	extends QueryStringBindable.Parsing[UserId](
	UserId.apply,
	userId => URLEncoder.encode(userId.id, "utf-8"),
	(key: String, e: Exception) => "Cannot parse parameter %s as UserId: %s".format(key, e.getMessage)
	)
	}
	case class UserId(id: String) extends AnyVal

As you can see an implicit pathBindable and queryStringBindable were added as well. Great, that worked.

However, a couple of month later following pull requests were merged as well: #8076 and #8093
Merging those two pull request made the above pathBindable/queryStringBindable useless, because now the newly added Path/QueryStringBindable's for AnyVal kick in and the above ones just get ignored.

I ran into this when hacking on scripted tests and changed the above bindables, but nothing happend...Took me a while to figure out what's actually going on. When removing the AnyVal extends, above binders will be used again.
I think that was the original meaning and sense of those binders, not introducing them so they are dead code...

[mkurz]

@Mergifyio backport 2.8.x

[mergify]

backport 2.8.x

🟠 Waiting for conditions to match

• merged [📌 backport requirement]

[gmethvin]

You've discovered an interesting issue here...

I have no idea why in #7621 those UserId classes were made to extends AnyVal.

This is a pretty standard use case for a value class, similar to what's described in https://docs.scala-lang.org/overviews/core/value-classes.html#correctness. The value class is just there to provide a type-safe wrapper around the String.

The idea behind #8076 and #8093 is that you generally want a value class to be treated like the underlying type for bindables, so it's nice to have that implementation provided by default. I'm not sure anyone considered the fact that the macro-generated bindables would take precedence over the ones defined in the companion object of the associated type.

This is obviously very surprising behavior, so I wonder if we should remove the macro from the implicit scope.

As a workaround here, I think I'd prefer to import our custom bindables explicitly in routesImport, rather than removing the extends AnyVal. In this small test project it doesn't matter much, but in a real-world codebase I would not want the code to break if some developer decided add extends AnyVal, since on its own that's a completely reasonable thing to do.

[mkurz]

As a workaround here, I think I'd prefer to import our custom bindables explicitly in routesImport, rather than removing the extends AnyVal.

Actually, that was what I did first. However, the import will be added to all generated routes files, which results in unused imports in some of them, which makes the scripted tests fail because they all run with the -Xfatal-warnings/-Werror flag (which I don't want to remove).
Anyway, now that Scala 2.12.13 is released with the -Wconf flag backported, I want to finally address #7382 as well, to ignore such unused import warnings automatically out of the box.
Afterwards I will come back to this issue here and change things so the custom bindables will be imported via routesImport.