Add ObjectWrapperReportedMemberTypes to Options

[lofcz]

Implements the feature discussed in #1945.

[lahma]

I think this needs some discussion.

The JavaScript behavior for get/set etc:

var base = {
  baseField: "foo",
  funcBase() {
  },
  get propBase() {
  },
  set propBase(val) {
  },
};

var a = {
  aField: 0,
  funcA() {
  },
  get propA() {
  },
  set propA(val) {
  },
}

Object.setPrototypeOf(a, base);

console.log("own: " + Object.getOwnPropertyNames(a));

console.log("enumerable properties");

for (var k in a) {
  console.log(k);
}

Output:

own: aField,funcA,propA
enumerable properties
aField
funcA
propA
baseField
funcBase
propBase

So the correct output for the "full" test case should be:

jsProps.Should().BeSameAs(["A", "F"]);

It should not contain inherited ones and property get/set and not listed as it's implied by the property. GetType is already special cased in configuration anyway not to be allowed as it opens reflection. So I think only addition should be own methods that are not property get/set or any other special name (m.IsSpecialName).

[lahma]

Maybe just engine.Evaluate("clrInstance.get_A()").Should().Be(10);, but I'm goin to argument that the current behavior is wrong

[lahma]

well not sure if the possibility to call is wrong, but the idiomatic way is to evaluate clrInstance.A

[lofcz]

Thanks for the feedback, the requested changes are in 654d557, we can squash it after your review.

1. All three flags for CLR types are now set on by default.
2. The "full" test returns ["A", "F"]
3. Added the List<>.Should() asserts.

[lofcz]

I have no idea why or how the Linux tests are failing:

Passed! - Failed: 0, Passed: 64848, Skipped: 7253, Total: 72101, Duration: 1 m 1 s - Jint.Tests.Test262.dll (net8.0)
Error: Process completed with exit code 1.

Maybe this?

[xUnit.net 00:00:04.07] [FATAL ERROR] System.AppDomainUnloadedException

[lahma]

No worries, I'll look into this and from quick look seems good. Thanks for iterating and discussing 👍🏻

[lahma]

Looks good, I tweaked a bit for readability. Thanks for fixing this!

[lofcz]

@lahma why are we taking DeclaredOnly fields and methods but not properties?

[lahma]

@lahma why are we taking DeclaredOnly fields and methods but not properties?

It would break existing test case, if you want to check the feasibility for the change, a PR and investigation is always welcome 🙂

[lofcz]

Could you point me to the failing test? I'm using Windows box and tests were passing here (or maybe I'm just stupid and didn't run all the tests?) without the DeclaredOnly constraint. I'd like to look into lifting it, as the current version won't report inherited fields/methods, which I need for the debugger scenario described in the linked issue.
I see the issue is with the full framework, as usual -.-

[lahma]

For me it's EngineShouldStringifyAnJObjectListWithValuesCorrectly and it's enough to have foreach (var p in ClrType.GetProperties(BindingFlags | BindingFlags.DeclaredOnly)) when runnin on NET 8.

[lahma]

So I guess it's a case of dynamic where nobody knows where the properties originate from. It's been a PITA for so long but a valid use case - probably ExpandoObject related or something.

[lofcz]

I've run the two tests included in this PR and they fail for net462 which I've not noticed before:

This is without any changes. The EngineShouldStringifyAnJObjectListWithValuesCorrectly seems to be another thing.

Is the Windows test run configured here in CI running both .net fw and core configurations? I can fix the test for the full fw, I just fail to understand how the CI tests passed.

[lahma]

This is my result when running on a Windows box with the flag changed for properties against latest main

[lofcz]

This one is passing for me, both with and without the DeclaredOnly flag:

My SDK is 8.0.302

[lahma]

Let build server be the judge: #1948

Failed test summary: https://github.com/sebastienros/jint/actions/runs/10434541945?pr=1948

[lofcz]

I'll try to get my environment to be consistent with the build server then and look into the failing test.