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

Type checks should be done after default is applied. #102

Closed
schwern opened this issue Jul 29, 2014 · 3 comments
Closed

Type checks should be done after default is applied. #102

schwern opened this issue Jul 29, 2014 · 3 comments
Assignees

Comments

@schwern
Copy link
Contributor

schwern commented Jul 29, 2014

use Method::Signatures;

func hello(
    Str $greeting //= "Hello",
    Str $place    //= "World"
) {
    print "$greeting, $place!\n";
}

# Hello, World!
hello();
# In call to main::hello(), the 'greeting' parameter (undef) is not of type Str at /Users/schwern/tmp/test.plx line 8.
hello(undef, undef);

The generated code for $greeting is this.

Method::Signatures->type_error('Str', $_[0], 'greeting')  if (@_ > 0) && !($Method::Signatures::mutc{cache}{'Str'} ||= Method::Signatures->_make_constraint('Str'))->check($_[0]);
my $greeting = !(@_ > 0) ? ( "Hello") : do{ no warnings; my $arg = $_[0]; $arg ~~ (undef) ? ( "Hello") : $arg };

Which boils down to that the type check is done before the default is applied. It should happen after the default is applied.

This also has the benefit of type checking the default avoiding this bug.

use Method::Signatures;

func hello(
    Object $greeting //= "Hello",
    Str $place    //= "World"
) {
    print "$greeting, $place!\n";
}

# Hello, World!
hello();
# In call to main::hello(), the 'greeting' parameter ("Hello") is not of type Object at /Users/schwern/tmp/test.plx line 8.
hello("Hello");
@schwern schwern self-assigned this Jul 29, 2014
@schwern
Copy link
Contributor Author

schwern commented Jul 29, 2014

The complexity is that the type check code is looking at $_[0] and not $place. This is easy enough to change.

The problem comes when the type check asks "should I do the type check"? If an argument is optional it will check to see if it exists first, but it will check @_ not $place to see if the optional argument was passed in. This misses that the argument has been set to a default.

I think the logic for "should I do a type check" should be...

  • Required argument, always check $variable.
  • Optional argument with no default, only check against $variable if it was passed in (using @_).
  • Optional argument with default, always check $variable.

@barefootcoder
Copy link
Contributor

Actually, I thought we talked about this before and decided that type-checking an explicit default value was unnecessary overhead, vaguely akin to type-checking the invocant (which, you may recall my pointing out, could also potentially catch bugs ;-> ).

Bu I suppose if there's a bug, that overrides any concerns about overhead. Your logic looks good to me. I'll merge the pull request and toss it out as a dev release.

barefootcoder added a commit that referenced this issue Aug 6, 2014
Type check *after* the default is applied.  For #102
@schwern
Copy link
Contributor Author

schwern commented Aug 6, 2014

Oh yeah, I did say that. Changing this to not type check default values at all (or at compile time) would make sense EXCEPT when the default is computed like $foo = some_function. We could probably use PPI to determine if an expression is a constant or not. I'll put in an issue for it.

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

No branches or pull requests

2 participants