godotengine / godotengine/godot-cpp

Shadowing _Wrapped members can cause incomprehensible errors

Open
#444 1 comment 0 reactions 0 assignees View on GitHub
enhancement
Dominant language
C++
Stars
2.7k
Forks
809
Avg merge
1d 3h
Merged PRs (30d)
8

Description

Just witnessed a case where someone accidentally put these two variables in a custom class:
```cpp
Variant _owner;
Variant _type_tag;
```
But it turns out every custom class inherit `Object`, which inherits `_Wrapped`. And `_Wrapped` contains this:
```cpp
// All classes inherit this
class _Wrapped {
public:
friend class detail::WrappedUtil;
godot_object *_owner;
size_t _type_tag;
};
```
I thought it would cause an error, but it didn't. Some places in the bindings access them from `Object*` so those would behave fine, but there are places where the context is a template, so the derived members get used. And it just "worked" because these derived `_owner` and `_type_tag` happen to be silently convertible from `Variant`. And then feeding null pointers and zero type tags to Godot.

This results in calls from GDNative to Godot functions being executed on null instances. For example, it was inheriting `Node2D`, calling `draw_circle`, and triggering an error in `VisualServer` saying the `CanvasItem` was not found, which is far from obvious.

So even though the user who wrote the shadowing variables didn't know what he was doing (added the two variables thinking it would "fix" a compiler error), I'm considering to put these variables private so it won't be possible to do that mistake.
There are two problems tho:
- Is anyone using them? The way I plan to hide them is actually moving their access point to specialized internal functions rather than just being naked members.
- These members are used in a fuckton of places. It's not a big deal to change, just many places to check.

This is what I was thinking to do:
```cpp
namespace detail {
struct WrappedUtil;
}

// All classes inherit this
class _Wrapped {
private:
friend class detail::WrappedUtil;
godot_object *_owner;
size_t _type_tag;
};

// Just so user code doesn't mess with these members.
// It used to be public but then it's easy to accidentally shadow and cause incomprehensible errors
namespace detail {
struct WrappedUtil {
static inline godot_object *get_owner(_Wrapped *w) {
return w->_owner;
}
static inline size_t get_type_tag(_Wrapped *w) {
return w->_type_tag;
}
static inline void set_owner(_Wrapped *w, godot_object *owner) {
w->_owner = owner;
}
static inline void set_type_tag(_Wrapped *w, size_t type_tag) {
w->_type_tag = type_tag;
}
};
} // namespace detail
```

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.