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

Don't make signature part of a callable named parameter #4538

Merged
merged 5 commits into from Oct 5, 2021

Conversation

vrurg
Copy link
Member

@vrurg vrurg commented Sep 28, 2021

When a named parameter is declared as :&foo:(Int) the name used for
parameter binding was foo:(Int) resulting in falling back to
parameter's default - Callable. A visible effect of this error was
parameter checking to attempt calling signature method on Callable
role.

Resolves #4537

@vrurg
Copy link
Member Author

vrurg commented Sep 28, 2021

PR needs one more change. Declarations like :&foo:(Int) cannot be optional because they would fail to match against parameter default Callable. Need to add either a compile-time warning and make the parameter implicitly non-optional, or make it an error.

@vrurg
Copy link
Member Author

vrurg commented Sep 29, 2021

I have fixed the case when a signature-constrained named parameter results in signature method called on Callable if a routine is not supplied with corresponding named argument. The fix makes any such parameter required implicitly unless ? is used which is a compile-time error.

There is a couple of notes on feasibility of making signature-constrained parameters optional. The primary problem about them is that the default value (which is Callable) doesn't have a signature method. And even if it would have it (I was playing with making Code the default for signature-constrained parameters); and the method would return some valid signature object (nonsense, but what if?) - even then it is unlikely to match user's constraint.

There are possible workarounds.

The first is to mixin a custom role into the parameter default with a signature method which returns the user's constraint signature.

The second is to add $!signature attribute to the Parameter class. Comes at the cost of extra memory footprint only to take care of rather rarely used feature. A workaround to this, again, is to mixin a role with the attribute. In this case we would need to move argument signature validation code from a post-constraint where block into signature binding. The downside: it's a lot of work for the basically deprecated compiler code facing the replacement with RakuAST.

User-based solution for optional callable parameter within the approach taken by this PR would be :&foo where { .defined && .signature ~~ :(Int) }.

UPD Optional constrained parameters are now implemented.

@vrurg
Copy link
Member Author

vrurg commented Sep 29, 2021

There is a bug related to type captures:

sub foo(::T, &bar:(T)) { ... }

The constraining signature is not getting instantiated. Perhaps, instantiating post-constraints list would work as expected. If not then attaching the constraining signature onto the Parameter object would be a better solution.

@vrurg vrurg force-pushed the rakudo_4537 branch 2 times, most recently from dc7d516 to 32703ba Compare September 30, 2021 02:59
When a named parameter is declared as `:&foo:(Int)` the name used for
parameter binding was `foo:(Int)` resulting in falling back to
parameter's default - `Callable`. A visible effect of this error was
parameter checking to attempt calling `signature` method on `Callable`
role.
A major change this commit introduces is relocation of signature
object from inside a `where` block into an attribute of
`Parameter::SignatureConstraint` role which is mixed into `Parameter`
when necessary. Correspondingly, constraint is now checked as part of
signature binding when and only when the signature is used in parameter
declaration.

This commit also introduces support for generic signature constraints.
I.e.:

    sub foo(::T, :&fn:(T)) {...}

now works as expected.

1 TODO in t/spec/S03-smartmatch/signature-signature.t is unlocked,
but t/spec/S06-signature/closure-parameters.t is broken.
Mark multi-candidates with signature constrained parameters as
`bind_check` and `constrainty`. Resolve ambiguous matching.
vrurg added a commit to vrurg/roast that referenced this pull request Sep 30, 2021
rakudo/rakudo#4538 makes signature constraints work in signature
smartmatching.
vrurg added a commit to vrurg/roast that referenced this pull request Sep 30, 2021
Unfudge a now passing test. It needed to be corrected a little too
because by default parameters are typed with `Mu`, not `Any`.

Added tests to cover rakudo/rakudo#4537.

Added tests for newly implemented support for generics.

In support of rakudo/rakudo#4538
It was about wrong assumption that a new `Attribute` instance would be
allocated per each instance of `Parameter`. No idea what's got over me
when the idea came into my mind...

`$!signature_constraint` is now a permement member of `Parameter` class.
As we now have access to this information, .raku output would better
resemble the original declaration.
@vrurg
Copy link
Member Author

vrurg commented Oct 1, 2021

This PR is ready for merge. CI JVM failures are there on master, they are not caused by the PR.

Signature constraint information has been relocated from inside a when block in @!post_constraints list to a new attribute $!signature_constraint. Argument's signature is checked explicitly at parameter bind time. This move also allowed me to implement support for generics. So, sub foo(::T, &fn:(T)) {...} and role R[::T] { method foo(&fn:(T)) {...} } are both working now.

Aside of that, it should be possible now to consider the constraining signature in multi-dispatch deliberately, using it to better narrow down the selection of matching candidates. Unfortunately, I'm not sufficiently familiar with multi-dispatch guts to tweak them.

I have not found a reliable way to determine if a named parameter, or an optional positional, was actually passed with a routine call or not. For this reason I deduct the fact of it being missing by comparing parameter variable value to parameter's default type. It would be totally OK with the default &parameter declaration as Callable cannot be used as an argument for signature-constrained parameters. But if a trait changes Parameter $!type to something different this could become a potential problem.

src/Perl6/Actions.nqp Show resolved Hide resolved
@vrurg vrurg merged commit ec65ffc into rakudo:master Oct 5, 2021
@jnthn
Copy link
Member

jnthn commented Oct 6, 2021

Aside of this, I think the whole thing is going to be different with RakuAST anyway. It is likely that dispatching will come as part of RakuAST implementation.

So far as RakuAST goes, my expectation is that we'll compile every kind of signature (including unpacks and destructuring), only using the slow-path binder for error reporting.

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

Successfully merging this pull request may close these issues.

Cannot use a signature constraint on a named callable argument
3 participants