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

Developers using Reflection should be able to use pointers and function pointers #44327

Closed
1 of 3 tasks
Tracked by #44314
steveharter opened this issue Nov 5, 2020 · 11 comments
Closed
1 of 3 tasks
Tracked by #44314
Assignees
Labels
area-System.Reflection Cost:L Work that requires one engineer up to 4 weeks Priority:2 Work that is important, but not critical for the release Team:Libraries tracking This issue is tracking the completion of other related issues. User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@steveharter
Copy link
Member

steveharter commented Nov 5, 2020

Treatment of pointers in general is hit-and-miss especially with the newer function pointer (delegate*) feature.

Currently with function pointers, an IntPtr or fnptr* types are returned, so it is not possible today to distinguish between function pointers and other uses of IntPtr, for example. It is also not possible to determine the parameter types, return type or the calling convention.

@steveharter steveharter added this to the 6.0.0 milestone Nov 5, 2020
@steveharter steveharter self-assigned this Nov 5, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Reflection untriaged New issue has not been triaged by the area owner labels Nov 5, 2020
@steveharter steveharter added enhancement Product code improvement that does NOT require public API changes/additions area-System.Reflection and removed area-System.Reflection untriaged New issue has not been triaged by the area owner labels Nov 5, 2020
@BreyerW
Copy link

BreyerW commented Nov 5, 2020

What about ref in general? Eg ref prop ref returning methods etc? This part historically was half assed since support for it was added relatively recently in languages (several years at this point). Is this on the table?

@steveharter
Copy link
Member Author

steveharter commented Nov 6, 2020

What about ref in general?

Any common use case that is not supported is being considered. @BreyerW can you share an example so this is properly captured here? Thanks

@BreyerW
Copy link

BreyerW commented Nov 6, 2020

@steveharter im not expert on reflection but i remember having trouble with ref return methods when generating dynamic methods it simply fails (dont remember exact exception since its year ago by now). Although theres workaround by defining entire assembly then generating ref return method inside that assembly but it isnt easy to discover i myself found that solution on accident in stackoverflow thread. Ref prop probably has similar issue since its method behind the scene.

Also frontline reflection API (or in other words most easily discoverable API) like GetValue dont have ref alternatives at all. I think even @jkotas or someone from net core team made a thread about this API type few years ago when dotnet/runtime was dotnet/corefx but you never fully committed resources to this and as such they never materialized

Last gripe is in System.Linq.Expression. this namespace is often bugged in presence of new uses of ref eg when trying to generate assignment to ref prop it fails because ref prop is defined as CanWrite=false despite being obviously wrong. Then again you killed this namespace's future so i guess this particular case is of no interest to you

@steveharter steveharter added Bottom Up Work Not part of a theme, epic, or user story User Story A single user-facing feature. Can be grouped under an epic. Cost:XL Work that requires one engineer more than 4 weeks and removed Team Epic enhancement Product code improvement that does NOT require public API changes/additions labels Nov 12, 2020
@steveharter
Copy link
Member Author

Last gripe is in System.Linq.Expression

@BreyerW it looks like you added this issue previously: #25700. If you consider it blocking key scenarios, add the "blocking" label to it.

@BreyerW
Copy link

BreyerW commented Nov 12, 2020

@steveharter not anymore due to this bug i simply moved away from S.L.E.

However i would consider it a bug in System.Reflection not in S.L.E in other words much higher priority than S.L.E i think? The root of evil here is in fact located in PropertyInfo.CanWrite NOT in S.L.E itself. Currently for ref returning props PropertyInfo.CanWrite reports false (because no setter available) while it should be true unless ref return is readonly ref

@steveharter
Copy link
Member Author

Currently for ref returning props PropertyInfo.CanWrite reports false (because no setter available) while it should be true unless ref return is readonly ref

If PropertyInfo.CanWrite semantics are changed for ref-returning properties that would likely be considered a breaking change as the documentation today states the calculation is based on whether there is a setter.

I see this S.L.E issue discusses it in more detail. It should also be basing it on whether the return type is a reference or byref type. Similar logic would be needed for parameter (not just return value). For example the code below outputs:

RefInt CanModify: True
RefType CanModify: True
Int CanModify: True
internal class Program
{
    private static void Main(string[] args)
    {
        CanModify(typeof(MyClass).GetProperty("RefInt")!);
        CanModify(typeof(MyClass).GetProperty("RefType")!);
        CanModify(typeof(MyClass).GetProperty("Int")!);

        static void CanModify(PropertyInfo info)
        {
            // Sample logic; not complete since they don't check if value type is readonly via [IsReadOnlyAttribute]
            bool canModify = info.CanWrite || !info.PropertyType.IsValueType || info.PropertyType.IsByRef;
            Console.WriteLine($"{info.Name} CanModify: {canModify}");
        }
    }
}
public class MyClass
{
    private int _refInt;
    public ref int RefInt => ref _refInt;
    public MyClass RefType => this;
    public int Int { get; set; }
}

@BreyerW
Copy link

BreyerW commented Nov 13, 2020

@steveharter i see too bad but makes sense

@danmoseley danmoseley changed the title Support newer language and runtime features in reflection Apps can access newer language and runtime features through reflection Nov 16, 2020
@danmoseley danmoseley changed the title Apps can access newer language and runtime features through reflection Developers using Reflection can access the new language and runtime features Nov 29, 2020
@jeffhandley jeffhandley added the Priority:3 Work that is nice to have label Jan 21, 2021
@steveharter
Copy link
Member Author

Moving to 7.0; many of the features require language support while others were not funded.

@steveharter steveharter modified the milestones: 6.0.0, 7.0.0 Jul 19, 2021
@ghost ghost added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Oct 12, 2021
@buyaa-n buyaa-n removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Oct 14, 2021
@joperezr joperezr added the tracking This issue is tracking the completion of other related issues. label Oct 14, 2021
@steveharter steveharter added Priority:2 Work that is important, but not critical for the release and removed Priority:3 Work that is nice to have labels Dec 16, 2021
@steveharter
Copy link
Member Author

steveharter commented Dec 16, 2021

Note the "fast invoke" and "ref struct" feature was moved to it's own user story at #45152.

@steveharter steveharter added Cost:L Work that requires one engineer up to 4 weeks and removed Cost:XL Work that requires one engineer more than 4 weeks labels Dec 17, 2021
@steveharter steveharter changed the title Developers using Reflection can access the new language and runtime features Developers using Reflection should be able to use pointers and function pointers Dec 17, 2021
@steveharter steveharter removed the Bottom Up Work Not part of a theme, epic, or user story label Jan 5, 2022
@danmoseley
Copy link
Member

cc @tannergooding in case of interest, since it came up this morning.

@steveharter
Copy link
Member Author

Closing for 8.0 since the remaining functionality is now tracked by #75358

@ghost ghost locked as resolved and limited conversation to collaborators Jan 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection Cost:L Work that requires one engineer up to 4 weeks Priority:2 Work that is important, but not critical for the release Team:Libraries tracking This issue is tracking the completion of other related issues. User Story A single user-facing feature. Can be grouped under an epic.
Projects
No open projects
Development

No branches or pull requests

7 participants