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

Add explicit unique ResourceTag-like keys #266

Open
nigredo-tori opened this issue Apr 20, 2021 · 4 comments
Open

Add explicit unique ResourceTag-like keys #266

nigredo-tori opened this issue Apr 20, 2021 · 4 comments

Comments

@nigredo-tori
Copy link

nigredo-tori commented Apr 20, 2021

This is inspired by the vault library.

At the moment global resources are indexed by ResourceTags and labels. This has its issues:

  1. The default ResourceTag implementation derives its identity from ClassTag. This leads to a possibility of collisions, as stated in the documentation. The suggested workarounds are monomorphic wrappers or custom ResourceTag instances.
  2. Labels mitigate the collision problem somewhat (or, rather, leave it up to discipline). But the labels are not tied to resource types, so it's easy to mix up which labels correspond to which resource.

Instead, we can create objects that are guaranteed to be unique, and which are associated with a specific type. Consider something like this:

// default `equals` - different objects are never equal.
final private class UniqueResourceTag[A](
  val description: String
) extends ResourceTag[A] {
  def cast(obj: Any): Option[A] = Some(obj.asInstanceOf[A])
}
def unsafeCreateUniqueTag[A](description: String): ResourceTag[A] =
  new UniqueResourceTag[A](description)

This can be used as follows:

// A "constant" somewhere in a common object
val fooTag = unsafeCreateUniqueTag[Foo]("")
// Inside a suite
global.getR()(fooTag)

The current API is obviously not perfect for this, but the general idea should be clear enough. Note that there's no way for fooTag to collide with anything else (assuming no broken equals implentations), and there's no way to mistake the type of the resource it points to.

@nigredo-tori nigredo-tori changed the title Add explicit typed ResourceTag-like keys Add explicitly typed ResourceTag-like keys Apr 20, 2021
@nigredo-tori nigredo-tori changed the title Add explicitly typed ResourceTag-like keys Add explicit unique ResourceTag-like keys Apr 20, 2021
@Baccata
Copy link
Contributor

Baccata commented Apr 20, 2021

Hello, and thank you for raising an issue

But the labels are not tied to resource types

As a matter of fact they are : ie the ResourceMap is keyed by ResourceTag + Option[String].

This leads to a possibility of collisions, as stated in the documentation.

Yes, and for this very reason we are protecting against it via forcing implicit ambiguity when collisions are possible (ie compile error rather than surprising runtime behaviour), and we've made the conscious decision to leave the API flexible enough so that users can bring in their own patterns (exactly like what you suggest if they want to use that).

The reason why I don't want to support your suggestion out of the box is that, unlike vault's examples, it's impossible to pass the values around logically, from the creation of a key, to its use. So such instances have to be instantiated globally, and for it to work, they have to be assigned to a val and not a def (otherwise you get a new instance on every callsite and referential equality can't kick-in). So as a library author/maintainer, I'd have to protect against assigning these to def via some macro (cause I can guarantee that some users will f***-up), and suddenly the maintenance burden increases.

So I'm more inclined to let this problem be solved by users when they encounter it, than introducing a new construct which is inherently risky without macros, and I don't really want to have to maintain more macros than what we already have. The existing solution might not be perfect, but at least mis-using classtag-based resourceTags lead to compile errors.

However, I'd be willing make a version of global.getR (and its putR dual) that'd take the tag explicitly rather than implicitly, so that solving this in userland wouldn't look as weird.

@nigredo-tori
Copy link
Author

As a matter of fact they are : ie the ResourceMap is keyed by ResourceTag + Option[String].

I guess I didn't phrase it right. Let's say I have putR[Foo](foo, "something".some) as my global setup, and getOrFailR[Foo]("something".some) in my test suites. Now let's say I have decided that instead of Foo I want to share a Bar. I change the first part to putR[Bar](bar, "something".some). Now all the getOrFailR parts compile fine, but fail at runtime!

Instead, if I had a single val somethingTag: ResourceTag[Foo] somewhere, and changed it to ResourceTag[Bar], then I would just have to follow the compiler errors.

So as a library author/maintainer, I'd have to protect against assigning these to def via some macro (cause I can guarantee that some users will f***-up), and suddenly the maintenance burden increases.

Fair point. I can't say for sure whether guarding this with macros would be warranted, but if you think it would be, then this becomes a non-trivial undertaking. Since I can rely on my coworkers being principled enough, I'm fine with keeping the "dangerous" version in our in-house "bits and bobs" library. 😄

@Baccata
Copy link
Contributor

Baccata commented Apr 20, 2021

Since I can rely on my coworkers being principled enough, I'm fine with keeping the "dangerous" version in our in-house "bits and bobs" library.

👍 that sounds like a good work environment 😄

Instead, if I had a single val somethingTag: ResourceTag[Foo] somewhere, and changed it to ResourceTag[Bar], then I would just have to follow the compiler errors.

So would a method that takes tags explicitly be a satisfying middle-ground ?

@nigredo-tori
Copy link
Author

So would a method that takes tags explicitly be a satisfying middle-ground ?

Yes.

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

No branches or pull requests

2 participants