Smart cast and overridden properties

1

Smart cast seems not to work in situation like this one:

override var mesh:MeshDisplay? = null
    set(value) {
        super.mesh = value

        if(field == null) {
            texture = null
        }
        else {
            texture = field!!.textures[0] // can't assume 'field' is not null, but NPE is impossible here, isn't it?
        }
    }

It looks like it treats field the same way as calling super.mesh, but shouldn’t field allow for direct backing variable access?

2

It’s not impossible for the field to be null in the else branch if this is called concurrently.

3

I think the easiest fix in this case would be

super.mesh = value
texture = field?.textures[0]

This way, if field is null texture is null, otherwise you get texture[0]. You’re right though - if you aren’t used to thinking about concurrency issues (I’m not), it’s confusing.

Actually though, I would’ve thought that a separate thread calling it wouldn’t change what field refers to; it’d be a separate execution “instance”, wouldn’t it?

4

Actually, ignore the last part of my comment - for some reason I was thinking of field as a parameter (i.e. value instead). Field can definitely get changed by concurrent threads.

(I’m leaving my previous comment instead of editing to remind me to pay a little more attention :slight_smile:)

5

That’s my point exactly :slight_smile: but compiler does not know that, somehow. I wonder if this is a bug or am I just missing something here? If field was a dynamic property (which it is not, is it?) there would be a possibility that the returned value changes during the get() call (if(field == null)), but I think it’s supposed to be just a field, as the name and docs suggest.

Hmm, I don’t think that creating a thread safe code should be relevant in this case (if I wanted this method/property to be thread safe, I’d add @Synchronized). I think that what the compiler assumes is that the property can change during its accessor call (which is true, but I’m not accessing the property this way here).

edit: @mplatvoet, I somehow missed the “not” in your previous post :slight_smile:

6

I think I’m missing you’re point then, ergo I don’t understand what you’re saying.

I thought you where saying that in your previous example it was impossible for the value of field in the else branch to be null. That’s the part I reacted to, it can be null if this code is called concurrently. Therefor the compiler can’t/shouldn’t make assumptions about the state of the field.

Hmm, I don’t think that creating a thread safe code should be relevant in this case (if I wanted this method/property to be thread safe, I’d add @Synchronized).

If you make it (provable) threadsafe maybe the compiler could avoid the second null check, though it’s not trivial prove that statically. Even in this case only adding @Synhronized to this overridden property can’t guarantee threadsafety because the super property should be guarded too with the same lock.

A reasonable sound approach for these cases is introducing a local val like so:

override var mesh:MeshDisplay? = null
    set(value) {
        super.mesh = value

        val current = field
        if(current == null) {
            texture = null
        }
        else {
            texture = current.textures[0]
        }
    }

Note that this is just an example to satisfy the compiler, not threadsafety.

7

This isn’t supposed to be thread-safe :slight_smile: (and I doubt it’s what the compiler complains about) - it’s about smart casts and that they for some reason don’t work in this scenario. The compiler says it’s not guaranteed field is not null in the else-branch, because it’s a dynamic property that could’ve changed. But it couldn’t!

You’re mentioning concurrency, but does the compiler assume everything has to be thread-safe? I don’t think so (this makes very little sense, because, in general, all code doesn’t have to be thread-safe). And apart from that there are no other ways of changing field’s value - it can only be changed by directly assigning another value to it OR by super.get()/set() call - both of these things aren’t there.

9

The compiler doesn’t know you’re writing code that doesn’t need to be thread safe; it’s working on trying to ensure that your code is thread safe. All it knows is that your trying to dereference a mutable and nullable field, and it doesn’t like that. If you’re not concerned with thread safety (i.e. you know it’s always going to be non-null in that instance), you can force the dereference like you did with !!. An alternative would be to use the safe access operator - field?.textures[0] - which should do the trick and keep the compiler happy.

1 Like