Remove backing fields when weaving generated classes #3088
Conversation
| var il = prop.GetMethod.Body.GetILProcessor(); | ||
| prop.GetMethod.Body.Instructions.Clear(); |
There was a problem hiding this comment.
I believe the previous implementation of replacing the getter was causing some unintended behavior, it was actually just adding instructions to the getter rather than removing the old instructions---the reason why this was not noticeable was because the old instruction became unreachable because of the ret/return statement. My ILSpy was actually giving me an error about this prior. One issue this was causing was the fact that it kept instructions that have a reference to the backing field, making it error-prone when removing them. This change makes it identical to the functionality in the ReplaceGeneratedClassSetter.
There was a problem hiding this comment.
Good eye! Need to take a look at this
...s/SourceGenerators/SourceGeneratorAssemblyToProcess/ErrorClasses/UnsupportedRequiredTypes.cs
Outdated
Show resolved
Hide resolved
gagik
force-pushed
the
gagik/removebackingfields
branch
from
039ce83
to
186ec09
Compare
Realm/Realm.Weaver/RealmWeaver.cs
Outdated
| // it considers this th start index of backing field initializaion instructions. | ||
| // And removes all backing field instructions from end to start. | ||
| else if (instruction.OpCode == OpCodes.Ldarg_0) { | ||
| for(var j = backingFieldInstructionsEnd; j >= i; j--) { |
There was a problem hiding this comment.
Setting backingFieldInstructionsEnd=-1 actually takes care of cases when we never come across any backing fields beforehand, but I could also more explicitly avoid this by doing something like if (instruction.OpCode == OpCodes.Ldarg_0 && backingFieldInstructionsEnd != -1)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks reasonable - you'll need to add some spaces between if/for/foreach and ( and move all the opening braces to a newline 😄 I have a low priority background task to figure out a linter that will automatically do this for you, but for now it's a matter of looking at warnings and doing what the analyzer tells you.
| var il = prop.GetMethod.Body.GetILProcessor(); | ||
| prop.GetMethod.Body.Instructions.Clear(); |
There was a problem hiding this comment.
Good eye! Need to take a look at this
...m.SourceGenerator/Realms.SourceGenerator.RealmGenerator/InitiializedFieldObject_generated.cs
Outdated
Show resolved
Hide resolved
...m.SourceGenerator/Realms.SourceGenerator.RealmGenerator/InitiializedFieldObject_generated.cs
Outdated
Show resolved
Hide resolved
gagik
changed the title
gagik
changed the title
Co-authored-by: Nikola Irinchev <irinchev@me.com>
Co-authored-by: Nikola Irinchev <irinchev@me.com>
Co-authored-by: Nikola Irinchev <irinchev@me.com>
Co-authored-by: Nikola Irinchev <irinchev@me.com>
gagik
force-pushed
the
gagik/removebackingfields
branch
from
e7f8219
to
cccdf49
Compare
Description
We are weaving the properties of the generated classes to go directly to the accessors, so the backing fields are unused. This PR attempts to remove them for generated classes.
Fixes #2994
TODO