New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Add [FormerlySerializedAs($fieldName$)] attribute when using Refactor-Rename on serialized fields #54

Closed
sindrijo opened this Issue Oct 20, 2016 · 13 comments

Comments

Projects
None yet
10 participants
@sindrijo
Copy link

sindrijo commented Oct 20, 2016

Renaming serialized fields in Unity can result in loss of data/serialized references if they are not marked with a FormerlySerializedAs attribute to denote their previous names.

Ideally ReSharper should add it when doing a rename refactor, such that renaming both fields below...
beforerefactor
... would result in something like this:
afterrefactor

See https://docs.unity3d.com/ScriptReference/SerializeField.html for a list types that are serializable, though it is missing some info: Classes marked with the System.Serializable attribute can also be serialized (provided they contain serializable types)

If renaming a field that already has a FormerlySerializedAs attribute it should append add a new attribute
FormerlySerializedAs attribute (that is, is should not overwrite the existing one).

@sindrijo sindrijo changed the title Feature request: Automatically add [FormerlySerializedAs($fieldName$)] when using Refactor-Rename on serialized fields Feature request: Add [FormerlySerializedAs($fieldName$)] when using Refactor-Rename on serialized fields Oct 20, 2016

@sindrijo sindrijo changed the title Feature request: Add [FormerlySerializedAs($fieldName$)] when using Refactor-Rename on serialized fields Feature request: Add [FormerlySerializedAs($fieldName$)] attribute when using Refactor-Rename on serialized fields Oct 20, 2016

@tac14

This comment has been minimized.

Copy link

tac14 commented Nov 3, 2016

I need it, I wait

@CAD97

This comment has been minimized.

Copy link

CAD97 commented Feb 20, 2017

What should be the behavior if I have

[FormallySerializedAs("foo")] public string bar;
public string baz;

and I refactor baz to be called foo?

(Additionally, how does Unity handle that case of two conflicting current/former serialize names? Just assume the most recent? If this won't ever cause an issue, the naive solution of just adding FormerlySerializedAs will work.)

@nkAlex

This comment has been minimized.

Copy link

nkAlex commented Feb 20, 2017

I suppose you could try it out and check ;)
Was thinking about it myself, but I am too lazy to test it, so I just keep stuff organized :)

Btw, you can also stack as many FormallySerializedAs() attributes on top of a property as you wish, so there is even more opportunities for collisions. Most likely assumption is that Unity just iterates through the field names of a corresponding type until it finds a match. Field order may also play a role.

@sindrijo

This comment has been minimized.

Copy link
Author

sindrijo commented Feb 20, 2017

Renaming two fields of the same type simultaneously to the others name will "swap" the values

    //[SerializeField] private int foo = 100;
    //[SerializeField] private int bar = 50;
    
    // Renaming both fields to each other
    [SerializeField, FormerlySerializedAs("foo")] private int bar;
    [SerializeField, FormerlySerializedAs("bar")] private int foo;

Field names take precedence over the FormerlySerializedAs attribute, probably because it is faster. The deserializer will only start looking at attributes if it doesn't find a field with a matching name for the value it is deserializing.

@CAD97 the case you describe wouldn't be a problem but I see where you are getting at so did a quick test:

public class TestFormerlySerialized : MonoBehaviour
{
    //public string foo = "AAA";
    //public string baz = "BBB";

    // Rename foo -> bar
    //[FormerlySerializedAs("foo")]
    //public string bar = "";
    //public string baz = "";

    // Rename baz -> foo
    //[FormerlySerializedAs("foo")] public string bar = "";
    //[FormerlySerializedAs("baz")] public string foo = "";

    // Rename bar -> baz
    //[FormerlySerializedAs("bar"), FormerlySerializedAs("foo")] public string baz = "";
    //[FormerlySerializedAs("baz")] public string foo = "";

