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

Static members in CLR types act like instance members when marshalled to JS #1977

Open
TeamDoodz opened this issue Oct 9, 2024 · 6 comments · May be fixed by #1981
Open

Static members in CLR types act like instance members when marshalled to JS #1977

TeamDoodz opened this issue Oct 9, 2024 · 6 comments · May be fixed by #1981

Comments

@TeamDoodz
Copy link

Version used

4.0.3

Describe the bug
When adding a CLR type reference to an engine via TypeReference.CreateTypeReference, static members in the type act the same way as instance members when used in JS.

To Reproduce

using Jint;
using Jint.Runtime.Interop;

internal class Program {
	static void Main(string[] args) {
		Engine engine = new Engine();
		engine.SetValue("ExampleType", TypeReference.CreateTypeReference<ExampleType>(engine));
		
		// Static members can be acessed as if they were instance members:
		Console.WriteLine(engine.Evaluate("JSON.stringify(new ExampleType())").ToString()); // Expected: {"Value": 0}, Actual: {"TestProperty": 5, "Value": 0, "Test": 5}
		Console.WriteLine(engine.Evaluate("new ExampleType().Test").ToString()); // Expected: undefined, Actual: 5
		Console.WriteLine(engine.Evaluate("new ExampleType().TestProperty").ToString()); // Expected: undefined, Actual: 5
		Console.WriteLine(engine.Evaluate("new ExampleType().TestMethod").ToString()); // Expected: undefined, Actual: function ExampleType.TestMethod() { [native code] }
		
		// Calling static/instance members using the type name works as expected, though:
		Console.WriteLine(engine.Evaluate("ExampleType.Test")); // Expected: 5, Actual: 5
		Console.WriteLine(engine.Evaluate("ExampleType.Value")); // Expected: undefined, Actual: undefined
	}
}

public struct ExampleType {
	public static readonly int Test = 5;
	public static int TestProperty => 5;

	public int Value;

	public static bool TestMethod() => true;
}

Expected behavior
Attempting to read static members on an object instance should return undefined, and stringifying an object instance should not include static fields/properties in the result.

Additional context

This is especially problematic for types that contain static members of themselves (such as Guid with Guid.Empty), as attempting to stringify instances of these types would result in a cyclic dependency.

@lahma
Copy link
Collaborator

lahma commented Oct 10, 2024

This would probably be a breaking change. Would you like to propose a PR and check how existing tests behave?

@TeamDoodz
Copy link
Author

OK, I'll do that.

TeamDoodz added a commit to TeamDoodz/jint that referenced this issue Oct 13, 2024
@TeamDoodz
Copy link
Author

I've made my changes and they do seem to break two existing tests: CanGiveCustomNameToInteropMembers and ShouldStringifyNetObjects; both of which rely on static members being accessible from object instances. Do you still want me to make the PR?

@lahma
Copy link
Collaborator

lahma commented Oct 13, 2024

I think this just needs some think trough, generally exposing statics sounds wrong. Usually checking how JS treats such things with Object.getOwnPropertyNames() gives some guidance. For example, Math.E should show similar behavior like double.MaxValue when thinking about static properties.

@TeamDoodz
Copy link
Author

Looking at JS's own static keyword, it behaves about the same as .NET statics do; they aren't accessible from object instances as demonstrated here (but do show up in Object.getOwnPropertyNames()). IMO, if this is how both JS statics and .NET statics function then I see no reason why .NET statics marshalled into JS shouldn't also act this way.

@lahma lahma linked a pull request Oct 20, 2024 that will close this issue
@lahma
Copy link
Collaborator

lahma commented Oct 20, 2024

I've done a bit simpler approach with #1981 which also allows reverting back to old behavior if needed.

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 a pull request may close this issue.

2 participants