Skip to content
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

Add validation for DirtyField strings #5713

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Tayrtahn
Copy link
Member

@Tayrtahn Tayrtahn commented Feb 27, 2025

This adds validation to DirtyField<T> method calls to make sure that the passed fieldName string matches the name of a field on the target component.

NextAttack is not a member of GunComponent

This is implemented as an attribute that can be applied to method parameters, ValidateMemberAttribute. The analyzer uses the first generic type argument of the method as the target - this is a little funky, but it doesn't seem to be possible to pass T into the attribute in the method signature, so this was the best I could do. It builds a HashSet of all the members of the target type, then iterates through the parameters of the method invocation looking for the ValidateMember attribute. If it finds one, it checks the compile-time constant string value of the parameter against the member HashSet and reports an error if it doesn't find a match.

Note that this will not catch situations where two components with identically-named members are swapped. In the screenshot example, if GunComponent did have a field named "NextAttack", passing in nameof(MeleeWeaponComponent.NextAttack) would not cause an error (either in the analyzer or at runtime) because the string would match. I don't think that can be addressed without some major changes to the way delta fields are implemented.

This PR applies the attribute to the DirtyField<T>(EntityUid, Component, ...) method of EntityManager and the DirtyField<T>(EntityUid, Component, ...) and DirtyField<T>(Entity<T>, ...) proxy methods of EntitySystem.

Resolves #5706

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DirtyField should cause an IDE warning or integration test fail for invalid datafields
1 participant