Yellow-Dog-Man / Resonite-Issues

Issue repository for Resonite.
https://resonite.com
140 stars 2 forks source link

Cannot write Type of Type to Data Model, stops ProtoFlux execution with exception #2522

Closed mpmxyz closed 4 months ago

mpmxyz commented 4 months ago

Describe the issue.

The write will neither succeed nor fail but the log shows the exception "Cannot assign non-data model type System.RuntimeType".

To Reproduce

  1. Create a data model store or similar world model variable of type Type.
  2. Attempt to write the type of a type to it.
  3. The Write node will not continue the impulse and an exception is logged.

Expected behavior

The write should succeed. The type of a type should be part of the world model. (maybe requires some conversion when doing so)

Screenshots

grafik "Type" worked. grafik Type of "Type" did not work.

Resonite Version Number

2024.7.10.1077

What Platforms does this occur on?

Windows

What headset if any do you use?

No response

Log Files

Anonymous - 2024.7.10.1077 - 2024-07-10 22_37_44.log

Additional Context

No response

Reporters

No response

Frooxius commented 4 months ago

This actually seems like correct behavior.

The exception says that it cannot write type System.RuntimeType into the data model. This is a distinct type from System.Type. It derives from it, but it's a separate type of it's own.

While the base System.Type is supported, System.RuntimeType is not.

We could consider adding it as a supported type as well - question is why do you need that specific subclass though? The type "Type" actually does work, so I think this is more just confusion on what the type of Type is - it's not Type, but RuntimeType. It is pretty confusing...

Does this break any existing creations?

Frooxius commented 4 months ago

I've made a small fix related to this - trying to write unsupported types will trigger the OnFail properly now.

mpmxyz commented 4 months ago

Yes, it broke a debugging tool I wrote when it was supposed to record information that happened to be a type value. Is the OnFail also properly handled with dynamic variables? That's what I'm actually using.

It also feels a bit contradictory that an inherent property of a world-model value is not part of the world model.

mpmxyz commented 4 months ago

I think a distinction between allowed types and allowed values could be appropriate here. Creating a variable of that type is probably too dependent on the implementation of types. Alternatively, does it make sense to hide this implementation detail by making the type of any type just "Type"?

Frooxius commented 4 months ago

Is the breakage because you can't write this specific type? Is it because the OnFail doesn't fire or because of other reasons?

What is the reason that you need to store a Type reference to the type of type?

I don't see how it's contradictory - any type (unless it's sealed) can have arbitrary number of derived types - doesn't mean that we allow all of them directly in the data model. You can still have fields that reference these through the base class, but not derived.

Consider that any derived type of allowed data model type would be implicitly allowed. We allow the object Type to be referenced and encoded. All types in C# derive from this - meaning that implicitly everything would be allowed - which is the exact thing we're trying to prevent.

Frooxius commented 4 months ago

I've made the change on 2024.7.10.1266, so it will now trigger OnFail instead of just doing an exception. Does that fix your tool?

mpmxyz commented 4 months ago

The original breakage was caused by the Exception. By chance the potentially affected code is resistant to OnFail-Writes but I'm using dynamic variables which are still broken: grafik (Beta 2024.7.10.1266! It also fails with Write Dynamic Variable.)

Frooxius commented 4 months ago

Oh fixing that one now!

Frooxius commented 4 months ago

Ok this is fixed in 2024.7.10.1338 now! I'm closing this issue now. Let me know if there are any more issues.

mpmxyz commented 4 months ago

On the topic of contradictions. I see it the following way: The problem of allowing fields/values in the world model is made up of two steps:

  1. Do all involved parties know of the type? Without knowledge of a type there is no chance of interacting with it.
  2. Can its meaning be preserved across the network? (values: transmitted directly, references: via RefID etc.)

(2) implies (1). There is no meaning in random bytes without knowing what kind of information it is supposed to be. object is a good example because it fulfills (1) but not (2) since certain subtypes - e.g. file streams - cannot be shared properly. This means a synced property of that type would be difficult since communicating it is not guaranteed. There are probably no IField<object>s due to this.

Type world model properties have now been changed to be properties of an imaginary DataModelType that is a subtype of Type with the confusing property that DataModelType is not a valid value of DataModelType.

PS: I'm doing a quick bug report on another type issue I found and then I'm back to check on the effects on my creation.

Frooxius commented 4 months ago

I think there's misunderstanding of what the type management system does. A type being a data model type means that data model can "refer" to it through that type.

It doesn't guarantee that you can actually transmit instances of that type over the network - that is separate from this - and there's narrower subset of types that are replicated.

You can do IField actually - this is a valid representation of a type in the data model. However you won't be able to obtain or put instances of arbitrary object into the data model. It only means you can "view" them as object.


Type world model properties have now been changed to be properties of an imaginary DataModelType that is a subtype of Type with the confusing property that DataModelType is not a valid value of DataModelType.

I am confused by this. There's not DataModelType? What do you mean by "Type world model properties"?

mpmxyz commented 4 months ago

So far I see it more of miscommunication than misunderstanding. (heavily on my side because I've got trouble converting my abstract and incomplete thoughts to words)

->There are probably no IFields due to this.

Adjust this to "There are probably no implementations of IField due to this.". IField being a valid type on its own is something I'm aware of and conforming to (1) but I failed to write it down properly.

->I am confused by this. There's not DataModelType? What do you mean by "Type world model properties"?

I started talking of types in a more theoretical way where types are sets and values are elements of those sets. So DataModelType is the name I gave to the set encompassing all types valid for the world model. It is by definition a subset of Type which is equivalent to being a subtype of it.

The part I'm concerned with is that this set does not contain itself or even subsets of it. (*) If one applied that to e.g. C# that would be the same as declaring that type values don't have a type.

To fix (*) I'm proposing to view all values of DataModelType to be of type Type without subtyping. This can either be integrated into the GetType node (early loss of information), the process of writing types to the data model (later loss of information) or done via an explicit operator ToDataModelType. The operator would take the type value and try to find a supertype that is a DataModelType. As of now you can't create type values of interfaces which are not a WorldModelType. Therefore inheritance of interfaces should not matter and the computation should be straightforward:

  1. get superclass until within WorldModelType
  2. guaranteed to end with object as it is within WorldModelType
  3. Does it need any special handling for primitive or delegate types?
Frooxius commented 4 months ago

I'm even more confused by this.

There's no subtyping going on at all with this. It's just a validation that puts types either on "Allowed" or "Not allowed" list. It has nothing to do with inheritance.

Banane9 commented 4 months ago

I think @mpmxyz got a bit carried away on the theoreticals there. The core problem I see here, which I think he's trying to point out, is the following: The type of the Type System.Type (=> System.RuntimeType) can't be saved into a value of the type System.Type. Or to put it in C# Terms, the protoflux above is failing to do this: Type dynVarValue = typeof(Type).GetType(). To put it in other terms: it can't store the Type of itself (because System.Type is just the base class and the instances are actually derived classes, which aren't on the game's whitelist).

Finally, his proposed fix is that in such cases the actually stored Type should be the first base class which is on the whitelist. E.g. in the case of System.RuntimeType, it would go up to System.Reflection.TypeInfo and then System.Type, which would be the stored value because it's allowed.

mpmxyz commented 4 months ago

I'm talking about the theoretical concept of a type. (The theory which is the basis for object oriented programming with all its fancy inheritance.) You don't create a "physical" C# type here but by making something like a set of types you create a - lets say "logical" - type. This is like the type of "all integer numbers from 0 to 2^31-1" which could be thought of as a subtype of the more general concept "number". (Java has this somewhat and Pascal does it strongly.)

Within the engine's code it is just a filter but from the perspective of a ProtoFlux developer it is a relevant type-like concept - even if it is never explicitly mentioned as one. It leaves a gap, literally: grafik (My code is running but that part of information is lost. As a workaround I will explicitly check if the input to GetType is a type and replace the type information with the base class Type.)

TLDR: Read WorldModelType as "on the 'Allowed' list". I just want to make sure that the language ProtoFlux doesn't get blindspots in its theoretical foundation.

Frooxius commented 4 months ago

@Banane9 Yes, I understand that, but that is correct behavior. You cannot reference types that aren't explicitly allowed in the data model - this is fully intentional behavior.

I do not think that "wrangling" the type to the nearest supported base type is a good idea. This can lead to unexpected and inconsistent behaviors.

E.g. consider stuff like types that aren't supported at all - you'd end up with a value typeof(object), which can be surprising and lead to "silent failures".

But it gets even weirder with generics. Consider for example Generic<UnsupportedType>. What do you do there? Change that to Generic<object>? That's very different type - one that's not even necessarily compatible!

Or even worse, what if you try to store UnsupportedType<ComplexSupportedType>? All that ComplexSupportedType now gets erased.

Just having it fail is the cleanest, simplest and least surprising thing we can do here.


@mpmxyz I'm sorry, but I just... I don't understand the stuff with the set of types and such and how it relates to this. Or how it even relates to ProtoFlux - this only affects what you can store in data model, but outside of the data model you can store anything.

Like I said, we can add System.RuntimeType into the actual supported list if this causes issues, that's not a big problem.

mpmxyz commented 4 months ago

Hmm... System.RuntimeType is a private implementation detail of the runtime's type system. I'm not sure if adding that makes sense. (A runtime could implement types completely differently like with multiple different subtypes instead of one.)

And don't stress yourself too much with trying to understand my ramblings! I had trouble boiling this down to specific, easier to digest examples. After a good night's rest I'd say that this shows up whenever the world model allows a node return value of an abstract/unsealed type which has implementations not covered by the 'allow' list. Another requirement is that the return type should not be a passthrough of a value already existing in ProtoFlux. Otherwise you get false-positives like ObjectCast<string,object> or Local<object>. Is anyone aware of any other examples than Type? Especially world model types instantiated via factory methods are a potential source for this issue.

I also view automatic conversions as potentially problematic which is why I'm more in favor of the operator ToDataModelBaseType.

That said it is probably best to just ignore the problem for now until it is more understood. (i.e. we have more examples) My workaround for types will be: (null==(Type)value)?GetType(value):System.Type

Once the update is shipped I will probably create code to actively search for those problematic types. I will share it and the results. (That assumes there is an easy way to query the list of data model types. I'd wish for a public method to get a read-only collection but just a test if a type is on the list would be enough to get this done.)

mpmxyz commented 4 months ago

As promised I spent too much time on this. lol (There is a summary of my findings below the walls of text/logs.)

C#-Code ```C# //Get all assembly types var allTypes = AppDomain.CurrentDomain.GetAssemblies() .SelectMany(assembly => { try { return assembly.GetTypes(); } catch (ReflectionTypeLoadException) { return Array.Empty(); } }) .Where((type) => type != null) .ToHashSet(); //Get this world's type manager var typeManager = world.Types; //Get all edges connecting types as supertype and subtype var typePairs = allTypes .SelectMany(type => { var generatedTypePairs = new List<(Type superType, Type subType)> { (type.BaseType, type) }; generatedTypePairs.AddRange(type.GetInterfaces().Select((i) => (i, type))); return generatedTypePairs; }) .Where(p => p.superType != null); //removes type arguments to avoid treating IField and IField differently Type StripTypeArgs (Type type) { if (type.IsGenericType) { return type.GetGenericTypeDefinition(); } return type; } //Group the edges by supertype var typeToSubTypes = typePairs .GroupBy(p => StripTypeArgs(p.superType), p => StripTypeArgs(p.subType)) .ToDictionary(g => g.Key, g => g.ToHashSet()); //Ignore direct descentants of object because it would bloat results with completely unrelated types. var ignoredBaseTypes = new HashSet { typeof(object) }; foreach (var typeSubTypes in typeToSubTypes) { var baseType = typeSubTypes.Key; var subTypes = typeSubTypes.Value; if (typeManager.IsSupported(baseType) && !ignoredBaseTypes.Contains(baseType)) { //Base type is supported. var nonSupportedSubTypes = subTypes.Where(subType => !typeManager.IsSupported(subType)).ToList(); if (nonSupportedSubTypes.Count != 0) { //Base type is supported but not the subtype. typeof(instance of subtype) cannot be persisted in the data model. UniLog.Warning($"Found DM base type with non-DM subtypes: {baseType}"); var supportedSubTypes = subTypes .Where((subType) => typeManager.IsSupported(subType)) .ToList(); if (supportedSubTypes.Count != 0) { UniLog.Warning($" Base type also has at least one DM subtype: {supportedSubTypes.Join()}"); } foreach (var nonSupported in nonSupportedSubTypes) { UniLog.Warning($" Non-DM subtype: {nonSupported}"); } } } } ```

This results in the following log output: DM-Analysis.txt

The problematic typing of this issue can be found in the first two lines:

Found DM base type with non-DM subtypes: System.Type
 Non-DM subtype: System.Reflection.TypeInfo

A look at a filtered list shows that there are a bunch of other base types with similar properties:

Filtered log output

This can be further summarized:

  • IFormatProvider has only has CultureInfo on the allow-list. The missing parts look fine to me. Please ignore NuGet which is there because I injected the code with MonkeyLoader!
  • IConnector is only DM-supported by some implementations. It looks so much like internals that I would not expect this to ever arrive within ProtoFlux.
  • A few ProtoFlux related types, mostly related to nodes: I have no idea if the non-DM-Types could ever be exposed. It looks unlikely.
  • ExecutionContext is indirectly implemented by the also DM-supportedFrooxEngineContext
  • Elements.Assets.SegmentedBuilder: All implementations are part of the DM.
  • System.Threading.Tasks.Task: I have no interpretation why that's part of DM but there are probably reasons for it. As long as no non-DM subclasses reach ProtoFlux there will never be any loss of type information.

type.GetType() apparently is the only thing that is really special with the world model unless you see a chance that any of the ProtoFlux or Task related types are exposed to ProtoFlux. Keep up the good work!

shiftyscales commented 4 months ago

Seeking further input / evaluation from @Frooxius on the above posts.

Frooxius commented 4 months ago

I'm not really sure how any of this really changes anything with the original issue? This feels like it's getting very off tangent, with lots of other stuff.

Like I mentioned above, we will not be implicitly allowing derived types (or even necessarily base types). That's not really a bug, it's how the system works - it doesn't really have anything to do with inheritance.

You'll be able to obtain and process other types in ProtoFlux that are outside of data model - but will not necessarily be allowed into the data model. That's expected behavior.

mpmxyz commented 4 months ago

I haven't expected any further changes since my tests hint that the "special" behaviour is only triggered by types of types in practice. And while confusing at first it can be learned like "You cannot call a method of a null value!" in other languages. It is not perfect but at least consistent behaviour. (Best kept consistent with whatever else also can't be persisted into the data model in the future!)