Enso.getMetaObject, Type.isMetaInstance and Meta.is_a consolidation

[JaroslavTulach]

Pull Request Description

Implements getMetaObject and related messages from Truffle interop for Enso values and types. Turns Meta.is_a into builtin and re-uses the same functionality.

Important Notes

Adds ValueGenerator testing infrastructure to provide unified access to special Enso values and builtin types that can be reused by other tests, not just MetaIsATest and MetaObjectTest.

Checklist

Please include the following checklist in your PR:

• The documentation has been updated if necessary.
• All code conforms to the
  Scala,
  Java,
  style guides.
• All code has been tested:
  • Unit tests have been written where possible.

[JaroslavTulach]

This is very surprising check, but needed. Otherwise for example this check yields wrong result!

I am trying to remove it, but so far stdlib tests depend on it.

[radeusgd]

I'm wondering though if this is the expected semantics of is_a? I.e. should we say is_a without this check is wrong or maybe this usage is wrong?

My intuition is that is_a checks if a value is of a given type. The linked example however checks for value, not type.

[radeusgd]

IMHO it will be easier for the users when x.is_a Typ will be aligned with case x of _ : Typ -> - there will be one concept to grasp. Otherwise we will have two very similar concepts that will differ in a subtle way - IMO the worst situation because the user can falsely think they do the same and then get shot in the foot when they actually differ.

[radeusgd]

There is, I think, only one instance where old is_a was shining (although I'm not sure it is currently used within the stdlib, but I think the feature itself is useful) - being able to check if a given value is an instance of a specific atom constructor without depending on this constructor's arity.

For example:

type Foo
    Bar a b c
    Baz x y

hmm v =
    v.is_a Foo.Bar

This way we can check if the value is of the specific Foo.Bar variant. This cannot be done with the _ : Typ match as it will match all Foo instances:

hmm v = case v of
   _ : Foo -> "I'm either a Bar or also Baz"
   _ -> "I'm sth else"

And with regular pattern match, we will need to know the arity:

hmm v = case v of
    Foo.Bar _ _ _ -> "I'm Foo.Bar!"
   _ -> "I'm sth else, maybe Foo.Baz, maybe not Foo at all :)"

Sometimes the arity changes and without good refactoring tools it is a pain to check all pattern matches (well the compiler shouts about it, so it's not the end of the world). But sometimes arity is just an implementation detail and shouldn't be exposed in places like this, just for the sake of cleanliness/elegance.

Now - it seems that this new implementation is losing this ability altogether (with the exception of 0-ary constructors which you link in the above example).

IMO if we want to keep the 0-ary, we should keep it for all kinds of constructors.

And I'd vote to have this kind of functionality as I (subjectively) think it can be sometimes useful (although it is not indispensable).

However, I'm really not sure if is_a is it's right place - I still think that ensuring is_a is aligned with case-of on types is the way to go. Maybe we can split this out into a separate functionality? Possibly on Meta.Constructor or something similar?

Anyway, if we are not using it currently, it may not be worth spending time on implementing it - but maybe worth at least creating a Pivotal issue to track that we may want to have it - or just remember at the back of our head that such possibility can potentially be implemented - as it can sometimes make our life easier.

Regardless, I'd keep that separate from is_a.

[JaroslavTulach]

when x.is_a Typ will be aligned with case x of _ : Typ -> - there will be one concept to grasp

That's the case now. Covered by MetaIsATest.consistencyWithCase test.

[JaroslavTulach]

Anyway, if we are not using it currently, it may not be worth spending time on implementing it - but maybe worth at least creating a Pivotal issue to track that we may want to have it - or just remember at the back of our head that such possibility can potentially be implemented - as it can sometimes make our life easier.

Reincarnated as #5792

[JaroslavTulach]

I can find following unit test failures in the evening run:

org.enso.interpreter.test.ForeignMethodInvokeTest.testForeignFunctionParseFailure`

and

   - should run internal IDE visualisation preprocessor catching error *** FAILED ***

both are caused by "catch Any" - we have to make sure everything is_a Any. That's fixed, but after changing that identical values aren't is_a of itself we have many error-related failures in Enso test suite in the latest run.

[hubertp]

In a number of places it seems like we could simply getType rather than duplicating their logic in getMetaObject definition.
Other than that, this looks really nice! I'm glad IsValueOfType integrated to so well with your stuff.

[hubertp]

That doesn't seem like a normal behaviour so probably should propagate ex via IllegalStateException, right?

[JaroslavTulach]

That would change the semantics. The code continues with isIdentity check as before.

[hubertp]

So it is the equals which may throw UnsupportedMessageException? Because we seem to check that left and right are strings a few lines before.
I just don't think that exceptions being part of a regular control flow is good in general and I would preferred if this was guarded somehow explicitly.

[hubertp]

could we make it a while loop with actual check, please? I'm allergic to (potentially) infinite loops

[JaroslavTulach]

It is not infinite loop as value of actual is decreasing with every loop iteration. Moreover it is covered by tests.

[radeusgd]

Still, isn't while(true) a bit clearer than for(;;)?

[JaroslavTulach]

I see for(;;) as a synonym for loop with breaks for languages that don't have (like Rust) loop construct directly.

[radeusgd]

Enso changes look good definitely 😁

Just a few comments

[radeusgd]

Suggested change

	if (value instanceof Boolean b ) {
	if (value instanceof Boolean b) {

[radeusgd]

Still, isn't while(true) a bit clearer than for(;;)?

[radeusgd]

Could you add a comment explaining what this method means?

My initial thought would be: "does the type have any inhabitants". But I see that Any::containsValues == false while Any - definitely has inhabitants (since every value is an inhabitant of Any). So this is something else. Is it something like does this type contain values that are direct inhabitants of it, excluding subtypes? But Number seems to violate that intuition as well. So I don't think I understand anymore. Would much appreciate some explanation in the function comment.

[radeusgd]

Seems like this assertion can be violated if someone calls getMetaParents when hasMetaParents returned false.

In that case we will throw an assertion error or usually when running without -ea we'll return some array with a null.

I think according to the getMetaParents contract defined in InteropLibrary we should instead throw UnsupportedMessageException.

[radeusgd]

Is a dataflow error an exception in the Truffle sense? IIRC We never throw it but just pass it around.

[JaroslavTulach]

Good question. It extends AbstractTruffleException, so it can be an exception. The throwException doesn't throw, but really returns it - e.g. it behaves as expected in Enso.

Moreover, when talking to Enso via org.graalvm.polyglot, it seems useful to be able to be able to check Value.isException() when one evaluates something.

I'd leave DataflowError as exception until we find it violates something.