Announcement

Collapse
No announcement yet.

Awkward [ObjectLiteral] bug with generic types and custom Equals method

Collapse
X
  • Filter
  • Time
  • Show
Clear All
new posts

    Awkward [ObjectLiteral] bug with generic types and custom Equals method

    Long time, no post! :)

    I've been experimenting with using [ObjectLiteral(ObjectCreateMode.Constructor)] on types because I've found that we can get much better performance from the deserialiser that we're using. However, adding it on all of the API types has introduced a bit of a nasty bug in particular circumstances that involve combining generic types. The minimum reproduce case that I've boiled it down to looks like this:

    using System;
    using System.Linq;
    using Bridge;
    
    public class Program
    {
        public static void Main()
        {
            var model = new MultiLanguageTextBoxModel(
                new[] { new LanguageTranslation(new LangKey(1), "English") },
                new LangKey(1)
            );
    
            // If we get here then it's all good! But currently the ObjectLiteral LangKey means that the constructor call above fails
            Console.WriteLine("Selected: " + model.Selected.Value);
        }
    
        [ObjectLiteral(ObjectCreateMode.Constructor)]
        public struct LangKey
        {
            public LangKey(int value) { Value = value; }
            public int Value { get; }
            public bool Equals(LangKey o) => o.Value == Value;
            public override bool Equals(object o) => o is LangKey && Equals((LangKey)o);
            public override int GetHashCode() => Value.GetHashCode();
        }
    
        public sealed class MultiLanguageTextBoxModel : AbstractMultiLanguageTextBoxModel<LangKey, LanguageTranslation>
        {
            public MultiLanguageTextBoxModel(LanguageTranslation[] translations, LangKey selected) : base(translations, selected) { }
        }
    
        public abstract class AbstractMultiLanguageTextBoxModel<TKey, TTranslation> where TTranslation : Translation<TKey>
        {
            public AbstractMultiLanguageTextBoxModel(TTranslation[] translations, TKey selected)
            {
                if (translations.Length == 0)
                    throw new Exception("There must be at least one Translation specified");
                if (!translations.Any(t => t.Key.Equals(selected)))
                    throw new Exception("The Selected value does not correspond to an entry in the Translations list");
                Translations = translations;
                Selected = selected;
            }
            public TTranslation[] Translations { get; }
            public TKey Selected { get; }
        }
    
        public sealed class LanguageTranslation : Translation<LangKey>
        {
            public LanguageTranslation(LangKey key, string text) : base(key, text) { }
        }
    
        public abstract class Translation<TKey>
        {
            public Translation(TKey key, string text)
            {
                Key = key;
                Text = text;
            }
            public TKey Key { get; }
            public string Text { get; }
        }
    }
    The code is available at https://deck.net/21366607cfb2703a4cb35523dad6f7d7 and an error may be seen in the console, caused by the custom "Equals" method on LangKey not being called by the "Any" code.

    If the [ObjectLiteral(ObjectCreateMode.Constructor)] attribute is removed then everything works as expected.

    If the code is changed such that the Translation<TKey> class is no longer generic then it also works (even with the ObjectLiteral attribute).

    The same code can be seen in .NET Fiddle, where it doesn't throw the error: https://dotnetfiddle.net/oelGjS

    #2
    I have another issue that I imagine is related.. I wasn't sure if it was better to include it here or to create a separate report for it - apologies if you would have preferred a separate report!

    With the code below, I would expect the output to be this:

    AbstractMultiLanguageTextBoxModel.ctor: Selected = 1
    model.Selected: 1
    .. but, instead, I get this:

    AbstractMultiLanguageTextBoxModel.ctor: Selected = System.Object
    model.Selected: 1
    If I removed the ObjectLiteral attribute from LangKey then it works correctly.

    using System;
    using Bridge;
    
    public class Program
    {
        public static void Main()
        {
            var model = new MultiLanguageTextBoxModel(new LangKey(1));
            Console.WriteLine("model.Selected: " + model.Selected.ToString());
        }
    
        [ObjectLiteral(ObjectCreateMode.Constructor)]
        public struct LangKey
        {
            public LangKey(int value) { Value = value; }
            public int Value { get; }
            public override string ToString() => Value.ToString();
        }
    
        public sealed class MultiLanguageTextBoxModel : AbstractMultiLanguageTextBoxModel<LangKey>
        {
            public MultiLanguageTextBoxModel(LangKey selected) : base(selected) { }
        }
    
        public abstract class AbstractMultiLanguageTextBoxModel<TKey>
        {
            public AbstractMultiLanguageTextBoxModel(TKey selected)
            {
                Selected = selected;
                Console.WriteLine("AbstractMultiLanguageTextBoxModel.ctor: Selected = " + Selected.ToString());
            }
            public TKey Selected { get; }
        }
    }
    Also available at https://deck.net/92e2691a92b89ceb3def06b3b413897f and https://deck.net/92e2691a92b89ceb3def06b3b413897f

    Comment


      #3
      Hello ProductiveRage! Sorry for the delay replying your inquiry!

      Well, the ObjectLiteral attribute is used to simplify the output object in javascript. This may have a good side of simpler, cleaner (and well, faster) code. But it is not usable in all cases due to its simplified nature.

      There's a good discussion on usage of the attribute in this forum thread: ObjectLiteral vs "regular" objects

      And this particularly relevant quote in the same thread:

      Originally posted by geoffrey.mcgill
      Generally this would be an advantage if you know you that you only require a simple object during runtime.
      In other words, if you will to use the object in its full C# compatibility, ObjectLiteral is probably not for you. If you still think you need it, then probably what you need is to have an auxiliary class you copy the values from one to the other (keep them "identical"), and then use each one at the correct time. I can't really figure a scenario where this would be true; but would be something like passing the result (or the state) of the object to a 3rd party javascript (client-side, external) library, yet being able to fully operate the object until it gets to the desired state.

      Any code involving ObjectLiteral is likely not to be comparable to a corresponding code in native .NET, simply due to the fact that there's no thing like ObjectLiteral in .NET.

      So, I'm not sure we should even consider this a bug or not at all. The simplified version does lose .NET functionalilty to some extent, not sure this level should be considered or not... I'm tending to believe that'd be expected not to work from an ObjectLiteral.

      Hope this helps! Please share your thoughts if you think this should really be a bug that we should fix.

      Comment


        #4
        Hmmmm.. well, I am relying upon (or hoping to!) the full C# compatibility with [ObjectLiteral] types. As I said, I'm using it for the performance improvement possible in deserialisation, rather than because I want a "simple" type to pass to an external library.

        I will propose the following counter arguments to your suggestion that it is not a bug and you guys let me know what you think! :)

        1. Some "full C# compatibility" functionality IS supported on [ObjectLiteral] types, such as overriding Equals and GetHashCode. In the following code:

        public class Program
        {
            public static void Main()
            {
                var x1 = new LangKey(1);
                var x2 = new LangKey(1);
                Console.WriteLine(x1.Equals(x2));
                Console.WriteLine(x1);
            }
        }
        
        [ObjectLiteral(ObjectCreateMode.Constructor)]
        public struct LangKey
        {
            public LangKey(int value) { Value = value; }
            public int Value { get; }
            public override string ToString() => Value.ToString();
            public override int GetHashCode() => Value;
            public override bool Equals(object other) => (other is LangKey otherLangKey) && (otherLangKey.Value == Value);
        }
        .. the "x1.Equals(x2)" call is correctly (in my opinion!) translated into the following JavaScript:

        Demo.LangKey.prototype.equals.call(x1, x2)
        (See https://deck.net/6873c2a5ed6496ebfd694d3f481e4c9d)

        Had the LangKey struct not had an overridden Equals method then the Equals call would have been translated into this:

        Bridge.equals(x1, x2)
        I think that it is inconsistent to support overriding Equals but not support overriding ToString.

        2. If the argument above doesn't win you over and you do decide that [ObjectLiteral] types should not support all of the C# functionality (such as overriding ToString) then I think that it would be really good to warn about this at compile time, rather than leaving it as something unpleasant to discover at runtime (as I have done!). The ideal scenario would be an analyser so that VS could warn in "real time" but it would still be a great improvement for the Bridge compiler to throw an error if an [ObjectLiteral] has a "ToString" override to let the developer know that that will not work.

        Comment


          #5
          I've been having a play around with this some more. I'm keen to be able to use [ObjectLiteral] in our API responses because some of them involve many objects and deserialising into [ObjectLiteral] results in much better performance.. but I'm also keen on being able to override Equals, GetHashCode and String so that we can use these types throughout the application (it's important for React performance that component properties be comparable so that React can avoid re-rendering if the "new" values are the same as the old one and we use custom Equals methods on the types to achieve that).

          So, I'm hoping that you don't decide to go for the "Idea Number 2" in my last post, which was to fail at compile time if any [ObjectLiteral] type has overridden Equals, GetHashCode or ToString methods!

          It looks like your current approach to tackling custom ToString, etc.. methods on [ObjectLiteral] types is to try to determine at translation time when a custom method should be called instead of bridge.toString or bridge.equals or bridge.getHashCode but this is not possible when the type in question is a generic type param. I wondered if it would be possible to handle it at runtime by having "bridge.toString" (or whichever method) look at the type meta data of the target, even if it is an [ObjectLiteral] and to use the overridden method if one is defined?

          I've added a workaround to one of my libraries to try this out (and to get my project working) - see

          https://github.com/ProductiveRage/Br...lEqualsHack.cs

          This sort of code makes me nervous because it has some dependency on your internal code! On the other hand, it has enabled me to progress. I wondered what your thoughts were on performing the checks at runtime, like this?

          Comment

          Working...
          X