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

Allow defaults in function signatures with syntax (..., p = e, ...). #30

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

m-kurtenacker
Copy link
Contributor

This hijacks the implicit summoning to also allow for defaults to be specified. Type checking infers a type for the supplied expression first, and then checks that the parameter matches. As a consequence, the following variants are all valid:

  • p : t = e
  • p = e : t
  • p = e, if the expression type can be inferred.

@m-kurtenacker m-kurtenacker marked this pull request as draft December 12, 2024 15:13
@m-kurtenacker
Copy link
Contributor Author

m-kurtenacker commented Dec 12, 2024

There is a potential issue when the other parameters are bound to the default expression:

fn test2(l : i32) { l }

fn test(a : i32, b = test2(a)) { a + b }

#[export]
fn main() -> i32 {
    test(4)
}

This code causes the scoping to break, which can be observed by a number of free parameters being found during thorin's internal verification.

Revert adding DefaultParamType. Instead, use CallExpr::infer.
This only works if the parameter and argument lists materialize as tuples.
Single parameters are not encapsulated like that and need special
treatment later down the line.
@m-kurtenacker
Copy link
Contributor Author

I changed the way defaults are propagated somewhat. The following example should work:

fn test(a : i32, b : i32, c = 5 : i32) {
    a + b + c
}

#[export]
fn main() -> i32 {
    test(1, 2) + test(1, 2, 3)
}

artic/src/check.cpp

Lines 1190 to 1191 in ed0d2d8

auto default_expr = default_param->default_expr.get();
args_tuple->args.push_back(std::unique_ptr<Expr>(default_expr));

I don't know of any good ways to copy an ast::Node the way it would be required here. The way this is currently implemented generated multiple unique pointers to the same address, leading to undefined behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant