jakt icon indicating copy to clipboard operation
jakt copied to clipboard

Don't infer return type in function/method blocks

Open 0GreenClover0 opened this issue 3 years ago • 12 comments

A spiritual ancestor of #748 and #215. Fixes #151 in case incompatible return types are actually reachable (which seemed to be the issue, given the title). #1133 follow-up.

Resolves #1092 and resolves #1093 because we no longer infer the return type for function blocks. Fixes #483.

Not sure about the generic stuff, please check :^)

I don't know how to put a commit description without commit linter complaining, so I'll just paste it here:

This refers to functions as well as methods that are not fat-arrow. Parser and a couple of tests needed to be updated in some cases, to specify the return type. When the return type is not specified for not-fat-arrow functions, it is set to void.

0GreenClover0 avatar Sep 02 '22 13:09 0GreenClover0

Might have fixed that for methods too. I'll test a little bit and push when I'm done

0GreenClover0 avatar Sep 02 '22 16:09 0GreenClover0

Hello!

One or more of the commit messages in this PR do not match the Jakt code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why. Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

BuggieBot avatar Sep 02 '22 17:09 BuggieBot

@jntrnr I've added some comments and deleted some debug variables / checks that I forgot about (oops...). Sorry for the mess, it must have been a pain to understand the changes.

~I've also added one fixme, because even after this PR we still don't check properly the type of make_memo() in /tests/typechecker/generic_across_scopes.jakt. The tests itself is unfortunately way above my head and I don't know what the exact problem is. If the PR gets merged I'll probably add an issue for that~ fixed

0GreenClover0 avatar Sep 03 '22 23:09 0GreenClover0

we still don't check properly the type of make_memo()

That has an explicit return type, and is correctly typechecked on main - what fails?

alimpfard avatar Sep 03 '22 23:09 alimpfard

we still don't check properly the type of make_memo()

That has an explicit return type, and is correctly typechecked on main - what fails?

Nah, it doesn't fail the test itself ~because we return early in that case~, but ~this means that~ when you change

function make_memo<R, S>() throws -> Memo<R, S> {
    let value: [R:S] = [:]
    return Memo(value)
}

to... for example this:

function make_memo<R, S>() throws -> Memo<R, S> {
    let value: [R:S] = [:]
    return ""
}

It doesn't complain and generates invalid cpp code (even on main)

0GreenClover0 avatar Sep 04 '22 07:09 0GreenClover0

Pushed up the changes. If I remove the or .get_type(checked_expr.type()) is TypeVariable on line 3824 in the early return, the only failing test now is generic_across_scopes.jakt with the error

----- ./tests/typechecker/generic_across_scopes.jakt:28:13
 27 |         if memo!.value.contains(x) { 
 28 |             return memo!.value[x] 
                  ^- Type mismatch: expected ‘i64’, but got ‘V’
 29 |         } 
-----

0GreenClover0 avatar Sep 04 '22 08:09 0GreenClover0

It might not even be related to the is TypeVariable early return Am I dumb or is this error getting swallowed? This does not print any Jakt errors and gets past the codegen stage, where invalid C++ is emitted

obraz

0GreenClover0 avatar Sep 04 '22 10:09 0GreenClover0

You learn something everyday obraz

Not sure if this is the way, but I set ignore_errors to false before checking for type compatibility in typecheck_return() and after the check I restore its value. Thanks to this, we properly error when the return type of make_memo is wrong. I've added a test case for it as well.

I don't think there are any other known issues.

0GreenClover0 avatar Sep 04 '22 11:09 0GreenClover0

The first pass over generic functions should ignore errors - that's expected The second specialised pass (on call) should not be ignoring it though

alimpfard avatar Sep 04 '22 12:09 alimpfard

So is there a bug somewhere? It seems to be set to true both times we call typecheck_return for make_memo

0GreenClover0 avatar Sep 04 '22 13:09 0GreenClover0

There seems to be a bug for sure.

function make_memo<R, S>() throws -> Memo<R, S> {
    let x: String = 32 
    let value: [R:S] = [:]
    return Memo(value)
}

Assigning i64 to String gives us...

root@CloverLaptop:/home/jakt# ./build/jakt ./tests/typechecker/generic_across_scopes.jakt internal error: Garbage statement in codegen Aborted

That's on main.

0GreenClover0 avatar Sep 04 '22 13:09 0GreenClover0

It turns out we don't really typecheck functions like make_memo at all currently, so I've removed the hack with setting .ignore_errors = false, I don't think it would work in all cases regardless.

0GreenClover0 avatar Sep 04 '22 20:09 0GreenClover0

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

stale[bot] avatar Jan 01 '23 12:01 stale[bot]

This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions!

stale[bot] avatar Jan 09 '23 05:01 stale[bot]