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

EitherIdOpsBinCompat0 causes scala-newtype tagged type to not compile #2491

Closed
tom91136 opened this issue Sep 16, 2018 · 5 comments
Closed
Labels
Milestone

Comments

@tom91136
Copy link

Quite an interesting issue here. Basically, I was using scala-newtype together with cats 1.4.0 which I upgraded from 1.2.0 just today and my project will not longer compile. I isolated the issue to this:
I had a @newtype case class Foo(value : String) which, at some point, compiles to:

	type Foo = Foo.Type
	object Foo {
		type Repr = String
		type Base = Any {type Foo$newtype}
		trait Tag extends Any
		type Type <: Base with Tag

		def apply(x: String): Foo = x.asInstanceOf[Foo]

		implicit final class Ops$newtype(val $this$: Type) extends AnyVal {
			def value: String = $this$.asInstanceOf[String]
		}
	}

	def a() = {
		import cats.implicits._
		val foo = Foo("")
		val value: String = foo.value // error
	}

And the compiler says:

[error]  found   : test.Foo
[error]     (which expands to)  test.Foo.Type
[error]  required: String
[error]                 val value: String = foo.value
[error]                                         ^

I've manually pasted the code generated from scala-newtype and removed the dependency and this still happens.
If I change Foo's value to some other name, say def abc: String = $this$.asInstanceOf[String] then it works.

It seems that 380f721 added the syntax final class EitherIdOpsBinCompat0[A](val value: A) extends AnyVal{...} which clashes with my value defined in my tagged type. To verify this, I also used the name a and that also causes the same issue but this time with final class ValidatedIdOpsBinCompat0[A](val a: A) extends AnyVal{...}. IntelliJ's click to open implementation when used on the foo.value symbol actually takes me to EitherIdOpsBinCompat0[A](val value: A) which confirms my suspicion.

The obvious workaround is to not have newtypes with a or value as fields but I imagine these are popular names for this kind of use.

@kailuowang
Copy link
Contributor

Thanks so much for investigating and reporting. This is rather unfortunate. Two value classes over compatible types can't share the same field name. We (cats) should have been more careful with polluting such a broad namespace. At the moment I couldn't think of a fix that doesn't break binary compatibility.
A work around I can think of is creating a new cats.implicits and cats.syntax.all (with slightly different name) that exclude this EitherIdOpsBinCompat0 if you really need to keep your value field name.

@kailuowang kailuowang added the bug label Sep 16, 2018
@kailuowang
Copy link
Contributor

I did some preliminary experiments, there might be a fix that is both source (mostly except the value field name, which shouldn't be called anyway) and binary compatible. The idea is to make the old implicit conversion package private, and then create a new public version with a better value field name (e.g. value$catsSyntaxExtension or something).
This way jars compiled against Cats 1.3.1 and 1.4.0 version would still use the package private version, and jars compiled against the new Cats 1.4.1 would use the new public version.
We might take this opportunity to fix all Id syntax extensions. I'd also propose we create a different cats.implicits that excludes all Id Ops that apply to all types.

@tom91136
Copy link
Author

tom91136 commented Sep 16, 2018

Sounds good. I'm all for a cats.implicits that excludes all types. For now I've simply renamed my fields to something else and all is fine. TBH the error is rather bizarre when just looking at the compiler output, the symbol value clash was not apparent until I individually checked out each revision to see where it stops compiling so making a less powerful implicit import is definitely a good thing.

Probably not a low hanging fruit but let me know if I can help in anyway.

@kailuowang
Copy link
Contributor

We are going to address this one in #2514

@kailuowang kailuowang added this to the 1.5.0-RC1 milestone Nov 17, 2018
@kailuowang
Copy link
Contributor

This is fixed #2614

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants