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

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!

 

+1 on this one!

Especially +1 for addition to LTS versions.

 

+1 for 2018 LTS.

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

 

https://gist.github.com/mavasani/fcac17a9581b5c54cef8a689eeec954a#development-experience-example

 

2018 LTS please!

 

Definitely +1 and yes 2018 LTS please!

 

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.

 

+1

 

+5!

 

+6

 

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.

 

+1

 

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.

 

+1

 

my 400+ 0649 warnings will go away for good.

 

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.

 

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.

 

+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.

 

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].

 

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+)

 

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. }

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 ...

 

+1 for 2018 LTS

 

Any news? Is Roslyn gonna be updated?

 

@renatop, does this thread have the FEEDBACK tag?

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

 

bump

 

Didn't put any tags :/ And it seems I'm not able to add tags now.

 

+149 (+1 for each spurious warning this would squelch)

 

+1
Please add this as soon as possible.

 

Bump. Definitely hoping to see this feature implemented with an upcoming Unity version soon.

 

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.

 

450+ Warnings
+450 for this feature

 

does Unity even care? I doubt they do...
they were so quick to close the original ticket and mark it "work as intended"

 

Well, what else to expect from Unity? Seriously!

 

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.

 

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"

 

+560 (warnings/votes)

 

+1 Pls

 

+1

 

+1

 

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.

 

It always amaze me how devs of Unity, a programmer-oriented game engine, seem to ignore the essential fixes/improvements for writing code properly with Unity, dismissing them as something "non-important". What can be more important than enabling your customers to use your product properly? Fancy marketing presentations I guess...

 

Oh come on... I get hundreds of warnings for what... proper encapsulation?
If this doesn't get fixed, console warnings are useless for me and everyone else using the [SerializeField] attribute. I pretty much stopped checking for warnings between these loads of pointless ones.

I've got some hope when I googled this issue. But then I saw the resolution... BY DESIGN

 

We are aware of this issue and we will add support for Roslyn analyzers/suppressors in a future version of Unity.

Meanwhile, you can disable the warnings with pragmas:

#pragma warning disable 0649
// your code
#pragma warning restore 0649

Or disable it globally by adding a file named csc.rsp to the root of you Assets folder, e.g. Assets/csc.rsp, with the following contents. Also attached to this post as a zip file because .rsp extensions are not allowed

/nowarn:0649

This will tell the C# compiler to not emit a warning for CS0649 / unused fields.

If you have .asmdefs with csc.rsp in your project, then you also need to add the "/nowarn:0649" line to those csc.rsp files.

I've also updated the linked issue "[SerializedField] fields produce "Field is never assigned to..." warning" with these details.

 

That's amazing! Thanks for the in-depth guide, specially with regards to the global solution. This is something that was bothering me quite a lot.

You rock Lukasz!

 

What's required for that to land? Since it's a Roslyn feature, and you're using Roslyn these days, I'm guessing that it's only a question of updating your Roslyn version, as well as piping the analyzers to the compiler? Or is there some other complexity I'm missing?

 

Using csc.rsp (C# Compiler Response File) you can pass additional arguments to the Roslyn C# compiler. You can also add /analyzer:<path to .dll> to file, similar to /nowarn:0649 and it would use the analyzers you pass it.

You can for instance add the Microsoft.NetCore.Analyzers .dlls to your project and mark them as being incompatible with all platforms in the Plugin Inspector. And then add e.g. "/analyzer:Assets/Microsoft.NetCore.CSharp.Analyzers.dll" and the analyzer would run as part of compilation.

I've attached a small Unity project that shows how to do this. You can edit the Test.cs file to get the analyzer messages, which are also shown in the attached screenshot.

The issue is that the workflow currently with csc.rsp is not very nice and you can overwrite csc.rsp options for each asmdefs. We need a solution that works for all assemblies in the project, but also allows you to opt-out of analyzers and has nicer worflow. And then we also need to develop and ship the Roslyn suppressor

 

Looks great, @lukaszunity, thanks !

Looking forward to the Unity's Roslyn suppressor to let us keep using proper encapsulation without warnings.

I wonder though if it's possible to similarly add any external code analyzing tools to the compilation process like inspectcode.exe from the JetBrains ReSharper Command Line Tools as well?

That would be super helpful to be able to see output from the such analyzing tools in the console and even prevent build process in case of errors found by the analyzers ^^

 

On CI you can use inspectcode https://github.com/JetBrains/resharper-unity/issues/536#issuecomment-458973836

 

Thanks, I already added Unity support to the R# CLT and calling it manually from the editor scripting, but my question was - if it's possible to natively integrate inspectcode into the compilation process via Roslyn "analyzer" argument to see inspectcode output at the console ^^

 

You should probably also remove the "By design" status. It's what got most people upset in the first place. They look up for a solution and see this.