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

Unreported type inference error with Array's in match case statements #4348

Open
redvers opened this issue Apr 26, 2023 · 9 comments
Open

Unreported type inference error with Array's in match case statements #4348

redvers opened this issue Apr 26, 2023 · 9 comments
Labels
bug Something isn't working help wanted Extra attention is needed needs investigation This needs to be looked into before its "ready for work"

Comments

@redvers
Copy link
Contributor

redvers commented Apr 26, 2023

Edited to put the minimal case up top instead of in the comments.

Running ponyc 0.54.0
Minimal case:

actor Main
  new create(env: Env) =>
    let t: Array[U8] val = [ 0x00 ; 0x01 ; 0x02 ]
    match t
    | [ 0x00 ; 0x00 ; 0x00 ] => false
    | [ 0x00 ; 0x01 ; 0x02 ] => true
    end

Here's how it looks:

[nix-shell:~/projects/pony/ssssss]$ ponyc --debug -r=refer
Building builtin -> /nix/store/nnvr2l04kdk62n633g74p0vdwf9bbrmy-ponyc-0.54.0/lib/pony/0.54.0/packages/builtin
Building . -> /home/red/projects/pony/ssssss

[nix-shell:~/projects/pony/ssssss]$ ponyc --debug -r=expr
Building builtin -> /nix/store/nnvr2l04kdk62n633g74p0vdwf9bbrmy-ponyc-0.54.0/lib/pony/0.54.0/packages/builtin
Building . -> /home/red/projects/pony/ssssss
Error: internal failure not reported
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Apr 26, 2023
@SeanTAllen
Copy link
Member

SeanTAllen commented Apr 27, 2023

Here's a more minimal example that gives an error:

class Fu
  fun bar(t: Array[U8] val) =>
    match t
    | [ U8(1) ] => false
    end

which results in:

main.pony:4:7: couldn't find 'eq' in 'Array'
    | [ U8(1) ] => false
      ^
Error:
main.pony:4:7: this pattern element doesn't support structural equality
    | [ U8(1) ] => false
      ^

If you remove the type info for the array:

class Fu
  fun bar(t: Array[U8] val) =>
    match t
    | [ 1 ] => false
    end

you get the original error message which is the default "it says there was an error but we got no message for it" message.

You can replicate this with the original example by making the following changes:

actor Main
  new create(env: Env) =>
    let t: Array[U8] val = [ 0x00 ; 0x01 ; 0x02 ]
    match t
    | [ U8(0x00) ; U8(0x00) ; U8(0x00) ] => false
    | [ U8(0x00) ; U8(0x01) ; U8(0x02) ] => true
    end

The missing error message is likely some form of "could not infer literal type, no valid types found" that you get when you do something like:

class Fu
  fun bar() =>
    let x = [ 1 ]

but in this case, in the match, it appears we don't give a nice error message for it.

@SeanTAllen SeanTAllen added help wanted Extra attention is needed bug Something isn't working needs investigation This needs to be looked into before its "ready for work" labels Apr 27, 2023
@redvers
Copy link
Contributor Author

redvers commented Apr 27, 2023

I wondered if eq ended up involved somehow (and part of the reason I asked about that in a separate thread on zulip).

That makes sense. Thanks.

So this is going to be a "turn into a compile-time error" type of solution I expect since implementing literal arrays in a match is a problem that is significantly bigger than a breadbox.

@SeanTAllen
Copy link
Member

Note this is reproducible with Pony 0.54.1

@SeanTAllen
Copy link
Member

So this is going to be a "turn into a compile-time error" type of solution I expect since implementing literal arrays in a match is a problem that is significantly bigger than a breadbox.

I'm not sure what you mean by "implementing literal array in a match". That isn't the bug here, the bug is that you aren't getting the error message that you should.

@SeanTAllen
Copy link
Member

Note this is a variation of #2110 and it appears we missed a case.

@SeanTAllen
Copy link
Member

This is hitting

  if(is_typecheck_error(pattern_type))
    return NULL

in match.c

https://github.com/ponylang/ponyc/blob/main/src/libponyc/expr/match.c#L56

We should never hit a typecheck error here, that it should have been caught before this point and the check is only a sanity check. This should have been caught and handled before we got to the make_pattern_type function.

In this particular case, pattern_type is coming back as null.

So, the preceeding ast_t* pattern_type = ast_type(pattern) is returning null, which I really wasn't expecting.

This is happening because

ast->annotation_type;

returns NULL in ast_type so the issue is, why is annotation_type NULL.

Based on everything here, I'm fairly confident that the "could not infer literal type, no valid types found" should have caught this already and we would never get here.

@SeanTAllen
Copy link
Member

Note when using a debug version of the compiler you will get:

/home/sean/code/ponylang/ponyc-3/src/libponyc/pass/expr.c:661: pass_expr: Assertion `errors_get_count(options->check. Errors) > 0` failed.

as the error message.

@SeanTAllen
Copy link
Member

Looking in expr_case, I see the following:

  if(ast_checkflag(match_expr, AST_FLAG_JUMPS_AWAY) ||
    is_typecheck_error(match_type))
    return false;

and the jumps away never gets handled until make_pattern_type:

  if(ast_checkflag(pattern, AST_FLAG_JUMPS_AWAY))
  {
    ast_error(opt->check.errors, pattern,
      "not a matchable pattern - the expression jumps away with no value");
    return NULL;
  }

so I think for this code it is reasonable that we need to note that we don't have a type as a check after the jumps away check. But I'd like to discuss with @jemc.

@SeanTAllen SeanTAllen changed the title Error: internal failure not reported (potentially in expr phase) Unreported type interference error with Array's in match case statements Apr 28, 2023
@SeanTAllen SeanTAllen changed the title Unreported type interference error with Array's in match case statements Unreported type inference error with Array's in match case statements May 9, 2023
@SeanTAllen SeanTAllen self-assigned this May 9, 2023
@jemc
Copy link
Member

jemc commented May 9, 2023

My suggestion:

In the find_antecedent_type function in expr.c, add another case to the switch that shows it how to find the proper antecedent if it determines that the AST in question is a match case pattern expression (which I think would be noticing that the ast_parent is a TK_CASE), and in that situation it could find the appropriate antecedent - the expression passed in to be matched (in this case, the variable t).

@SeanTAllen SeanTAllen removed the discuss during sync Should be discussed during an upcoming sync label May 30, 2023
@SeanTAllen SeanTAllen removed their assignment Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed needs investigation This needs to be looked into before its "ready for work"
Projects
None yet
Development

No branches or pull requests

4 participants