huh, that overload looks pointless. I should be ab...

# strikt

[robfletcher]

huh, that overload looks pointless. I should be able to remove the 2nd version of that method

[christophsturm]

oops was that my change to make isNotNull work on non nullable subjects?

[Eric]

I actually like that isNotNull doesn’t didn’t work on non-nullable objects

[christophsturm]

why?

[robfletcher]

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

[robfletcher]

which is why the original function was fun <T : Any> Assertion.Builder<T?>.isNotNull(): Assertion.Builder<T>

[robfletcher]

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

[robfletcher]

not sure what the solution here could be aside from backing out that overloaded version of the function

[christophsturm]

For me it's not a big deal. Just revert it

[christophsturm]

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.

[christophsturm]

Code changes after the test is written

[robfletcher]

right

[robfletcher]

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

[christophsturm]

Maybe it's a compiler bug and it should use the nullable version?

[Roukanken]

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

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...)

[Roukanken]

https://pl.kotl.in/zuVz_oiS4

[robfletcher]

nasty

[christophsturm]

oh wow i had a bad feeling. if we want to add that feature we need to test for this case

[robfletcher]

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??>

[robfletcher]

(which is obviously not a thing but leads to the confusion in that special case described there)

[Roukanken]

well it's more like because it's a static call from the JVM view

[Roukanken]

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

[Roukanken]

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...

[christophsturm]

I agree that both impls should check if the subject really is not null just to be sure

[robfletcher]

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!

[christophsturm]

its probably not worth the hassle

[robfletcher]

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

[christophsturm]

maybe with contracts?

[robfletcher]

hmm, I wonder yes

[Roukanken]

can't work with contracts, because:

expectThat(x) {
    isNotNull().somethingOnlyOnNonNullable()
    somethingOnlyOnNonNullable()
}

if first works because contracts, then the second has to work too (which it obviously can't)

[Roukanken]

(eg, compiler would not see difference between types of this@expectThat and isNotNull() )

[dave08]

isNotNull {.... }

[dave08]

That would show the intention too... I find myself writing isNotNull().and {} pretty often...