ClearScript icon indicating copy to clipboard operation
ClearScript copied to clipboard

Suggestion: option to throw exception when adding non-public types

Open zvrba opened this issue 8 months ago • 4 comments

Minimal reproducible example for version 7.5.0 is given below.

When WithIndexer is internal, the program throws "Microsoft.ClearScript.ScriptEngineException: 'TypeError: Cannot read properties of undefined (reading 'get')'."

When WithIndexer is made public, the test program gives the expected output.

Now, this issue has bitten me a couple of times already, and every time I used ca 15 minutes to figure out what's wrong: googling around, comparing with the suggestions in old issues, trying to make small changes, etc, until it finally dawns on me: PUBLIC! I'm repeatedly stumbling over this, and it's never "obvious".

It would be nice to have an option that writes out a warning or even throws exception when an internal type is given to AddHostType or AddHostObject. It seemingly works, until it doesn't.

I'm unsure what behavior I'd like when adding an internal class that derives from a public class or implements an internal interface.

Perhaps some general event that is raised in all such cases instead would be more appropriate than logging or throwing. It'd immensely help with diagnostics.

PS: Thanks for this great project. It makes it is really easy to script a C# app with JS.

using Microsoft.ClearScript.V8;

namespace ClearScriptTest;

internal class Program
{
    static readonly V8ScriptEngine Engine = new();

    static void Main(string[] args)
    {
        Engine.AddHostType("Console", typeof(Console));
        Engine.AddHostObject("I", new WithIndexer());

        Engine.Execute(TestIndexer);
        Console.WriteLine("DONE");

    }

    static readonly string TestIndexer =
@"
Console.WriteLine(I);
Console.WriteLine(I.Item.get(0));
I.Item.set(2, -2);
Console.WriteLine(I.Item.get(2));
";
}

/*public*/ class WithIndexer
{
    private readonly int[] x = new int[10];

    public int this[int i] {
        get => x[i];
        set => x[i] = value;
    }

}

zvrba avatar May 15 '25 16:05 zvrba

Hi @zvrba,

It would be nice to have an option that writes out a warning or even throws exception when an internal type is given to AddHostType or AddHostObject.

We could probably add such an option. However:

  1. It isn't just a matter of internal; a type's script visibility is a function of its accessibility modifier, its nesting hierarchy, and the engine's AccessContext. The best the option could do is inform you whether the type is visible.
  2. Those methods aren't the only way to expose resources to the script engine. A .NET method could return an object not otherwise exposed, a type collection could provide access to an internal type, etc.
  3. A type's visibility doesn't tell you much directly. Instead, it's a factor in the determination of member visibility. A comprehensive implementation of this feature could have significant performance impact.

I'm unsure what behavior I'd like when adding an internal class that derives from a public class or implements an internal interface.

That's a good point. Some or all of a type's members could be visible even if the type itself isn't, and all .NET types are ultimately derived from a public one. The difficulty in designing a feature like this is making it easy to predict its effect in various situations.

Perhaps some general event that is raised in all such cases instead would be more appropriate than logging or throwing. It'd immensely help with diagnostics.

That's an interesting suggestion. We'll give it some thought and take a closer look.

PS: Thanks for this great project. It makes it is really easy to script a C# app with JS.

Thank you for your suggestion and your kind words!

ClearScriptLib avatar May 16 '25 14:05 ClearScriptLib

Hi,

Thanks for the explanation.

A comprehensive implementation of this feature could have significant performance impact.

Indeed. Now that I've read your comment and thought about this some more: things "just work" most of the time :) and an event-based facility such as I suggested is probably an overkill.

How about the following approach: what if ScriptEngine exposed a built-in global method such as __ExplainVisibility(obj) that, given an object instance, could give a member-by-member explanation of why the member was imported or ignored? Performance would not matter either as that method would be used for diagnostics only.

Documentation about AccessContext is terse. Could you please elaborate a bit more? The part "is to be treated as if it were part of that type's implementation" could be interpreted:

  • Narrowly: the script gets access to the type's private/protected members.
  • Widely: the script gets access to the above, PLUS all internal classes and their public/internal members in the type's assembly.
  • Raises questions: If AccessContext is a class, is the script considered a subclass of that class? Can it override methods? Can it add new methods callable by C# through dynamic?

zvrba avatar May 16 '25 15:05 zvrba

Hi @zvrba,

How about the following approach: what if ScriptEngine exposed a built-in global method such as __ExplainVisibility(obj) that, given an object instance, could give a member-by-member explanation of why the member was imported or ignored? Performance would not matter either as that method would be used for diagnostics only.

That's another interesting idea. Thanks!

Documentation about AccessContext is terse.

Sorry about that. The documentation also has a bug: The description should be "Allows script code to access non-public resources." We'll fix that.

Widely: the script gets access to the above, PLUS all internal classes and their public/internal members in the type's assembly.

That's correct, although other factors could be at play – e.g., HostItemFlags.PrivateAccess.

If AccessContext is a class, is the script considered a subclass of that class?

No, it's considered part of the code that implements that class.

Determining member visibility requires information about both the member and the caller. For .NET code, the caller (aka access context) is simply the type containing the calling code. As script code isn't part of a .NET type, it needs an explicitly assigned access context to provide caller information.

BTW, by default, with no access context, script code can only see members that are "fully public" – that is, public all the way up the nesting hierarchy. An access context could allow it to see more.

Hope that helps!

ClearScriptLib avatar May 16 '25 19:05 ClearScriptLib

The following

For .NET code, the caller (aka access context) is simply the type containing the calling code.

was very helpful. Indeed, you could just copy-paste the two paragraphs into the documentation for AccessContext. It also deserves an entry in "FAQtorial".

Thanks!

zvrba avatar May 17 '25 05:05 zvrba