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

Implement named constructor #188

Merged
merged 2 commits into from
Feb 23, 2024
Merged

Conversation

ehllie
Copy link
Contributor

@ehllie ehllie commented Feb 7, 2024

Attempts to address #163

src/nameresolution/mod.rs Outdated Show resolved Hide resolved
@ehllie ehllie force-pushed the named-constructor branch 6 times, most recently from d16e8a0 to e1a7f04 Compare February 8, 2024 00:59
@ehllie ehllie changed the title Implement named constructor frontend Implement named constructor Feb 8, 2024
@ehllie ehllie marked this pull request as ready for review February 8, 2024 22:11
After jfecher#186 all module paths should be absolute, and as such it's not necessary to prefix them with ./
@ehllie
Copy link
Contributor Author

ehllie commented Feb 8, 2024

There seems to be a bug in the cranelift backend that I don't fully understand. It compiling the examples/typechecking/named_constructor.an file:

$ ante -d --backend llvm examples/typechecking/named_constructor.an
ld: warning: object file (/Users/ellie/Code/ante/examples/typechecking/named_constructor.o) was built for newer macOS version (14.0) than being linked (11.0)

$ ante -d --backend cranelift examples/typechecking/named_constructor.an
thread 'main' panicked at src/cranelift_backend/context.rs:197:13:
- inst0 (v1, v2, v3 = call fn0(v0, v1, v0)): uses value v1 from itself

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

It works while using the llvm backend, so it does not seem to be directly related to this feature, and I remember you mentioning making big changes to the backend soon, so I feel like this PR should be good as is for now.

@jfecher
Copy link
Owner

jfecher commented Feb 8, 2024

Hmm, I don't think this PR should affect the backend at all. I wonder if it has an issue that is going unnoticed until it triggers a check in cranelift? I've never seen the "uses value v1 from itself" error before though. Usually it is a type error or using a value before it is defined. I'd double-check the output (specifically the definition ids for each variable) produced by this desugaring by printing out the monomorphized IR.

I believe the only backend changes I had in mind were for the algebraic effects branch so it will still be some time before that is merged.

@jfecher
Copy link
Owner

jfecher commented Feb 23, 2024

What is the status on this PR?

@ehllie
Copy link
Contributor Author

ehllie commented Feb 23, 2024

I had misread you saying that you'd look into the generated IR. The HIR generated is the following

$ ante -e hir examples/typechecking/named_constructor.an

();
x_v0 = ("Hello World", 11_usz);
();
foo_v6 = (hello_foo_v1 42_i32)


hello_foo_v1 = (fn v2 : i32 -> {{Ptr, usz}, i32} =
        (Foo_v5 x_v0 y_v2)
)

Foo_v5 = (fn v3 v4 : {Ptr, usz} -> i32 -> {{Ptr, usz}, i32} = (v3, v4))

But generating the cranelift IR panics

$ ante -e ir examples/typechecking/named_constructor.an
main =
function u0:0() -> i32 apple_aarch64 {
    gv0 = symbol colocated u1:0
    sig0 = (i32) -> i64, i64, i32 fast
    fn0 = u0:1 sig0

block0:
    v0 = symbol_value.i64 gv0
    v1 = iconst.i64 11
    v2 = iconst.i32 42
    v3, v4, v5 = call fn0(v2)
    v6 = iconst.i32 0
    return v6
}

lambdav1 =
function u0:1(i32) -> i64, i64, i32 fast {
    sig0 = (i64, i64, i32) -> i64, i64, i32 fast
    fn0 = u0:2 sig0

block0(v0: i32):
    v1, v2, v3 = call fn0(v0, v1, v0)
    return v1, v2, v3
}

thread 'main' panicked at src/cranelift_backend/context.rs:197:13:
- inst0 (v1, v2, v3 = call fn0(v0, v1, v0)): uses value v1 from itself

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

In all honesty, I'm feeling somewhat out of my depth with debugging codegen

@jfecher
Copy link
Owner

jfecher commented Feb 23, 2024

@ehllie ah, sorry about that, it has been some time and I just forgot where things stood on this PR. I can take it over and do some debugging if that'd be simpler for you. I'll make the PR on your fork when that is done, sorry for the confusion.

@jfecher
Copy link
Owner

jfecher commented Feb 23, 2024

Alright, the named_constructors test is just exhibiting the existing issue from #142 since hello_foo is capturing the global there. Instead of making a PR on your fork I'm going to merge this since the PR looks good to me and I can edit the test to remove the bug afterward.

Thank you (and sorry for the delay)!

Copy link
Owner

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

👍

@jfecher jfecher merged commit b3d9609 into jfecher:master Feb 23, 2024
1 check passed
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.

2 participants