search

smallrye / smallrye-common

Common utilities for SmallRye
Apache License 2.0
21 stars 24 forks source link

Additional type pollution fix #224

Closed Sanne closed 1 year ago

Sanne commented 1 year ago

Similar to #190 , this looks rather trivial but has severe performance implications.

I wonder if we shouldn't delete all uses of Assert.checkNotNullParam, it seems rather dangerous.

franz1981 commented 1 year ago

Avoiding the cast by returning void will work too

Sanne commented 1 year ago

Avoiding the cast by returning void will work too

right that's essentially what I've done here: ignore the return type. Perhaps that would be a good change to the Assert.checkNotNullParam so to avoid the problem in future.. WDYT @radcortez ?

radcortez commented 1 year ago

right that's essentially what I've done here: ignore the return type. Perhaps that would be a good change to the Assert.checkNotNullParam so to avoid the problem in future.. WDYT @radcortez ?

Sure. We probably need to deprecate that method and introduce a new one.

Ladicek commented 1 year ago

Something like this.foo = Objects.requireNonNull(foo) (or Assert.checkNotNullParam() for that matter) is a very common idiom in constructors. I have no idea why it was used here (and I'm obviously +1 for avoiding it), but removing the Assert.checkNotNullParam() method is not gonna remove the usages of this idiom. The Objects.requireNonNull() method exists in the JDK, and I'm pretty sure a lot of other projects have their own copy as well (I know I do in SmallRye Fault Tolerance, for example).

dmlloyd commented 1 year ago

I have to also add a 🛑 to this line of reasoning... what we're working around here is a JVM bug which people are working on fixing, whereas an API change is much more long-term. As @Ladicek pointed out this is a commonly used pattern so changing the API isn't really warranted.

I'm OK with some simple/localized band-aid type fixes for the type pollution issue to stop the bleeding but I am against significant reengineering or API redesigning to work around it. Once the issue is fixed, I feel we will regret the amount of effort we spent on the problem which could have been spent in other areas, and we will also regret how we hobbled our own programming models for a short-term gain.

Sanne commented 1 year ago

Could this get merged?

Sanne commented 1 year ago

Thanks!