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

Builtin methods should support default parameters #5312

Closed
wdanilo opened this issue Feb 5, 2023 · 1 comment
Closed

Builtin methods should support default parameters #5312

wdanilo opened this issue Feb 5, 2023 · 1 comment
Labels
-compiler -syntax p-low Low priority x-new-feature Type: new feature request

Comments

@wdanilo
Copy link
Member

wdanilo commented Feb 5, 2023

This task is automatically imported from the old Task Issue Board and it was originally created by Hubert Plociniczak.
Original issue is here.


It is currently not possible to declare a builtin method with defaults:

    sort self (comparator = (_.compare_to _)) = @Builtin_Method "Array.sort"

Instead you have to write

    sort self (comparator = (_.compare_to _)) = self.sort_builtin comparator

    sort_builtin self comparator = @Builtin_Method "Array.sort_builtin"

This pattern is present all over the stdlib.

Comments:

After some investigation. When the sort_builtin pattern works, then the stacktrace that allocates ArgumentDefinition is:

"main"
	at org.enso.interpreter.runtime.callable.argument.ArgumentDefinition.<init>(ArgumentDefinition.java:54)
	at org.enso.compiler.codegen.IrToTruffle$DefinitionArgumentProcessor.run(IrToTruffle.scala:1862)
	at org.enso.compiler.codegen.IrToTruffle$ExpressionProcessor$BuildFunctionBody.$anonfun$computeSlots$1(IrToTruffle.scala:1503)
	at org.enso.compiler.codegen.IrToTruffle$ExpressionProcessor$BuildFunctionBody.$anonfun$computeSlots$1$adapted(IrToTruffle.scala:1501)
	at org.enso.compiler.codegen.IrToTruffle$ExpressionProcessor$BuildFunctionBody$$Lambda$1322.2106129052.apply
	at scala.collection.immutable.List.map(List.scala:250)
	at org.enso.compiler.codegen.IrToTruffle$ExpressionProcessor$BuildFunctionBody.computeSlots(IrToTruffle.scala:1501)
	at org.enso.compiler.codegen.IrToTruffle$ExpressionProcessor$BuildFunctionBody.slots$lzycompute(IrToTruffle.scala:1465)
	at org.enso.compiler.codegen.IrToTruffle$ExpressionProcessor$BuildFunctionBody.slots(IrToTruffle.scala:1465)
	at org.enso.compiler.codegen.IrToTruffle$ExpressionProcessor$BuildFunctionBody.args(IrToTruffle.scala:1468)
	at org.enso.compiler.codegen.IrToTruffle.$anonfun$processModule$15(IrToTruffle.scala:401)
	at org.enso.compiler.codegen.IrToTruffle.$anonfun$processModule$15$adapted(IrToTruffle.scala:332)
	at org.enso.compiler.codegen.IrToTruffle$$Lambda$1317.1720332964.apply
	at scala.Option.foreach(Option.scala:437)
	at org.enso.compiler.codegen.IrToTruffle.$anonfun$processModule$9(IrToTruffle.scala:332)

however, when the default value is ignored, then the ArgumentDefinition is allocated completely differently:

"main"
	at org.enso.interpreter.runtime.callable.argument.ArgumentDefinition.<init>(ArgumentDefinition.java:54)
	at org.enso.interpreter.runtime.callable.argument.ArgumentDefinition.<init>(ArgumentDefinition.java:39)
	at org.enso.interpreter.node.expression.builtin.number.integer.ParseIntegerMethodGen.makeFunction(ParseIntegerMethodGen.java:44)
	at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(NativeMethodAccessorImpl.java)
	at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:566)
	at org.enso.interpreter.runtime.builtin.Builtins$LoadedBuiltinMethod.toFunction(Builtins.java:616)
	at org.enso.interpreter.runtime.builtin.Builtins.getBuiltinFunction(Builtins.java:386)
	at org.enso.compiler.codegen.IrToTruffle.$anonfun$processModule$15(IrToTruffle.scala:362)
	at org.enso.compiler.codegen.IrToTruffle.$anonfun$processModule$15$adapted(IrToTruffle.scala:332)

probably we would have to somehow enhance ParseIntegerMethodGen code. Btw. line numbers correspond to:

3a09e2d5bc67fabeeb73b0d42189120e657f6b6 (origin/develop, origin/HEAD, develop)
Author: Adam Obuchowicz <[email protected]>
Date:   Wed Nov 30 07:24:54 2022 +0100

    Fix node still editing after accepting Expression Input (#3905)
    
    So far this branch contains more diagnostics, and PR is for building PRs for @sylwiabr .

(jaroslavtulach - Nov 30, 2022)


It would also be good to support such defaults in atom constructors of builtins too.

I wanted to move Illegal_Argument to be a builtin (as I want to throw it from another builtin method) and could not do it, because

type Illegal_Argument
    Error message cause=Nothing

the constructor has a default argument.

I'm implementing a workaround, but ideally it would be good to be able to move types whose constructors have default arguments to be builtins if necessary.

Still, it probably has lower priority than defaults for methods which are supposedly more common. (Radosław Waśko - Dec 21, 2022)


@jdunkerley
Copy link
Member

We should do this by wrappers.

@jdunkerley jdunkerley closed this as not planned Won't fix, can't repro, duplicate, stale Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-compiler -syntax p-low Low priority x-new-feature Type: new feature request
Projects
None yet
Development

No branches or pull requests

3 participants