Collector.register's signature doesn't make sense

[mberndt123]

Hi,

the current signature of Collector.register is

public <T extends Collector> T register(CollectorRegistry registry)

This doesn't make sense, there is no correct implementation of such a signature. It is impossible to return something of type T for every subtype T of Collector, yet this is what the signature promises. Among others, this leads to dead code warnings when using this library with the Scala programming language.

I can see two possible solutions to this problem: simplify the method's return type to Collector or void, or make T a type parameter of the Collector class itself like so:

public abstract class Collector<T extends Collector> {
  …
  protected abstract T self(); // override in subclasses to return `this`
  public T register(CollectorRegistry registry) {
    registry.register(this);
    return self();
  }
  …
}

[brian-brazil]

What you propose would be a breaking change for all existing users. It promises to return a T which is a subclass of Collector, and does just that by returning itself. This way of doing things ensures that the resultant type is not Collector and thus the user doesn't have to cast it.

[mberndt123]

Hi @brian-brazil, thanks for responding.

What you propose would be a breaking change for all existing users.

I realize that it can not be changed in a bugfix release.

promises to return a T which is a subclass of Collector, and does just that by returning itself

No, that is not what is promises. It promises to return an arbitrary subtype of Collector that the caller gets to choose.

This way of doing things ensures that the resultant type is not Collector and thus the user doesn't have to cast it.

I'm afraid that's not how this works. Something like this won't compile:

Counter.build().name("foo").help("bar").create().register().inc();

The compiler has no reason to infer T = Counter, hence it doesn't know that the returned value has an inc method. What you have to write instead is this:

Counter.build().name("foo").help("bar").create().<Counter>register().inc()

I don't see how that's any better than a cast. In fact it's much worse, because when there is a context to infer the type parameter from, the Java compiler will do just that. So this compiles and crashes at run-time:

Gauge g = Counter.build().name("foo").help("bar").create().register(); // no cast needed

If you want to offer a convenient, fluent interface, you need to do what I proposed above and declare the type parameter on the class rather than the method.

[brian-brazil]

The current logic was added in #86 for the purposes of working with Scala.

Counter.build().name("foo").help("bar").create().register().inc();

This is not sane usage, a counter should be assigned to a field.

[mberndt123]

The current logic was added in #86 for the purposes of working with Scala.

Well, what can I say? It shouldn't have been.

This is not sane usage, a counter should be assigned to a field.

That is besides the point. Please take another look at my last example:

Gauge g = Counter.build().name("foo").help("bar").create().register(); // no cast needed

This looks innocuous at first but will crash at run-time. Don't you agree that that's a problem?

[brian-brazil]

It'll crash immediately at startup with expected usage, not during serving. Do you have a suggestion that's not a breaking change?

[mberndt123]

No, there is no way to fix this without changing the signature. It's the signature that's wrong, not the implementation.

[mberndt123]

By the way, the current signature of Collector.register wasn't introduced in #86. That signature was SimpleCollector.Builder.register, not Collector.register. And in fact that commit does exactly what I propose to do with Collector too: add a type parameter to the class (in this case the Builder class) and remove it from the create and register methods.

[mberndt123]

Note that most users would not be affected if a type parameter is added to Collector. It would only break compatibility for users who derive from the Collector class. I would guess that not many people do that, and it's a change that is trivial to adapt to. I can send a PR if you like…

[mberndt123]

It'll crash immediately at startup with expected usage

By the way, this is actually not true when you declare the variable using a type parameter (because of erasure).

class Foo<T extends Collector> {
  public T t = Counter.build().name("foo").help("bar").create().register();
}

new Foo<Gauge>();

This will compile and not crash, until somebody tries to use t.

[brian-brazil]

I would guess that not many people do that, and it's a change that is trivial to adapt to.

Custom collectors are reasonably common, all exporters have them for example.

class Foo {

This is not a sane usage.

[mberndt123]

This is not a sane usage.

I disagree with the notion that every usage you don't like is “not sane”. The only thing I see here that isn't sane is the signature of the method.

By the way, why is the API break in #86 acceptable? Because it changes the API in the exact same way that I'm proposing here.

[eugengarkusha]

Counter.build().name("foo").help("bar").create().register().inc();

This is not sane usage, a counter should be assigned to a field.

@brian-brazil pls correct me if I am wrong but one of the main purposes of type system is to prevent accidental bugs caused by unintentional "not sane usage".

For example, passing a Boolean into a method that accepts strings is obviously not a sane usage of that method but compiler helps us out there emitting an error and not letting this break at runtime.

It'll crash immediately at startup with expected usage, not during serving.....

When things pass compilation - they get into run time. There is no such a notion of "just before the real run time".

@brian-brazil do you agree that this signature can be improved in future releases to protect inexperienced programmers from accidental bugs?

[brian-brazil]

There is no such a notion of "just before the real run time".

There's initialisation, and then there's serving. This is one of a few errors that'd only get caught on initialisation.

@brian-brazil do you agree that this signature can be improved in future releases to protect inexperienced programmers from accidental bugs?

I'm open to non-breaking suggestions.

[mberndt123]

There's initialisation, and then there's serving.

Yeah, and then there's lazy initialization, so this can lead to bugs at any time.

I'm open to non-breaking suggestions.

This is a fancy way of saying that you're not open to suggestions since, as I pointed out before, there is no backward-compatible way to fix this. I can only ask you to reconsider, I don't think this kind of bug is acceptable in a well-behaved Java library.

@willfleury, you were involved in #86, what's your opinion about this?

[mberndt123]

Oh, I would like to point out that adding a type parameter to the Collector class would not affect binary compatibility since generics are erased. So this would only affect source compatibility, not binary compatibility.

[brian-brazil]

I care about source compatibility. Is this actually something you've run into, or more a theoretical concern?

[mberndt123]

It causes issues for me because it causes dead code warnings in the Scala compiler. The Scala compiler understands that there is no sound way to implement a signature like that. Since we compile with warnings as errors on CI, this does cause problems for me.

[mberndt123]

So I take it nothing is going to happen here?

[mr-git]

instead of "nice looking" val c = Counter.build().name("foo").help("bar").create().register() we changed code to

val c: Counter = {
  val c = Counter.build().name("foo").help("bar").create()
  CollectorRegistry.defaultRegistry.register(c)
  c
}

This ugly workaround does the trick and doesn't trigger "dead-code after register() call" warning.

[brian-brazil]

Sounds like there's nothing we can do here, and there is a workaround.

[mberndt123]

Sounds like there's nothing we can do here, and there is a workaround.

Oh, you very much can do something, and you did in #86. The issue is you don't want to, please be honest about that.

[mr-git]

maybe we could add second set of API, with better types and users would be able to pick any of APIs to use?