stevan / p5-mop-redux

A(nother) MOP for Perl 5
139 stars 36 forks source link

Syntax errors in methods cause misleading error messages #70

Closed latk closed 11 years ago

latk commented 11 years ago

During parsing of method bodies, syntax errors in the body cause the Parse::Keyword::parse_block function to return a false value. The MOP consideres the result an unimplemented method. Example:

use mop;
class Foo {
  method glurg {
    my $what = "glurg" # missing semicolon
    print "$what\n";
  }
}

Output:

syntax error at test.pl line 5, near "print"
Required method(s) [glurg] are not allowed in Foo unless class is declared abstract at /some/path/mop/class.pm line 157.

This could be fixed by checking the return value for truth, and aborting compilation if neccessary. E.g. at syntax.pm, line 359 it could read:

my $code = parse_stuff_with_values($preamble, \&parse_block)
  or die "Error while parsing body for $type $name in $CURRENT_CLASS_NAME. Will not continue.\n";

I'm not sure if this is the correct way to address this, but it would aid debuggability.

stevan commented 11 years ago

Hmm, this one is kind of tricky, while the "required method" error is clearly wrong doing what you propose will actually mask the legitimate error message. I will have to think about this a little.

stevan commented 11 years ago

I added in your test as a TODO test https://github.com/stevan/p5-mop-redux/commit/f432fd0eea40a2ffec2d23857a6c720c9c75e609

doy commented 11 years ago

Actually, because of the weird way that error messages are handled during parsing, it won't mask the real error message. I think we should do this.

stevan commented 11 years ago

@doy well it masks it with an eval. When I added the code @latk suggested, it made this test fail https://github.com/stevan/p5-mop-redux/blob/master/t/110-oddities/001-syntax-error.t#L11-L19.

latk commented 11 years ago

@stevan That test passes when we do this instead:

    my $code = parse_stuff_with_values($preamble, \&parse_block);
    if(!$code) {
        die $@ if $@;  # although this is not elegant
        die "Error while parsing body for $type $name in $CURRENT_CLASS_NAME. Will not continue.\n";
    }
doy commented 11 years ago

Yeah, you just have to actually propagate $@ if it's set. I'll push up the change.