Make more Opcodes hold a FieldType

[C0D3-M4513R]

I wanted to include a more exhaustive test, but that was prevented by #41

[staktrace]

This doesn't look right to me. Conceptually the classinfo field contains the name of a class, which is different from a field type. For instance if the class happened to be called "B", parsing it as a field type would turn it into FieldType::Ty(Ty::Base(BaseType::Byte)) which is incorrect. On top of that this approach of handling Err and assuming it is an Ty::Object seems not very robust. Err can be returned for any number of reasons and is not a good way to handle control flow. You're basically hacking around the fact that ClassInfo structures contain a different syntax (e.g. java/lang/String) than what field type is intended to parse (Ljava/lang/String;) but this is not a good solution.

[C0D3-M4513R]

Sorry to circle back to this, but having to manually parse the Cow of anewarray, instanceof and checkcast for array depths also seems a little strange though

Like imagine casting an Object to an int[].

[staktrace]

Yeah like I said in the comment below, I'm not opposed to putting in a structured type here instead of the Cow. I'm just saying using FieldType is wrong, and it should be a different structured type that precisely represents the possible range of values here.

[C0D3-M4513R]

How is it wrong? can you not checkcast from Object to [J or [B?

[staktrace]

For instance if the class happened to be called "B", parsing it as a field type would turn it into FieldType::Ty(Ty::Base(BaseType::Byte)) which is incorrect.

[C0D3-M4513R]

Like I said I'm not at all opposed to this. I just want the implementation to use types with precision.

I proposed multiple solutions now, but you find none sufficient.
Tbh I do not know what you want from me at this point.

You say you see too many issues with my solution, so is there something I'm not seeing?

[C0D3-M4513R]

Also are you saying that the following sample opcode is wrong too? (also taken from #41)

21: checkcast #21 // class "[Ljava/lang/String;"

There's a number of problems with this code, starting with the fact that the class name in these instructions is not encoded with L...;.

[C0D3-M4513R]

[staktrace]

The code works when you use strip_prefix instead of split_prefix. I added the L...; bit as an extra bit of precaution, just to be sure.

fn main() {
    let ty = "[[[test";
    let mut ty = ty;
    let mut dims = 0;
    while let Some(new_ty) = ty.strip_prefix("[") {
        dims+=1;
        ty=new_ty;
    }
    
    let ty = ty.strip_prefix("L").unwrap_or(ty);
    let ty = ty.strip_suffix(";").unwrap_or(ty);
    
    println!("{ty}, {dims}");
}

Try this java code:

class L {
    public L tryCast(Object o) {
        L l = (L)o;
        return l;
    }
}

Look at what the checkcast instruction generates, the string value that you get, and what your parsing code does to it.

[C0D3-M4513R]

With my new parsing code (ReferenceType) I'd expect it to correctly resolve to the Object Variant of ReferenceType.

I did miss a couple of cases of FieldType though, that should be replaced with ReferenceType.

[staktrace]

I think this PR is fundamentally flawed because you're trying to use a FieldType out of convenience (i.e it represents approximately the things you care about) rather than out of correctness. If you need to have a more strongly typed operand here for the opcodes, I'd suggest creating a new type to represent that more exactly. Let me know if you disagree!

[C0D3-M4513R]

I think this is entirely justified.
See the bytecode in #41. You have some types there that entirely match the FieldType.

[staktrace]

I think this is entirely justified.

Are you referring to your PR, or my response?

See the bytecode in #41. You have some types there that entirely match the FieldType.

But would those types (in the case where they are valid) be used for these opcodes? And even if there is some overlap, I wouldn't want to use the type unless it is semantically equivalent, which it doesn't seem like it is. In my experience reusing code for syntactic/structural reasons usually ends up getting a mistake. I'm happy to explain more in detail if you'd like what I mean by that.

[C0D3-M4513R]

Col-E/Recaf#837 (comment)

[staktrace]

I implemented this to my satisfaction in #49.