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

Resolve System.Type.GetMethod(name) at compile time #2321

Open
lfr opened this issue Dec 17, 2020 · 12 comments
Open

Resolve System.Type.GetMethod(name) at compile time #2321

lfr opened this issue Dec 17, 2020 · 12 comments

Comments

@lfr
Copy link

lfr commented Dec 17, 2020

After having worked around most limitations, it appears I cannot make FSharp.Domain.Validation compatible with Fable which is a shame as it's a match made in heaven for web dev for form validation.

I found workarounds to read the content of the blocks, but I can't create them because the information required to create the blocks (such as their generic parameters) is stored in the interface they implement, and the limited reflection support that I gather from these tests doesn't include GetInterfaces().

I'd love to contribute but I'm not myself a web developer and I don't think the solution to this is pure F#. Is better interface reflection support planned or should I give up?

@lfr
Copy link
Author

lfr commented Dec 17, 2020

I found a workaround to my problem by limiting the block types to blocks of non-DU types, which I think it's an ok compromise since 99% of the time people are going to be validating strings, ints, etc.

Since I already had a ticket open about reflection, I'll close this one.

@lfr lfr closed this as completed Dec 17, 2020
@alfonsogarciacaro
Copy link
Member

In the early stages of Fable we picked interfaces to easily type objects coming from JS, and we cannot change this now because a lot of bindings depend on this, that's why we cannot do too many things with interfaces because Fable sees them just as a contract of the object shape and doesn't expect the object to contain any information about the interfaces it implements, so we cannot do interface testing or similar. Though sometimes we do dirty runtime checks like typeof x.Dispose === "function".

However, I guess it would be possible to implement .GetInterfaces() for types known at compile time. We should only be careful because interface declarations are erased so we would have to generate the interface information in the call site. Which means that if you call .GetInterfaces() multiple times on the same type in different places Fable will output duplicated code.

@alfonsogarciacaro
Copy link
Member

Ok, adding support for .GetInterface was not too problematic when types can be resolved at compile time, so in the next release you'll be able to do something like this. I hope it helps!

@lfr
Copy link
Author

lfr commented Dec 19, 2020

@alfonsogarciacaro it'll help if we also get the following, first four seem to work according to tests, not sure if it's possible for the last two:

✅ Type.MakeGenericType
✅ GetGenericArguments
✅ Type.IsGenericType
✅ Type.GetGenericTypeDefinition
🟩 Type.GetMethod
🟩 MethodBase.Invoke

Do you think this is realistic?

@alfonsogarciacaro
Copy link
Member

It's... problematic. Because being able to get a reference of all methods of a type at runtime means you cannot do tree shaking. This is for example the reason Dart doesn't support reflection. But maybe we could give support for Type.GetMethod(methodName) when the method can be resolved at compile time as we did with .GetInterface. Would that work for you?

@lfr
Copy link
Author

lfr commented Dec 20, 2020 via email

@alfonsogarciacaro alfonsogarciacaro changed the title Better interface support 😥 Resolve System.Type.GetMethod(name) at compile time Dec 22, 2020
@alfonsogarciacaro
Copy link
Member

@lfr I think you found a workaround, is that right? I had a look at this, but unfortunately it would require more changes than I expected because the code to mangle method names depends right now on the F# AST, and in the Replacementes module we only have access to the Fable AST. I need to change this at some point, but it will take time.

@lfr
Copy link
Author

lfr commented Jan 13, 2021

@alfonsogarciacaro This is the one thing I didn't find a workaround for, I ended up publishing the library without one of its functions and added this note to the demo page:

image

If you're talking a couple of months it's fine for people to use the code above IMO, my library is still not 1.0

@lfr
Copy link
Author

lfr commented Feb 10, 2021

@alfonsogarciacaro I was hoping to have the Fable version of my lib feature-complete with the standard version before going 1.0, realistically speaking how soon or late do you think this has a chance of being implemented?

On a related note, do we actually know that MethodBase.Invoke works? I can't find it in ReflectionTests but somehow I was under the impression that it wouldn't be a problem. Here's the code the way it's going to look if GetMethod gets implemented:

let wrapMethodInfo =
  typedefof<Box<_,_>>
    .MakeGenericType(typeof<'a>, errorType)
    .GetMethod("wrap", BindingFlags.Static ||| BindingFlags.Public)

wrapMethodInfo.Invoke(null, [|box inp|])

@alfonsogarciacaro
Copy link
Member

TBH, I'm not sure. I want to do the refactor that enables this, but the priority now is polishing, fixing bugs and update the documentation.

Not sure about MethodBase.Invoke either, sorry. I think we don't have any support at the moment for invoking methods via reflection in order to avoid keeping references around that prevent tree shaking.

@lfr
Copy link
Author

lfr commented Feb 11, 2021

Believe me, I understand! I just needed to have an idea so I can make an informed decision about publishing 1.0 with or without the feature, so thanks for letting me know 🙏

@alfonsogarciacaro
Copy link
Member

Some other nice-to-have reflection APIs: #2119 (comment)

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