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

Coercion type apparently does not check the actual type of the coerced value #6656

Closed
p6rt opened this issue Mar 14, 2018 · 11 comments
Closed

Coercion type apparently does not check the actual type of the coerced value #6656

p6rt opened this issue Mar 14, 2018 · 11 comments

Comments

@p6rt
Copy link

@p6rt p6rt commented Mar 14, 2018

Migrated from rt.perl.org#132980 (status was 'open')

Searchable as RT132980$

@p6rt
Copy link
Author

@p6rt p6rt commented Mar 14, 2018

From @briandfoy

I was playing with coercion types and wondered what would happen if
a .Int method did not return the right sort of type​:

  class Foo {
  method Int ( --> Str ) { 'Hello' }
  }

  put try-it( Foo.new );

  sub try-it ( Int() $n ) { "Got <$n> of type <{$n.^name}>" }

Although the subroutine signature demanded an Int, it accepted
something that claimed to be able to convert but actually didn't​:

  Got <Hello> of type <Str>

I would have expected the runtime constraint to check the ultimate
value against the type and this would have failed.

--
brian d foy <brian.d.foy@​gmail.com>
http://www.pair.com/~comdog/

Loading

@p6rt
Copy link
Author

@p6rt p6rt commented Mar 14, 2018

From @zoffixznet

On Wed, 14 Mar 2018 14​:16​:06 -0700, comdog wrote​:

I was playing with coercion types and wondered what would happen if
a .Int method did not return the right sort of type​:

class Foo \{
    method Int \( \-\-> Str \) \{ 'Hello' \}
    \}

put try\-it\( Foo\.new \);

sub try\-it \( Int\(\) $n \) \{ "Got \<$n> of type \<\{$n\.^name\}>" \}

Although the subroutine signature demanded an Int, it accepted
something that claimed to be able to convert but actually didn't​:

Got \<Hello> of type \<Str>

I would have expected the runtime constraint to check the ultimate
value against the type and this would have failed.

Thanks for the report.

We'll likely want to extend the features of the coercers, and include the return
type check, but not until 6.e or later language versions, when we have some
headroom with respect to performance.

I've documented[^1] that no check is currently performed on the result and there
are some extra proposed spec[^2] for coercers marked for 6.d review already, so I'll mark
this ticket as resolved. (though given 6.d is on already the horizon, the check of return
type would likely not make it into 6.d). The other feature we'd want to eventually implement
is coercing via `Target.new(Source)`, if not `Source.Target` method exists.

I've tried[^3] implementation[^4] of checking the final result in January and it
dropped performance of coercers by 8%. Given the marginal benefit offered by the
fix, we just can't afford to spend a 8% drop in a common feature at the moment.
So currently, it's just a "verbal" contract that method Foo returns object Foo,
similar how to `sub _foo` is a private method in Perl 5 by convention, despite it being public.

[1] Raku/doc@ee34834
[2] https://github.com/perl6/roast/blob/master/S12-coercion/coercion-types.t
[3] https://irclog.perlgeek.de/perl6-dev/2018-01-07#i_15661357
[4] https://gist.github.com/zoffixznet/6d0a2085bd343535b99f548d15547729

Loading

@p6rt
Copy link
Author

@p6rt p6rt commented Mar 14, 2018

The RT System itself - Status changed from 'new' to 'open'

Loading

@p6rt
Copy link
Author

@p6rt p6rt commented Mar 14, 2018

@zoffixznet - Status changed from 'open' to 'resolved'

Loading

@p6rt
Copy link
Author

@p6rt p6rt commented Jun 17, 2018

@zoffixznet - Status changed from 'resolved' to 'open'

Loading

@p6rt
Copy link
Author

@p6rt p6rt commented Jun 17, 2018

From @zoffixznet

Re-opening this, as there's a much more common case where a typecheck of the coercion result would be desired and that's when coercion results in a Failure​:

  <Zoffix_> m​: multi sub foo(Int() $) { }; foo Inf
  <camelia> rakudo-moar 5a6ff4073​: ( no output )

Loading

@raiph
Copy link

@raiph raiph commented Jul 7, 2021

Quoting brian's opening comment:

class Foo {
  method Int ( --> Str ) { 'Hello' }
  }

  put try-it( Foo.new );

  sub try-it ( Int() $n ) { "Got <$n> of type <{$n.^name}>" }

Although the subroutine signature demanded an Int, it accepted
something that claimed to be able to convert but actually didn't​:

Got <Hello> of type <Str>

I would have expected the runtime constraint to check the ultimate
value against the type and this would have failed.

In recent Rakudos the above code displays:

Impossible coercion from 'Foo' into 'Int': method Int returned an instance of Str

Loading

@lizmat
Copy link
Contributor

@lizmat lizmat commented Jul 7, 2021

So it would appear this can be closed?

Loading

@raiph
Copy link

@raiph raiph commented Jul 7, 2021

Quoting zoffix's last comment:

Re-opening this, as there's a much more common case where a typecheck of the coercion result would be desired and that's when coercion results in a Failure​:

  <Zoffix_> m​: multi sub foo(Int() $) { }; foo Inf
  <camelia> rakudo-moar 5a6ff4073​: ( no output )

That this works without an error is consistent with how the rest of Raku works. For example:

my Int() $foo := Failure.new: 'a';

silently works, as intended. This is the point of Failures, which the doc describes as "Delayed exception". So this is arguably not a problem.

Failures can be promoted to immediate exceptions:

use fatal;
my Int() $foo := Failure.new: 'a';

displays:

a
  in block <unit> at main.raku line 2

So perhaps this issue should be closed.

Loading

@lizmat lizmat closed this Jul 7, 2021
@lizmat
Copy link
Contributor

@lizmat lizmat commented Jul 7, 2021

Thanks for the headsup!

Loading

@raiph
Copy link

@raiph raiph commented Jul 7, 2021

I do wonder if this should promote the failure to an immediate exception:

my Int:D() $foo := Failure.new: 'a';

(One of the reasons I'm revisiting this issue is I'm pondering the whole Nil / Hole thing. My initial sense was that Hole was better, but your advocacy for Nil is forcing me to properly justify adding yet another special value, and that's pulling me toward just using Nil. But something doesn't smell right, and the :D case is something I'm looking at atm.)

While I really like the way Raku type objects play the role other PLs assign to Maybe aka Option, it feels like explicitly specifying a :D type smiley is about being explicitly strict, and in that context it currently feels to me like Failures ought perhaps be auto-promoted to being fatal (unless there's an explicit no fatal in effect?), and Nils ought resolve to whatever default, and, if that default isn't acceptable, then throw.

Loading

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

Successfully merging a pull request may close this issue.

None yet
3 participants