Page 1 of 1

ZScript: Why is casting a readonly pointer not allowed?

PostPosted: Tue Nov 13, 2018 2:09 am
by Player701
I'm not sure if this is a bug, an unimplemented feature, or an intentional restriction, so I thought I'd ask here first. Please read the full post before answering, the real question is closer to the bottom. Sorry the post turned out to be that long :(

Example problem: I have a class name which I know is of type Actor, but I don't know anything else about it. I need to check if this class is an Inventory, and if it is, then print its default MaxAmount.

Seems easy, right? Let's use GetDefaultByType for this:
Code: Select allExpand view
class Test
{
    static void PrintMaxAmountIfInventory(name className)
    {
        let defaultInstance = GetDefaultByType((class<Actor>)(className));
        let inventory = Inventory(defaultInstance);
       
        if (inventory)
        {
            Console.Printf("Max amount of %s: %d", className, inventory.MaxAmount);
        }
        else
        {
            Console.Printf("%s is not of type Inventory.", className);
        }
    }
}

Now let's run it and...
Code: Select allExpand view
Script error, "zscript.txt:ZSCRIPT" line 6:
Cannot cast a readonly pointer

Oh noes! What has gone wrong?

Actually, this is completely understandable, so this is NOT the question - please read on. This error has appeared because the value GetDefaultByType returns is not an Actor but a readonly<Actor> - something akin to a const AActor* in C++ code. This is because we are not supposed to modify the instance GetDefaultByType has returned. We can only read its fields, and we can call its functions only if they are marked const. We can't write a new value to any field or call any non-const function. While I think it would be a useful feature to be able to change actors' defaults sometimes, that is a whole another story and thus also not the point of this thread.

Okay, so we can't cast the result to Inventory because it would violate const-correctness since Inventory is not readonly, and if we were allowed to do such a cast, then we could modify the instance. All right, let's try to cast the result to readonly<Inventory> then!
Code: Select allExpand view
... class and function declarations omitted

let defaultInstance = GetDefaultByType((class<Actor>)(className));
let inventory = (readonly<Inventory>)(defaultInstance);

if (inventory)
{
... rest omitted

Aaand...
Code: Select allExpand view
Script error, "zscript.txt:ZSCRIPT" line 6:
Unexpected 'readonly'
Expecting '-' or '+' or '++' or '--' or '(' or 'class' or identifier or string constant or 'super' or '~' or '!' or 'sizeof' or 'alignof' or integer constant or unsigned constant or float constant or name constant or 'false' or 'true' or 'null'

Well, now THIS is weird. Why not allow casting a readonly to another readonly? Const-correctness is almost certainly not violated in this case, as the resulting value would have the same restrictions on it. Okay, maybe we could at least check if the result is an Inventory with the is operator? (NB: It seems that this operator checks for exact type without accounting for inheritance, so it wouldn't suit our needs in this problem but I still want to see if it works)
Code: Select allExpand view
... class and function declarations omitted

let defaultInstance = GetDefaultByType((class<Actor>)(className));

if (defaultInstance is "Inventory")
{
... rest omitted

Let's see...
Code: Select allExpand view
Script error, "zscript.txt:ZSCRIPT" line 7:
Cannot convert Pointer<Class<Actor>readonly > to Pointer<Class<Object>>

Nope. We can't. Even though we don't even get a new value from this operator, so there isn't even a theoretical possibility to violate const-correctness in this case. The error message also doesn't seem to make much sense. "Pointer<Class<Object>>"? Why is that there?

Okay, well, I thought, there has to be some way to achieve what I want! And indeed there is. It turns out that I need to cast not the instance that is returned by GetDefaultByType but the class name. Then check if the resulting class is not null. The correct code looks like this:
Code: Select allExpand view
class Test
{
    static void PrintMaxAmountIfInventory(name className)
    {
        let inventoryClass = (class<Inventory>)(className);
        if (inventoryClass)
        {
            let inventory = GetDefaultByType(inventoryClass);
            Console.Printf("Max amount of %s: %d", className, inventory.MaxAmount);
        }
        else
        {
            Console.Printf("Class %s is not of type Inventory.", className);
        }               
    }
}

Yes, this method does sound logical. However, I would really like to know why it is not possible to cast a readonly<Actor> to a readonly<Inventory> or use the is operator on it. It doesn't look like it would make the world explode if it were allowed. Is it simply something that was never implemented because it was never considered, or is there a logical reason behind it?

Re: ZScript: Why is casting a readonly pointer not allowed?

PostPosted: Tue Nov 13, 2018 9:18 am
by Apeirogon
Mmmmm...its look like a bug, but why you use "let"? I missed part where you say why you do so.

Re: ZScript: Why is casting a readonly pointer not allowed?

PostPosted: Tue Nov 13, 2018 9:27 am
by Player701
Apeirogon wrote:Mmmmm...its look like a bug, but why you use "let"? I missed part where you say why you do so.

"let" is used to make the code shorter since I don't have to specify variable types explicitly. In this case, it doesn't affect how the code works (or doesn't). For example, this assignment is not allowed because GetDefaultByType returns a readonly pointer:
Code: Select allExpand view
Actor defaultInstance = GetDefaultByType((class<Actor>)(className));

However, this assignment is valid:
Code: Select allExpand view
readonly<Actor> defaultInstance = GetDefaultByType((class<Actor>)(className));

Re: ZScript: Why is casting a readonly pointer not allowed?

PostPosted: Tue Nov 13, 2018 10:13 am
by phantombeta
As you may see from the list of expected tokens that GZDoom spits out if you try to do a readonly cast, GZDoom simply does not support readonly casts. It's not a bug, it's just something that no one thought to code into the ZScript compiler.
ZScript has quite a few features that were either added as an afterthought or only added because gzdoom.pk3 needed it, and readonly pointers are likely one of the former.

Re: ZScript: Why is casting a readonly pointer not allowed?

PostPosted: Tue Nov 13, 2018 10:17 am
by Player701
Then I guess the inability to use the "is" operator on readonly pointers is also an unimplemented feature and not a technical restriction?

Re: ZScript: Why is casting a readonly pointer not allowed?

PostPosted: Tue Nov 13, 2018 10:22 am
by phantombeta
Indeed. Here's some answers to some of the other things about the "is" operator:
The error message also doesn't seem to make much sense. "Pointer<Class<Object>>"? Why is that there?

All classes inherit from Object, so it's very likely that it just expects a pointer to a class.
(NB: It seems that this operator checks for exact type without accounting for inheritance, so it wouldn't suit our needs in this problem but I still want to see if it works)

Quite the opposite, actually. The "is" operator does, in fact, check inheritance.

Re: ZScript: Why is casting a readonly pointer not allowed?

PostPosted: Tue Nov 13, 2018 10:23 am
by Player701
phantombeta wrote:Quite the opposite, actually. The "is" operator does, in fact, check inheritance.

Hmm... I think I checked it last time and it wasn't the case. Let me check again...

Edit: Yes, it's true. While testing, I forgot that there was a random factor involved and that messed up the results. I guess I have to account for it in my code and replace it with GetClassName or something, because there are places where I need to check for an exact class name. BTW, thanks for pointing it out :)

Edit #2:
phantombeta wrote:All classes inherit from Object, so it's very likely that it just expects a pointer to a class.

What I meant is that the error message doesn't quite make it obvious what the cause of the error is.

Re: ZScript: Why is casting a readonly pointer not allowed?

PostPosted: Tue Nov 13, 2018 3:55 pm
by Graf Zahl
A long wall of text - but in the end the simple answer is, it was simply overlooked. Of course this should exist, the main problem right now being that it has been over a year that I last did some work on the compiler. So don't expect a quick fix.

ZScript suffers somewhat from a grammar that was written without ever actually trying to test it on realistic code so what I got when I started was rather incomplete and/or broken. But since back then it was still ZDoom I didn't want to totally obliterate Randi's work, so many structural problems were not fixed until it was too late. The poor casting behavior is one of these victims.

In hindsight it might have been the better option to rip out some of the more questionable ideas in it right at the start. Then we wouldn't face this problem now.

EDIT: Looking at the code, the error message looks like a leftover for a condition that no longer applies. Can you test what happens if you just remove the check in FxDynamicCast::Resolve?

Re: ZScript: Why is casting a readonly pointer not allowed?

PostPosted: Tue Nov 13, 2018 5:02 pm
by Player701
Graf Zahl wrote:EDIT: Looking at the code, the error message looks like a leftover for a condition that no longer applies. Can you test what happens if you just remove the check in FxDynamicCast::Resolve?

Hmmm. You mean this check? It looks like it really does fix readonly casting if I remove it. And it even preserves const-correctness too, so when I do "Inventory(GetDefaultByType(...))", I get a readonly<Inventory> as a result. I'm not sure if everything works correctly, though, as I only tried to access one field of the returned Inventory instance (and it worked). Maybe I could test it some more tomorrow if there is a need for that.

The is operator still doesn't work, though (not surprising). I foolishly thought I could fix it myself, but all those macros and the immense length of codegen.cpp (over 11000 lines of code) make it very difficult to work with... :?

Re: ZScript: Why is casting a readonly pointer not allowed?

PostPosted: Thu Nov 15, 2018 10:52 am
by Player701
Sorry, yesterday was quite hectic and I couldn't find enough time for this.

So I've run a few more tests, tried to access different fields, call const-marked functions, pass pointers around. Everything seems to be fine. I guess that check can indeed be removed without breaking anything, but like I've said before, the actual code is quite difficult to understand and work with, so this shouldn't be taken for granted.

Re: ZScript: Why is casting a readonly pointer not allowed?

PostPosted: Thu Nov 15, 2018 12:46 pm
by Graf Zahl
That check was for something that no longer exists. Back in the old days the actor defaults had no class information but after starting with ZScript I quickly noticed that they NEED that info to work, or very bad things happen, so I had to run them all through actual object construction. This particular abort condition should have been removed back then.

Re: ZScript: Why is casting a readonly pointer not allowed?

PostPosted: Thu Nov 15, 2018 1:14 pm
by Player701
I see. Should I make a PR for it then, or will you remove it yourself?

Re: ZScript: Why is casting a readonly pointer not allowed?

PostPosted: Thu Nov 15, 2018 2:29 pm
by Graf Zahl
Whatever suits you best, I guess...

Re: ZScript: Why is casting a readonly pointer not allowed?

PostPosted: Thu Nov 15, 2018 3:41 pm
by Player701
All right. Here you go, then.

Re: ZScript: Why is casting a readonly pointer not allowed?

PostPosted: Fri Nov 16, 2018 9:24 am
by Player701
Tried to fix the is operator, seems to work, but not 100% sure it doesn't break anything. PR here.