    // Rename foo -> bar = Result: Value of baz is deserialized/"copied" into bar, we end up with two fields with the same value
    [FormerlySerializedAs("bar"), FormerlySerializedAs("foo")] public string baz = "";
    [FormerlySerializedAs("foo"), FormerlySerializedAs("baz")] public string bar = "";
}

I think it would be possible to restrict the feature to cases where it won't mess up (it won't mess up if the field is not already marked with the attribute. Though multiple renaming on the same type where the names collide (without going back to unity to serialize between each rename will still be affected by the "swap/copy"-problem.) I don't think it is an issue the plugin should concern itself if it can't do prevent/do anything about it.

@citizenmatt

This comment has been minimized.

Copy link
Member

citizenmatt commented Jan 9, 2018

See also RIDER-12298

@Skjalgsm

This comment has been minimized.

Copy link

Skjalgsm commented Feb 15, 2018

Any progress on this enhancement? It's a really nice suggestions

@citizenmatt

This comment has been minimized.

Copy link
Member

citizenmatt commented Feb 15, 2018

What? It's only been open for a little bit more than a year. :)

We'll see if we can look at is as part of the 2018.1 milestone, but to be honest, it's looking pretty busy already.

This does look like a feature that is almost exclusively made up of edge cases, though :)

@Skjalgsm

This comment has been minimized.

Copy link

Skjalgsm commented Feb 15, 2018

What exactly do you mean by edge cases?

If you are referring to the comments discussing internal serializing in unity, then I have to say that there's a lot of comments in this issue that is not actually really related to the feature request itself.

The feature request is really simple though, all you have to do is add the [FormerlySerializedAs("$oldFieldName")] attribute above a field that you have renamed if the field is serializable (meaning that it is a private field with [SerializedField] attribute (or a public field) in a MonoBehaviour.

@Skjalgsm

This comment has been minimized.

Copy link

Skjalgsm commented Feb 15, 2018

When I rename CoinBalance to expBalance
rider64_2018-02-15_10-40-08

It turns into this
rider64_2018-02-15_10-40-26

But I also want it to add this attribute.
rider64_2018-02-15_10-40-36

Thats it.

@citizenmatt

This comment has been minimized.

Copy link
Member

citizenmatt commented Feb 15, 2018

It's a bit of a cheeky comment to be honest, as the mainline case is as you say.

But there are definitely edge cases - e.g. what if you rename the field multiple times in a single code session (between source commits)? If none of the intermediate names made it off your machine, what should we do with them? And there's also the scenario listed above about using the name of a previous field - should we treat a previous name as a conflict and prevent that case? Also, when can we remove these attributes? Does order matter?

It all plays into the question of "are we doing the best thing for the user?" - we want to make sure that we're offering value and not making it quick and easy to shoot yourself in the foot.

@AngryAnt

This comment has been minimized.

Copy link

AngryAnt commented Feb 15, 2018

Just keep appending the attributes - with a quick check to see that your addition would not duplicate an existing one.

The rest of the comments in here are related to understanding the underlying Unity feature.

Making any further assumptions would require inspecting the Unity serialiser of the project you think holds all relevant data. In other words a lot of work for something which heavily relies on crossed fingers and a failure state of data loss & hard to debug behaviour.

So, only acting on what you know, all you can do safely is append attributes when you know a rename refactor is happening. And that is plenty.

Everything on the script<->serialiser boundary requires a user aware of what they are doing. Tools here trying too much guesswork are way less than helpful. This requested feature, however, would be.

citizenmatt added a commit that referenced this issue Jun 13, 2018

Add FormerlySerializedAs on rename
Fixes #54, fixes RSRP-12298
@sorencoder

This comment has been minimized.

Copy link

sorencoder commented Aug 22, 2018

Has this feature been added to rider/resharper?

@van800

This comment has been minimized.

Copy link
Contributor

van800 commented Aug 22, 2018

@sorencoder it is already in Rider 2018.2. And it will get to Re# 2018.2, when the plugin itself is published. (#706)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment