- 
                Notifications
    You must be signed in to change notification settings 
- Fork 602
Implement named parameters in signatures (PPC0024) #23871
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
Conversation
f8ecfa6    to
    a82ff80      
    Compare
  
            
          
                pp.c
              
                Outdated
          
        
      | else { | ||
| // TODO: Consider collecting up all the names of unrecognised | ||
| // in one string | ||
| croak("Unrecognized named parameter '%s' to subroutine '%" SVf "'", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you change these croaks to Perl_croak_caller() the errors will include the line number of the caller rather than where the subroutine is defined which would be way more helpful (and in line with what signatures do without named parameters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namepv may or may not be utf8 encoded, and this message doesn't account for that.
croak_caller("Unrecognized named parameter '" UTF8f "' to subroutine '%" SVf "'",
                    UTF8fARG(SvUTF8(name), namepv), S_find_runcv_name());
If the above changes to fetch the UTF8 name this should pass true instead of SvUTF8(name).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| It seems that a slurp '@' doesn't allow you to pass extra parameters:  | 
| Extra parameters aren't detected if they have no key value pair. Consider: vs ... compared with normal sub sigs which gives "Too many arguments for subroutine 'main::foo'" | 
| This recurses into madness consuming memory:  | 
| if(!named->is_required || !SvPADSTALE(PAD_SVl(named->padix))) | ||
| continue; | ||
|  | ||
| // TODO: Consider collecting up all the names of missing | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please (and unrecognized ones above...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix those TODOs in a follow-up commit once the initial bit is merged. That's more of a "nice-to-have" feature.
|  | ||
| /* For now, build the named param array in iteration order. We'll | ||
| * sort it at the end. | ||
| */ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it actually sorted? If I do sub foo (:$z, :$x) {} foo(); the error I get is for 'z' missing, but I'd expect 'x'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah it's currently not sorted at all. Eventually it'll be sorted by order of the hash value - so not something that's useful for end users.
| 
 From reading the docs and the code, it seems like the intent is.. if you have named parameters then anything slurpy at the end should be key / values pairs even if it's  I'm not sure I agree with this. I think if you want k/v pairs still, use  | 
| With all of that said, I'm very excited for this! Thanks again! | 
| If we detect it's a named parameter can we give a more accurate error message? The above suggests   | 
| Something odd is happening with  If  | 
| I think declaring the same named parameter twice in one signature should be a compiler error. | 
| I think this should be a compiler error as well. It doesn't make sense to have optional positional parameters if there are required named parameters in the same signature because you can never call  | 
        
          
                pod/perlsub.pod
              
                Outdated
          
        
      | foo(%params); | ||
|  | ||
| As with positional parameters, named parameters may be declared with a | ||
| defaulting expression to provide a value to assign into the varible if the | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"variable"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
a82ff80    to
    e929c00      
    Compare
  
    | 
 Fixed. 
 Fixed. This was a while loop that decremented 2 from  
 Ahyes, that's because the argc-parity check comes quite early on. Here, it realises the parity of argc is wrong, so complains about that before it does anything else. 
 This is now fixed and a test added. Specifically, with a slurpy array and a single spare value, it checks only that value is pushed, and not an additional  
 Fixed, by adding a new message. | 
| 
 Fixed. 
 Fixed. 
 Hrmmm. Something tricky, probably because the  | 
| Reviewers: Thanks all for the many and varied comments. Besides one awkward bug to do with  | 
e929c00    to
    0928cf9      
    Compare
  
    0928cf9    to
    a3cdd57      
    Compare
  
    | 
 Actually, this is way weirder. I don't think it is related to the  Having added some debug prints, I can get this: It seems that when the anonymous  | 
a3cdd57    to
    e287ee0      
    Compare
  
    | Last bug now fixed by e287ee0 - turned out to be an action block in  I believe that's all the issues fixed now | 
| 
 I tested the pull request with Perl5::TestEachCommit. All commits PASSed on their own. LGTM. | 
e0d65bb    to
    7676a6b      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a review on docs' (perlsub.pod) spelling etc
        
          
                pod/perlsub.pod
              
                Outdated
          
        
      | passed by the caller when the subroutine is invoked. Whereas positional | ||
| parameters take their values from the corresponding position in the list | ||
| passed by the caller, named parameters are matched by name-value pairs | ||
| from the caller, where the name matches the variable name of the parameter | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think , (the comma) is not needed here before "where".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably reads better as a separate sentence even.
| Named parameters in signatures are currently considered B<experimental>, and | ||
| their behaviour may change in future releases of Perl. Uses of named | ||
| parameters in a signature will currently emit a warning in the | ||
| C<experimental::signature_named_parameters> category. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could instead be a link to that category ie. within the pod of the experimental pragma (so that the users could conveniently read more if needed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
experimental isn't (exactly) a core module, but also its documentation doesn't have separate sections per warning category. Nor does warnings.
        
          
                pod/perlsub.pod
              
                Outdated
          
        
      |  | ||
| To invoke this function, the caller must pass pairs of names and values to | ||
| provide values for the parameters. Each name must match the name of one of | ||
| the variables, omimtting its leading C<$> sigil. The order of these values | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"omitting", without the second M, also as previously I find the comma to be unnecessary and confusing here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
        
          
                pod/perlsub.pod
              
                Outdated
          
        
      | To invoke this function, the caller must pass pairs of names and values to | ||
| provide values for the parameters. Each name must match the name of one of | ||
| the variables, omimtting its leading C<$> sigil. The order of these values | ||
| is not important; both of the following lines will cause the same behaviour. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expected : (colon) rather than . (comma) in the end to introduce the following lines (code samples).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
| =item * | ||
|  | ||
| zero or more mandatory positional scalar parameters | ||
|  | ||
| =item * | ||
|  | ||
| zero or more optional positional scalar parameters | ||
|  | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe merge into a single list item as with the following named scalar parameters using OR (ie. mandatory or optional) for consistency in the Summary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No; the entire point here is that there's zero-or-more mandatory, followed by zero-or-more optional, in two distinct groups. They aren't all mixed together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
Just a few minor tweaks to some of the doc wording.
        
          
                pp.c
              
                Outdated
          
        
      |  | ||
| /* namepv / namelen are always UTF-8 */ | ||
| STRLEN namelen; | ||
| const char *namepv = SvPVutf8(name, namelen); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the callers SV is modifiable and isn't special (it won't overwrite references or GVs for example) this will upgrade it in place, modifying the caller's SV.
If the SV isn't utf8 you could use bytes_to_utf8_free_me() or bytes_to_utf8_temp_pv() to get the UTF8 version cheaply:
const char *namepv = SvPV(name, namelen);
if (!SvUTF8(name))
    namepv = bytes_to_utf8_temp_pv(namepv, &namelen);
which saves the cost of making a new SV, and bytes_to_utf8_temp_pv() is smart about invariants (it will return namepv if the string contains only invariants).
The _temp_pv() variant uses SAVEFREEPV() to ensure the namepv is release (if allocated), alternatively you can use the _free_me() variant where you need to Safefree() the allocation, but the croak() makes that difficult to use here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahyes, now changed and added a test to check that non-UTF8 SVs from the caller are not upgraded like that any more.
2786df0    to
    7cc7882      
    Compare
  
    | 
 Thanks, now all applied. | 
| If reviewers are happy their comments have been addressed, I shall squash all the fixup commits back down to a single one | 
7cc7882    to
    730a582      
    Compare
  
    | Intending to merge on Friday (31st oct) if no further comments | 
This adds a major new ability to subroutine signatures, allowing callers
to pass parameters by name/value pairs rather than by position.
  sub f ($x, $y, :$alpha, :$beta = undef) { ... }
  f( 123, 456, alpha => 789 );
Originally specified in
  https://github.com/Perl/PPCs/blob/main/ppcs/ppc0024-signature-named-parameters.md
This feature is currently considered experimental.
    730a582    to
    e299bf2      
    Compare
  
    
This adds a major new ability to subroutine signatures, allowing callers to pass parameters by name/value pairs rather than by position.
Originally specified in
https://github.com/Perl/PPCs/blob/main/ppcs/ppc0024-signature-named-parameters.md
This feature is currently considered experimental.