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 "Argument ref" parameters #64

Open
slang25 opened this issue Jul 9, 2023 · 6 comments
Open

Add "Argument ref" parameters #64

slang25 opened this issue Jul 9, 2023 · 6 comments

Comments

@slang25
Copy link

slang25 commented Jul 9, 2023

From looking at the example in the README, it looks like a nice feature would be to have an attribute that could only be used with a ref parameter.

So the example would become this:

public static partial class ExampleFilter
{
    [ArgumentativeFilter]
    public static ValueTask<object?> NormalizeParameterCase(EndpointFilterInvocationContext context, EndpointFilterDelegate next, [ArgumentRef] ref string parameter)
    {
        parameter = parameter.ToUpperInvariant();
        return next(context);
    }
}

The code gen would ref return the value from the context.Arguments array behind the scenes, reusing the existing IndexOf functionality.

@slang25
Copy link
Author

slang25 commented Jul 9, 2023

Although I've just had a look at adding this myself and it's not going to be simple as context.Arguments is an IList<object?>, so getting a reference and narrowing the type while still being a reference is not possible without an intermediate copy, and then the bound method needs awaiting, so there could be a bit of unwanted overhead.

Or maybe there's some crazy ref-foo I don't know about. (maybe ref Unsafe.As<object, ?>(ref ...)

Edit: Ok ignore that last suggestion, getting a ref to the underlying context.Arguments isn't going to help because the implementation makes sure to box & unbox before giving you a value, so for non-reference types it would be misleading. It's an IList<object?> but with custom implementations that are not the usual List<T>.

@hwoodiwiss
Copy link
Owner

hwoodiwiss commented Jul 10, 2023

The usage side should be fairly easy, I would expect there's a reasonably easy way to get whether a parameter has the ref modifier on it.

And yeah, it looks like EndpointFilterInvocationContext, at least for endpoints with 9 or less params, uses a generated version of EndpointFilterInvocationContext<T1..Tx>, that just directly gets and sets based on index, meaning boxing and unboxing is a bit hidden, without doing gross stuff that would end up expensive in its own right.

@hwoodiwiss
Copy link
Owner

I'll add support for ref params as a first pass, with nothing special as far as retrieving the values, as I ideally want to support as many use cases as possible, even if there isn't much real benefit.

@hwoodiwiss
Copy link
Owner

I've create #66 to add basic ref parameters support, I'm going to look at ways to try and convert to the specialized EndpointFilterInvocationContext<T1..Tx> types, without anything too awful, or enough reflection to make using ref pointless from a performance perspective.

@slang25
Copy link
Author

slang25 commented Aug 4, 2023

I'm not sure if my opening post made sense, what I was suggesting was to use a ref parameter as a way of avoid using IndexOf to index into the Arguments property, I thought it could be cleaner that way. I think what I was asking for is ultimately invalid, as it may not be possible to get a reference to the underlying value.

If the source generator worked by taking an object, then it would make sense to bind a property to an argument, then the setter would delegate to the underlying EndpointFilterInvocationContext<T1..Tx>, however that design change is a bit overkill for what is otherwise a really elegant solution.

@hwoodiwiss
Copy link
Owner

hwoodiwiss commented Aug 8, 2023

Aaaah, got you, yeah, that would be quite nice and ergonomic for mutating parameters, I was trying to think of ways this could be doable. now the libraries split into code and generator, I might add a type for convenience that allows something like this that gets special handling by the generator.

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

No branches or pull requests

2 participants