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 parameter list ref usage analyzer for low level reader/writer #1769

Open
Wraith2 opened this issue Feb 6, 2024 · 1 comment
Open

Add parameter list ref usage analyzer for low level reader/writer #1769

Wraith2 opened this issue Feb 6, 2024 · 1 comment

Comments

@Wraith2
Copy link

Wraith2 commented Feb 6, 2024

While trying to write a custom formatter for a complex type I hit a problem where the write phase was producing illegal output which seemed to be caused by a missing field write. I have now tracked this down to a missing ref on an extension method.

        public static void Write(this /* ref */ MessagePackWriter writer, int? value)
        {
            if (value.HasValue)
            {
                writer.Write(value.Value);
            }
            else
            {
                writer.WriteNil();
            }
        }

The MessagePackWriter is a struct for performance reasons and if the ref is omitted a copy will happen here. The write advance is applied to the copy and then lost when the method returns. This leads to the write vanishing in this case. This is a subtle and hard to diagnose pit of failure.

I suggest that an analyzer be added looking at the use of MessagePackReader and MessagePackWriter in parameter lists which check to see if they are being passed by ref and if they aren't producing an error.

@AArnott
Copy link
Collaborator

AArnott commented Feb 6, 2024

Not a bad idea. If you send a PR for it yourself, be sure you target the develop branch, as there have been a lot of changes between that and master (our default branch).

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

No branches or pull requests

2 participants