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

Parse inline function signatures #8470

Merged
merged 8 commits into from
Dec 12, 2023
Merged

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented Dec 6, 2023

Pull Request Description

Implements #6166.

Important Notes

  • More consistent handling of default arguments. default is a valid identifier, and only has special meaning when it isn't bound in scope. Since distinguishing the builtin default from an identifier called default cannot be done until alias analysis has been performed, default is now represented in the AST as a regular identifier.
  • TreeToIr: Remove insideTypeAscription. It was only used for bug-for-bug compatibility with the old parser during the transition.

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,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@kazcw kazcw self-assigned this Dec 6, 2023
@kazcw kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label Dec 6, 2023
@kazcw kazcw marked this pull request as ready for review December 6, 2023 05:58
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

This PR claims to fix #6166. It seems to do so, but it also does something with default. That doesn't seem to be in scope of #6166...

More consistent handling of default arguments. default is a valid identifier, and only has special meaning when it isn't bound in scope. Since distinguishing the builtin default from an identifier called default cannot be done until alias analysis has been performed, default is now represented in the AST as a regular identifier.

... OK, can we see how the default behaves in the runtime? For example by modifying the ExecCompilerTest?

("->" (Ident Integer))
"=" (Ident that)));
// Edge case
test!("number -> Integer = 23",
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add such a test to ExecCompilerTest?

@kazcw
Copy link
Contributor Author

kazcw commented Dec 6, 2023

@JaroslavTulach I added tests for default. One is @Ignored for now, because it seems default is not implemented yet. Opened a compiler bug: #8480.

Comment on lines +202 to +209
@Test
public void inlineReturnSignatureWithoutArguments() throws Exception {
var module = ctx.eval("enso", """
the_number -> Integer = 23
""");
var result = module.invokeMember("eval_expression", "the_number");
assertEquals("Function-return syntax can be used with 0 arguments", 23, result.asInt());
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also get a test when there is more than 0 arguments 🙂

I'd be happy to add a test case, may I push to your branch?

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to add a following patch:

Subject: [PATCH] add 2 more compiler test cases
---
Index: engine/runtime/src/test/java/org/enso/compiler/ExecCompilerTest.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/engine/runtime/src/test/java/org/enso/compiler/ExecCompilerTest.java b/engine/runtime/src/test/java/org/enso/compiler/ExecCompilerTest.java
--- a/engine/runtime/src/test/java/org/enso/compiler/ExecCompilerTest.java	(revision 45ed77d513735bce59e2c8739a55a38560a69ccb)
+++ b/engine/runtime/src/test/java/org/enso/compiler/ExecCompilerTest.java	(revision 7273a5c43575d2e913caa18f9a4edc071d6df135)
@@ -199,6 +199,29 @@
     assertEquals("10 % 3 is one", 1, result.asInt());
   }
 
+  @Test
+  public void inlineReturnSignature() throws Exception {
+    var module = ctx.eval("enso", """
+    foo (x : Integer) (y : Integer) -> Integer = 10*x + y
+    """);
+    var foo = module.invokeMember("eval_expression", "foo");
+    assertTrue("foo a function", foo.canExecute());
+    assertEquals(45, foo.execute(4, 5).asInt());
+  }
+
+  @Test
+  public void inlineReturnSignatureOnMemberMethod() throws Exception {
+    var module = ctx.eval("enso", """
+    type My_Type
+        Value x
+        
+        foo self (y : Integer) z -> Integer = 100*z + 10*y + self.x
+    """);
+    var instance = module.invokeMember("eval_expression", "My_Type.Value 1");
+    var result = instance.invokeMember("foo", 2, 3);
+    assertEquals(321, result.asInt());
+  }
+
   @Test
   public void inlineReturnSignatureWithoutArguments() throws Exception {
     var module = ctx.eval("enso", """

Tests are passing with it.

@kazcw May I push it to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@radeusgd Sure.

@kazcw kazcw added the CI: Ready to merge This PR is eligible for automatic merge label Dec 12, 2023
@mergify mergify bot merged commit ce6c770 into develop Dec 12, 2023
34 of 35 checks passed
@mergify mergify bot deleted the wip/kw/parse-inline-signatures branch December 12, 2023 14:48
var returnSignature = fn.getReturns();
if (returnSignature != null) {
var returnType = translateType(returnSignature.getType());
// TODO(#8240): Make use of return type declaration.
Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-gui -parser CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants