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

Use BuiltinRootNode.ArgNode to extract argument for a builtin method #12201

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jan 30, 2025

Pull Request Description

  • as Polish to_text on intersection types #12192 (comment) states:
  • there is a need to extract values from EnsoMultiValue in the "builtin method prelude code"
  • to enable specializations based on type of arguments, we need to wrap such prelude by a Node
  • hence introducing ArgNode & co.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
  • Unit tests keep running
  • Engine benchmarks are OK: https://github.com/enso-org/enso/actions/runs/13068031263
  • Libs benchmarks are OK

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 30, 2025
@JaroslavTulach JaroslavTulach self-assigned this Jan 30, 2025
@JaroslavTulach JaroslavTulach marked this pull request as draft January 30, 2025 19:57
@JaroslavTulach JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Jan 30, 2025
public class Boolean extends Builtin {
public final class Boolean extends Builtin {
public Boolean() {
super(java.lang.Boolean.class);
Copy link
Member Author

Choose a reason for hiding this comment

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

Now we attempt to provide a representationType to each Builtin. That way we can deduce from signature methods having argument like boolean that the desired builtin is Boolean and the Enso type should be Standard.Base.Data.Boolean.Boolean. With that information we can extract (via CastToNode) such a type from EnsoMultiValue....

@JaroslavTulach
Copy link
Member Author

Right now there are two dataflow errors:

sbt:enso> runEngineDistribution --run test/Base_Tests/src/Semantic/Warnings_Spec.enso should.be.allowed.in.Vector|should.be.preserved.after.operations.on.Vector

[FAILED] Dataflow Warnings: [0/2, 770ms]

    - [FAILED] should be allowed in Vector [522ms]
        Reason: ['d', 'b', 'a'] did not equal [(Map_Error.Error 1 'b'), (Map_Error.Error 0 'a'), 'd']; first difference at index 0  (at Base_Tests/src/Semantic/Warnings_Spec.enso:272:9-126).


    - [FAILED] should be preserved after operations on Vector [248ms]
        Reason: ['warn4', 'warn3', 'warn2', 'warn1'] did not equal [(Map_Error.Error 3 'warn4'), (Map_Error.Error 2 'warn3'), (Map_Error.Error 1 'warn2'), (Map_Error.Error 0 'warn1')]; first difference at index 0  (at Base_Tests/src/Semantic/Warnings_Spec.enso:284:9-191).

I was debugging thru it, but I don't think I get any idea what can be the problem? Maybe something with wrapping errors? But why?

@JaroslavTulach
Copy link
Member Author

Currently there are two failures:

Again, I am not really sure why at present.

@@ -3,4 +3,8 @@
import org.enso.interpreter.dsl.BuiltinType;

@BuiltinType
public class Polyglot extends Builtin {}
public final class Polyglot extends Builtin {
public Polyglot() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd say we have way too many Builtins. Not sure why Polyglot or Debug should be builtin types at all!?

}
if (is(REQUIRES_CAST)) {
var ctx = EnsoContext.get(this);
if (this.ensoType == null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The first time ArgNode is requested to "cast", it searches for the ensoType representing the value of the argument. It would be better to do this when the ArgNode is being created, but it may be too early (before Standard.Base types are loaded in), so I haven't even tried.

Opinions?

@JaroslavTulach
Copy link
Member Author

I believe the code is in share to receive some inception review.

@JaroslavTulach JaroslavTulach marked this pull request as ready for review January 31, 2025 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant