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

Forward ref check #136

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cchantep
Copy link
Member

Fix #120

@cchantep cchantep force-pushed the fix/120_forward_ref_check branch 2 times, most recently from 40554be to 9575b0a Compare January 27, 2018 14:43
val implTpeName = Option(impl.tpe).fold("null")(_.toString)

if (implTpeName.startsWith(forwardPrefix) ||
(implTpeName.startsWith("play.api.libs.json") &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think starting with play.api.libs.json proves it's a built-in format. If you use Writes.apply you'll have a class named something like play.api.libs.json.Writes$$anon$6@509f0a9d.

Maybe it's worth just skipping this check and doing the null check for everything?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? I'm pretty sure the lambda is defined in the class it's lexically inside. E.g. if you go to https://alvinalexander.com/scala/anonymous-classes-in-scala-examples and grep for AnonymousClassTest1 you'll see an example.

But I also have no problem with doing a null check for everything since the cost will be low.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cchantep can you verify this with a unit test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's planned

Copy link
Member

@gmethvin gmethvin May 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be mistaken here, but as I understand it impl is the Tree representing the implementation of the implicit Writes/Reads/Format. So if I have something like:

implicit val fooWrites = Writes[Foo] { foo => JsString(foo.value) }

then Play is creating the anonymous class for you based on the lambda you passed. I wouldn't expect impl.tpe to be useful here in identifying Play-provided formats.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such factory is instantiating from FunctionalX classes, not excluded from the null-check, contrary to other types from the package

@cchantep cchantep force-pushed the fix/120_forward_ref_check branch from 9575b0a to 1050dd5 Compare April 28, 2018 19:27
val implTpeName = Option(impl.tpe).fold("null")(_.toString)

if (implTpeName.startsWith(forwardPrefix) ||
(implTpeName.startsWith("play.api.libs.json") &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? I'm pretty sure the lambda is defined in the class it's lexically inside. E.g. if you go to https://alvinalexander.com/scala/anonymous-classes-in-scala-examples and grep for AnonymousClassTest1 you'll see an example.

But I also have no problem with doing a null check for everything since the cost will be low.

!implTpeName.contains("MacroSpec"))) {
impl // Avoid extra check for builtin formats
} else {
q"""_root_.java.util.Objects.requireNonNull($impl, "Invalid implicit resolution (forward reference?) for '" + $cn + "': " + ${implTpeName})"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalid implicit resolution -> Implicit value was null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@cchantep cchantep force-pushed the fix/120_forward_ref_check branch 2 times, most recently from 213db84 to b0222b1 Compare September 25, 2018 12:47
@@ -32,26 +32,27 @@ object OFormat {

implicit def GenericOFormat[T](implicit fjs: Reads[T], tjs: OWrites[T]): Format[T] = apply(fjs, tjs)

def apply[A](read: JsValue => JsResult[A], write: A => JsObject): OFormat[A] = new OFormat[A] {
def apply[A](read: JsValue => JsResult[A], write: A => JsObject): OFormat[A] =
new FunctionalOFormat[A](read, write)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use specific impl class, not to exclude null-check for instances created by these factories

val implTpeName = Option(impl.tpe).fold("null")(_.toString)

if (implTpeName.startsWith(forwardPrefix) ||
(implTpeName.startsWith("play.api.libs.json") &&
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such factory is instantiating from FunctionalX classes, not excluded from the null-check, contrary to other types from the package

@@ -187,44 +190,6 @@ object Reads extends ConstraintReads with PathReads with DefaultReads with Gener
implicit val JsArrayReducer = Reducer[JsValue, JsArray](js => JsArray(Array(js)))
}

/**
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved down

@SethTisue SethTisue marked this pull request as draft April 13, 2021 16:52
@SethTisue
Copy link
Member

is anyone interested in pursuing this further, or should we just close it for inactivity?

@cchantep cchantep force-pushed the fix/120_forward_ref_check branch from b0222b1 to ab55f42 Compare June 26, 2021 12:28
@lightbend-cla-validator

At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user

@cchantep cchantep marked this pull request as ready for review June 26, 2021 12:29
@cchantep cchantep force-pushed the fix/120_forward_ref_check branch from ab55f42 to bc570f2 Compare June 26, 2021 13:00
@lightbend-cla-validator

At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user

@cchantep cchantep force-pushed the fix/120_forward_ref_check branch from bc570f2 to 02570d0 Compare June 26, 2021 13:06
@lightbend-cla-validator

At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user

@SethTisue SethTisue marked this pull request as draft June 26, 2021 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NullPointerException trying to pickle Coursier case class
6 participants