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

Fix throwing on accessing CLR FunctionDeclaration #1949

Merged
merged 1 commit into from
Aug 18, 2024

Conversation

lofcz
Copy link
Contributor

@lofcz lofcz commented Aug 18, 2024

I'm open to finding a better solution to the problem described below, as the current fix has weird ergonomics.

Accessing FunctionDeclaration of a MethodInfoFunction (CLR functions) currently throws - the test* included describes the scenario. I believe accessing the property should never throw (I hate linking to MSDN, alas: https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/property?redirectedfrom=MSDN).
This forces a sprinkle of ! in the codebase in places where we know the function is not a CLR function (this is the part I don't like).

*I've included two tests now, one for the delegate, and one for the method info function.

Unrelated to this, for the debugger I'm working on, I'd need to make at least some information of MethodInfoFunction and DelegateWrapper public to provide information about the native function (get the parameters, largely the same thing Jint does internally). This is outside of this PR's scope, just announcing ahead of time to discuss, should there be objections to that.

This would be:

  • DelegateWrapper: Delegate _d
  • MethodInfoFunction: MethodDescriptor[] _methods, possibly ClrFunction _fallbackClrFunctionInstance

@lofcz lofcz force-pushed the feat-fix-function-declaration branch 2 times, most recently from 2eb6faf to 9f02e9a Compare August 18, 2024 02:38
@lofcz
Copy link
Contributor Author

lofcz commented Aug 18, 2024

The failing test is something flaky with async implementation, unrelated.

Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

@lahma lahma enabled auto-merge (squash) August 18, 2024 05:20
@lahma lahma merged commit 4767b3a into sebastienros:main Aug 18, 2024
3 checks passed
@lahma
Copy link
Collaborator

lahma commented Aug 18, 2024

Unrelated to this, for the debugger I'm working on, I'd need to make at least some information of MethodInfoFunction and DelegateWrapper public to provide information about the native function (get the parameters, largely the same thing Jint does internally).

So I guess you need the typed params and it would not be enough to implement CLR method's custom IFunction descriptor which would return the names which IFunction has a property for. Creating such fake wrapper would also offer always non-null function declaration.

@lofcz
Copy link
Contributor Author

lofcz commented Aug 18, 2024

I pondered the IFunction idea but ultimately I don't see the fake wrapper as able to expose the data I need (typed params). If you are ok with it, I can create a PR tomorrow exposing the members listed above.

Probably also XML doc for IFunction? FunctionDeclaration to explain that it's always null for CLR functions.

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.

2 participants