Search Unity

[Feature Request] Use new DiagnosticSuppressor API to suppress CS0649 on SerializeField

Discussion in 'Editor & General Support' started by renatop, Jun 19, 2019.

  1. renatop

    renatop

    Joined:
    Aug 7, 2018
    Posts:
    5
    Roslyn merged a new analyzer API, DiagnosticSuppressor, that allow programatic suppression of warnings. The pull request can be seen here: https://github.com/dotnet/roslyn/pull/36067. It closes an issue that was open specifically about Unity's SerializeField: https://github.com/dotnet/roslyn/issues/30172.

    It looks like now Unity just needs to update its compiler version and implement a DiagnosticSuppressor to suppress CS0649. A new version of Roslyn is probably not released yet since this was merged yesterday, but I thought I should let you guys know.

    And please add this to 2018 LTS!
     
    Last edited: Jun 19, 2019
  2. FlaSh-G

    FlaSh-G

    Joined:
    Apr 21, 2010
    Posts:
    195
    +1 on this one!

    Especially +1 for addition to LTS versions.
     
  3. Michael-Ryan

    Michael-Ryan

    Joined:
    Apr 10, 2009
    Posts:
    149
    +1 for 2018 LTS.

    I’ve been waiting for this fix. Hopefully it will be included in Unity soon.
     
  4. bdovaz

    bdovaz

    Joined:
    Dec 10, 2011
    Posts:
    688
  5. Greyborn

    Greyborn

    Joined:
    May 26, 2016
    Posts:
    33
    2018 LTS please!
     
    Michael-Ryan likes this.
  6. Vallar

    Vallar

    Joined:
    Oct 18, 2012
    Posts:
    155
    Definitely +1 and yes 2018 LTS please!
     
    Michael-Ryan likes this.
  7. Baste

    Baste

    Joined:
    Jan 24, 2013
    Posts:
    4,286
    Yup!

    This was made very much with the problem Unity had in mind. Pinging @JoshPeterson, @joncham and @LeonhardP as they were the people responding in the original thread. Will you be implementing this feature in the engine?

    I'm not quite sure how this works, but I assume that this won't land in LTS - it requires updating the Roslyn version, which doesn't really sound very LTS-y.
     
    Rallix, Lahcene, rob8861 and 3 others like this.
  8. IcyHammer

    IcyHammer

    Joined:
    Dec 2, 2013
    Posts:
    54
  9. YD_JMysior

    YD_JMysior

    Joined:
    Aug 4, 2016
    Posts:
    21
  10. abuki

    abuki

    Joined:
    Nov 14, 2013
    Posts:
    39
  11. rob8861

    rob8861

    Joined:
    Sep 25, 2015
    Posts:
    77
    Can this thread get a reply from Unity. This is one of biggest annoyance I've ever dealt with - needing to "decorate" my code with dumb #pragma statements.
     
    Lahcene likes this.
  12. Kamyker

    Kamyker

    Joined:
    May 14, 2013
    Posts:
    74
  13. FlaSh-G

    FlaSh-G

    Joined:
    Apr 21, 2010
    Posts:
    195
    Code (CSharp):
    1. [SerializeField]
    2. private Thing thing = default;
    Until we get this, use the default keyword to explicitly initialize the field with a default value. The requested feature would allow us to skip this as well, but until then, it's definitely better than pragma statements.
     
    Lahcene likes this.
  14. Sylmerria

    Sylmerria

    Joined:
    Jul 2, 2012
    Posts:
    250
  15. Yemnefer

    Yemnefer

    Joined:
    Sep 12, 2017
    Posts:
    1
    my 400+ 0649 warnings will go away for good.
     
  16. Xarbrough

    Xarbrough

    Joined:
    Dec 11, 2014
    Posts:
    519
    From the descriptions, I would say I want this. Would this be something that only Unity can add to the compiler or would it be possible for users to modify their local settings? If possible, we could start testing this.
     
  17. Baste

    Baste

    Joined:
    Jan 24, 2013
    Posts:
    4,286
    We need to have a compiler which supports this feature.
    The feature was merged into one of Roslyn's feature branches on the 19th of June, and was merged from there into Roslyn's master branch. The last Roslyn release was on the 21th of May. So unless Unity's pulling Roslyn directly from Github instead of using official releases, it'll be a while until this will be available in Unity.
     
    Lahcene and Xarbrough like this.
  18. ethankennerly

    ethankennerly

    Joined:
    Dec 25, 2015
    Posts:
    1
    +1 and 2017 LTS please.

    I appreciate the solution at the root cause.

    Otherwise, using the default assignment causes some compilers in IDEs, like JetBrains Rider, to warn that a variable was redundantly assigned the default value.
     
    Michael-Ryan likes this.
  19. Michael-Ryan

    Michael-Ryan

    Joined:
    Apr 10, 2009
    Posts:
    149
    I'm not sure if it can also be addressed with the Roslyn update, but I often see Visual Studio or ReSharper recommendations to convert simple Properties (that just return field values) to auto-properties even when the field has the [SerializeField] attribute.
    Code (CSharp):
    1.       [SerializeField]
    2.       private string descriptor;
    3.  
    4.       public string Descriptor => this.descriptor;
    That recommendation is desired when dealing with a regular field, however fields that have the [SerializeField] attribute must not be converted to an auto-property. Doing so would remove the field from the Inspector.

    I would love to see this recommendation suppressed when the property is associated with a field marked with [SerializeField].
     
  20. Baste

    Baste

    Joined:
    Jan 24, 2013
    Posts:
    4,286
    That's not something that can be done on Unity's side, it has to happen on Jetbrain's side. It's already an issue on their Github, so they're aware of it.

    Thing is, this works:

    Code (csharp):
    1. [field: Serializefield]
    2. public string Descriptor { get; private set; }
    Though the result gives a garbled look in the inspector. So it's not completely wrong to convert that property, given that you're on a Unity version that supports attributes targeting backing fields (C# 7+)
     
  21. Michael-Ryan

    Michael-Ryan

    Joined:
    Apr 10, 2009
    Posts:
    149
    That's interesting. I read about the field-targeted attributes recently, but haven't used them at all yet. I'm assuming any custom inspectors or SerializedObject.FindProperty() calls will need to use the internal field name generated for the auto-property. Is that correct?

    My project is using Odin, and field-targeting can be used on the [LabelText] attribute to change the appearance of the field in the inspector. Based on the following code, the "<Field Targeted Property 2>k__Backing Field" becomes "FieldTargetedProperty2" when viewed in the Inspector.

    Code (CSharp):
    1. public class FieldTargetAttributeTest : MonoBehaviour
    2. {
    3.    [SerializeField]
    4.    private int serializedField;
    5.  
    6.    [field: SerializeField]
    7.    public int FieldTargetedProperty1 { get; }
    8.  
    9.    [field: SerializeField]
    10.    [field: LabelText(nameof(FieldTargetedProperty2))]
    11.    public int FieldTargetedProperty2 { get; }
    12. }
    devenv_2019-07-18_11-10-59.png

    As you can see in the above code and screenshot, there are some properties with field-targeted attributes, which show up in the Inspector in "normal" mode, however they do not appear in the Inspector "debug" mode. That's where I would expect to see the internal field name generated by the auto-property. Why would it be missing here?

    Update: Adding a setter to the auto-property (even a private one), will cause the field to appear in the "debug" Inspector. The following code will appear in the "debug" Inspector as, "<FieldTargetedProperty1>k__BackingField".

    Code (CSharp):
    1.    [field: SerializeField]
    2.    public int FieldTargetedProperty1 { get; private set; }

    One downside to the field-targeting attribute on the auto-property is that the field as actually serialized using the generated backing field name. If the internal implementation of backing field names were to change, the serialized data might not be readable.
    Code (csharp):
    1. MonoBehaviour:
    2.   m_ObjectHideFlags: 0
    3.   m_CorrespondingSourceObject: {fileID: 0}
    4.   m_PrefabInstance: {fileID: 0}
    5.   m_PrefabAsset: {fileID: 0}
    6.   m_GameObject: {fileID: 5938582256316890316}
    7.   m_Enabled: 1
    8.   m_EditorHideFlags: 0
    9.   m_Script: {fileID: 11500000, guid: 0b47428690ea70f4689c760c981d385b, type: 3}
    10.   m_Name:
    11.   m_EditorClassIdentifier:
    12.   serializedField: 0
    13.   <FieldTargetedProperty1>k__BackingField: 111
    14.   <FieldTargetedProperty2>k__BackingField: 222
    Also, while I was able to access the auto-property backing fields from a custom Inspector script, changes to the serialized properties would not save for me. They would look normal in the Inspector, but after making changes to the field and hitting "Enter", the value would revert back to its previous state. I tried changing the property setting to public (which would really defeat the point of using auto-properties in the first place), but that didn't seem to make a difference. Changes were not recorded.
    Code (CSharp):
    1. [CustomEditor(typeof(FieldTargetAttributeTest))]
    2. public class FieldTargetAttributeTestInspector : Editor
    3. {
    4.    private SerializedProperty p1;
    5.  
    6.    private SerializedProperty p2;
    7.  
    8.    private FieldTargetAttributeTest targetObject;
    9.  
    10.    public override void OnInspectorGUI()
    11.    {
    12.       // The following fields appears in the inspector, but changes made to them don't stick.
    13.       EditorGUILayout.PropertyField(this.p1, new GUIContent(nameof(this.targetObject.FieldTargetedProperty1)));
    14.       EditorGUILayout.PropertyField(this.p2, new GUIContent(nameof(this.targetObject.FieldTargetedProperty2)));
    15.    }
    16.  
    17.    private static FieldInfo GetBackingField(object obj, string propertyName)
    18.    {
    19.       return obj.GetType().GetField(
    20.          GetBackingFieldName(propertyName),
    21.          BindingFlags.Instance | BindingFlags.NonPublic);
    22.    }
    23.  
    24.    private static string GetBackingFieldName(string propertyName)
    25.    {
    26.       return $"<{propertyName}>k__BackingField";
    27.    }
    28.  
    29.    private void OnEnable()
    30.    {
    31.       this.targetObject = this.target as FieldTargetAttributeTest;
    32.  
    33.       this.p1 = this.serializedObject.FindProperty("<FieldTargetedProperty1>k__BackingField");
    34.       this.p2 = this.serializedObject.FindProperty(
    35.          GetBackingFieldName(nameof(this.targetObject.FieldTargetedProperty2)));
    36.  
    37.       // The following will not compile, because the property setter is private.
    38.       ////this.targetObject.FieldTargetedProperty2 = -1;
    39.  
    40.       // Using reflection, you can still set the value of the property with a private setter.
    41.       var fieldInfo = GetBackingField(this.targetObject, nameof(this.targetObject.FieldTargetedProperty2));
    42.       fieldInfo.SetValue(this.targetObject, -1);
    43.    }
    44. }
    At this point, I'll probably stick with using explicit private fields with the [SerializeField] attribute and wait for JetBrains to suppress the "convert to auto-property" suggestion.

    This post has clearly gone off on a tangent, so that's all I'll say on the matter.

    And now back to our regularly scheduled programming ...
     
    Last edited: Jul 18, 2019
  22. dmax1

    dmax1

    Joined:
    Jun 10, 2015
    Posts:
    4
    +1 for 2018 LTS
     
    Michael-Ryan likes this.
  23. Lahcene

    Lahcene

    Joined:
    Jun 18, 2013
    Posts:
    22
    Any news? Is Roslyn gonna be updated?
     
  24. Michael-Ryan

    Michael-Ryan

    Joined:
    Apr 10, 2009
    Posts:
    149
    @renatop, does this thread have the FEEDBACK tag?

    I’d love for someone at Unity to comment here.
     
  25. rob8861

    rob8861

    Joined:
    Sep 25, 2015
    Posts:
    77
  26. renatop

    renatop

    Joined:
    Aug 7, 2018
    Posts:
    5
    Didn't put any tags :/ And it seems I'm not able to add tags now.
     
  27. SmilingCatLtd

    SmilingCatLtd

    Joined:
    Jun 8, 2013
    Posts:
    6
    +149 (+1 for each spurious warning this would squelch)
     
  28. mnarimani

    mnarimani

    Joined:
    Mar 27, 2017
    Posts:
    212
    +1
    Please add this as soon as possible.
     
  29. tarmo-jussila

    tarmo-jussila

    Joined:
    Jun 4, 2015
    Posts:
    6
    Bump. Definitely hoping to see this feature implemented with an upcoming Unity version soon.
     
  30. RKOvlesen

    RKOvlesen

    Joined:
    Oct 29, 2013
    Posts:
    7
    We have a large shared codebase at our company and right now I'm getting over 200 warnings from this.

    Having to go through everything and set a default value for all the applicable variables would be a huge waste of time.
     
  31. Mese

    Mese

    Joined:
    Dec 13, 2015
    Posts:
    14
    450+ Warnings
    +450 for this feature
     
    Yozaro likes this.
  32. rob8861

    rob8861

    Joined:
    Sep 25, 2015
    Posts:
    77
    does Unity even care? I doubt they do...
    they were so quick to close the original ticket and mark it "work as intended"
     
    gyltefors likes this.
  33. gyltefors

    gyltefors

    Joined:
    Apr 5, 2013
    Posts:
    45
    Well, what else to expect from Unity? Seriously!
     
  34. bdovaz

    bdovaz

    Joined:
    Dec 10, 2011
    Posts:
    688
    I find it unprofessional that they don't even show up to make any kind of comment.

    If they don't solve this, I don't really know what function the console performs now, because with the spam we now have from dozens of warnings, nobody is going to bother looking at anything.
     
  35. Lorash

    Lorash

    Joined:
    May 6, 2019
    Posts:
    23
    Almost 3 months and not even a "hey this looks nice but we're too busy with other stuff" or a "we won't do this lol"
     
  36. FontouraCollide

    FontouraCollide

    Joined:
    Nov 15, 2016
    Posts:
    11
    +560 (warnings/votes)
     
  37. Jannify

    Jannify

    Joined:
    May 24, 2017
    Posts:
    1
    +1 Pls
     
  38. Flavelius

    Flavelius

    Joined:
    Jul 8, 2012
    Posts:
    649
  39. Bernarche

    Bernarche

    Joined:
    Aug 19, 2014
    Posts:
    1
  40. codestage

    codestage

    Joined:
    Jul 27, 2012
    Posts:
    1,269
    Looking forward to this to be included into 2018 LTS or at least 2019 LTS, would be nice to hear any news from Unity regarding this issue.