https://kotlinlang.org logo
#strikt
Title
# strikt
r

robfletcher

02/01/2022, 4:07 PM
huh, that overload looks pointless. I should be able to remove the 2nd version of that method
👍 1
c

christophsturm

02/01/2022, 4:37 PM
oops was that my change to make isNotNull work on non nullable subjects?
e

Eric

02/01/2022, 4:38 PM
I actually like that
isNotNull
doesn’t didn’t work on non-nullable objects
c

christophsturm

02/01/2022, 5:28 PM
why?
r

robfletcher

02/01/2022, 6:02 PM
Hm, this is kinda tricky. If the function has
<T : Any?>
then it's impossible to define that it returns an assertion enclosing a definitely non-null subject
which is why the original function was
fun <T : Any> Assertion.Builder<T?>.isNotNull(): Assertion.Builder<T>
I'm inclined to agree that being able to use
isNotNull
on a known non-null subject is not something I'd actually do, but I was happy to merge because it seemed at least harmless
not sure what the solution here could be aside from backing out that overloaded version of the function
c

christophsturm

02/01/2022, 6:28 PM
For me it's not a big deal. Just revert it
I would not write a isnotnull assert on a non nullable subject, but I sometimes change nullability and then it's strange to see isnotnull become red.
Code changes after the test is written
r

robfletcher

02/01/2022, 6:32 PM
right
I'm going to try to figure out if there's a way that I can make it work, but if I can't I'll revert
c

christophsturm

02/01/2022, 7:00 PM
Maybe it's a compiler bug and it should use the nullable version?
r

Roukanken

02/01/2022, 9:08 PM
additional argument against
Builder<T>.isNotNull()
- if you're doing any generics (weird in tests, but probably can happen in complex projects?) then stuff can get confusing
Copy code
fun <T> test(subject: T) {
    expectThat(subject).isNotNull()  
}

fun main() {
    test<Int?>(null)
}
afaik this will pass currently I guess it won't happen that often in strikt, as you would rather write a custom assertion, and then this won't happen as much I think, but if you need to do something like communicate interactively a with some generic payload, stuff can get iffy... (eg, do asserts in middle of communication of the payload and so, over different message types, maybe even collection types...)
r

robfletcher

02/01/2022, 9:31 PM
nasty
c

christophsturm

02/02/2022, 8:41 AM
oh wow i had a bad feeling. if we want to add that feature we need to test for this case
r

robfletcher

02/02/2022, 4:21 PM
Yeah, I was fiddling with it a little yesterday and I think it may be due to the type signature of the function. Its generic is
<T : Any?>
and the receiver is
Assertion.Builder<T?>
which means the type could be say
String?
and then the receiver is
Assertion.Builder<String??>
(which is obviously not a thing but leads to the confusion in that special case described there)
r

Roukanken

02/02/2022, 4:24 PM
well it's more like because it's a static call from the JVM view
since it's a extension method, there always has to be the same one called in
fun <X> test()
and since
Builder<T?>.isNotNull()
can't handle both cases
X = Int
and
X = Int?
it statically chooses
Builder<T>.isNotNull()
and uses it always Edit: use different generics to make it more readable
but you can fix this without removing the method, with 2 choices: a) make the
Builder<T>.isNotNull()
callable only non nullable T's, as it thinks it is (mostly, true but sometimes not) - eg, change signature to
fun <T: Any> Builder<T>.isNotNull(): Builder<T>
• afaik, this would completely forbit usage of it in the case I described b) give the
Builder<T>.isNotNull()
same implementation as the other one, as it can happen it can get called on nullable T's, in special cases. this ofc doesn't resolve the issue with
T!
's...
c

christophsturm

02/02/2022, 4:42 PM
I agree that both impls should check if the subject really is not null just to be sure
r

robfletcher

02/03/2022, 12:46 AM
I pushed out a release that removes the overload for now. If there's a way to avoid the ambiguity I'm happy to re-add it, but it actually impacted someone here at work!
👍 2
c

christophsturm

02/03/2022, 7:53 AM
its probably not worth the hassle
r

robfletcher

02/03/2022, 2:47 PM
I'd like to be able to define a single function, but it's very useful that
isNotNull
narrows the subject type from
T?
to
T
and I don't see a way to do that while allowing for a non-null subject going in
👍 1
c

christophsturm

02/03/2022, 2:49 PM
maybe with contracts?
r

robfletcher

02/03/2022, 2:52 PM
hmm, I wonder yes
r

Roukanken

02/03/2022, 3:25 PM
can't work with contracts, because:
Copy code
expectThat(x) {
    isNotNull().somethingOnlyOnNonNullable()
    somethingOnlyOnNonNullable()
}
if first works because contracts, then the second has to work too (which it obviously can't)
(eg, compiler would not see difference between types of
this@expectThat
and
isNotNull()
)
d

dave08

02/07/2022, 4:02 AM
isNotNull {.... }
That would show the intention too... I find myself writing isNotNull().and {} pretty often...