search

Raku / problem-solving

🦋 Problem Solving, a repo for handling problems that require review, deliberation and possibly debate
Artistic License 2.0
70 stars 16 forks source link

Uncertain status of tests in the roast #217

Open vrurg opened 4 years ago

vrurg commented 4 years ago

After sorting out test not included into spectest.data (see Raku/roast#657) I ended up with a number of cases where it is uncertain what to do with them. Because most of the doubts are caused by questionable status of some language features, syntaxes, or semantics I put this under the language label.

The list of problematic tests following.

1. S13-overloading/fallbacks-deep.t

is deep trait for a operator routine which is supposed to autogenerate different variants of the op. Looks like it is obsoleted by meta-ops. Even the related synopsis states:

[Note: the following section on "is deep" may no longer be necessary given the way metaoperators are now constructed.]

Proposal: to remove

2. S13-overloading/multiple-signatures.t, S06-signature/multiple-signatures.t

Tests for:

    multi sub three  (Str $s, Int $i, Num :$n)
                   | (Int $i, Str :$s, Num :$n)
                   | (Num :$s, Int :$i, Str :$n) {
        "$s $i $n";
    }

Is this syntax is still planned?

Proposal: fudge the test with eval (I made it so that evaled tests will be reported as passing TODOs now) and add to spectest.data.

3. S01-perl-5-integration/modify_inside_p5.t and S01-perl-5-integration/modify_inside_p5_p6.t

{
  use v5;
  $x .= 'ing';
 {
    use v6;
    $y ~= 'book';
  }
}

Our current design prohibits language version switching inside a compunit. I don't see why language switching should be different.

Proposal: to remove

4. S12-traits/basic.t and S12-traits/parameterized.t

Tests are using this syntax:

  multi sub trait_auxiliary:<is>(cool $trait, Any $container:) {   #OK not used

It's not even defined in synopses. Not that I found a mention of it.

Proposal: to remove

5. S03-operators/shortcuts.t

class C {
  method @.[$i] { $i }
  method %.{$k} { $k }
  method &.($a) { 'karma ' ~ $a }
}
C.new[42] == 42;

Is it still planned? Looks nice, should be implementable by converting each corresponding shortcut into AT-POS, AT-KEY, and CALL-ME methods.

Proposal: fudge with eval

6. S12-class/open_closed.t

Allow or prevent class augmentation.

Likely requires reconsideration. Current Rakudo implementation tends more towards openness of classes. Perhaps closing semantics and syntax would eventually have different implementation.

Proposal: remove it or leave and mark as "For historical reference"

7. S13-syntax/aliasing.t

    my class Baz {
        method bar() { 42 }
        our &baz ::= &bar;
    }

Requires implementation of &name for methods and ::=. While ::= just awaits for its time to come, &method is a controversial thing. Not to mention that our is rather related to class methods.

Another related consideration is that for aliasing within a class a trait fits better (see Method::Also):

method bar() is also('baz') { 42 }

But for subclasses we need another syntax.

I have fudged the with eval and two canary tests for now.

Proposal: to remove

8. S24-testing/2-force_todo.t

    force_todo(1, 3, 5, 7 .. 9, 11);

Awaits for &Test::force_todo. But the way the test expects the feature to work looks plain awful to me as it requires binding to particular test numbers which may change easily.

Proposal: reconsider the feature

9. S24-testing/1-basic.t

Depends on resolution of #215.

Proposal: remove all tests with :desc and :todo

10. S10-packages/require-and-use--dead-file.t

Tests for a module return value. But in Raku a module returns nothing.

Proposal: to remove

11. S05-capture/hash.t

"  a b\tc" ~~ m/%<chars>=( \s+ \S+ )/

Getting The use of hash variables in regexes is reserved. No idea what is planned for this.

The semantics of this feature looks rather fuzzy and confusing to me.

Proposal: to remove

12. S05-capture/external-aliasing.t

my $x;
our $y;

ok 'ab cd ef' ~~ m/:s <ident> $x=<ident> $y=<ident>/, 
   'regex matched';

Not sure if planned for implementation.

Proposal: none

13. S11-modules/use-perl-6.t

use Perl:ver<6.*>;

Does it ever make sense to implement the construct? In either case, it can only appear as the first statement in a compunit.

Proposal: none

14. S05-nonstrings/basic.t

class Dog {};
regex canine { <.isa(Dog)> };
Dog.new ~~ /<canine>/;

Current Rakudo implementation would stringify ~~ lhs rendering the feature plain impossible. Not sure if we can change this semantics without severely breaking the ecosystem.

Proposal: to remove

15. S11-modules/re-export.t

Re-export of symbols with use Foo :ALL, :EXPORT; syntax. Perhaps the actual implementation would differ.

Proposal: postpone

16. S12-attributes/trusts.t

class A {
    trusts B;
    has $!foo = 42;
}
class B {
    has A $.a .= new;
    method foo {
        say $!a!A::foo; # 42
    }
}

Not sure if it's still planned. Would be good to have this implemented.

Proposal: fudge with eval.

17. S06-signature/slurpy-blocks.t

It seems to me that we have this implemented by passing a block as a Code object. See no use in foo():{ 42 } syntax.

Proposal: to remove

18. S06-macros/returning-string.t, S06-macros/postfix.t

The tests are likely to be reconsidered upon release of RakuAST.

Proposal: postpone with high likeliness of rewriting for different syntax

19. S04-phasers/exit-in-check.t

Tests for exit called in a CHECK phaser to invoke END phaser. Currently fails. Not sure what semantics is expected, but I'd consider END to be invoked always.

Proposal: fudge until the expected behavior is implemented.

20. S04-statements/goto.t

A big one raising many questions. For example:

Perhaps worth separate discussion.

Proposal: postpone

21. S04-statements/lazy.t

Lazy blocks: my $var = lazy { 42 }.

Proposal: fudge

22. S32-temporal/time.t

Tests for Perl-style routines and semantics of gmtime, etc. We have DateTime now.

Proposal: to remove

23. S06-advanced/return_function.t

Consider removal. The proposed semantics of | is rather awkward and fuzzy.

24. S06-advanced/caller.t

caller routine is not implemented and not planned.

Proposal: to remove

S32-str/val.t

Many questions. Trying to reach out @lizmat in Raku/roast#657. But anyone with expertise is welcome to consider it.

Proposal: either fix test or val(). In the latter case try to fudge the test with todo.

vrurg commented 4 years ago

@jnthn Since most of the problems one way or another are related to language design, I'd have to put it on your shoulders. But most of those were already waiting long enough, no hurry to get all this resolved. Therefore all I'm asking about is spend no more than a few seconds per case. If there is an immediate answer – ok. If not – just skip.

AlexDaniel commented 4 years ago

@vrurg I'd love to make it easier for you and everybody else. Thing is, there are many things that were “decided” at some point, and people are treating them as some sort of wisdom from the past. However, the fact that a lot of these features didn't make it into the language (and nobody is continuously requesting them) is a manifestation that they can be simply rejected. Also, I think people today have a much better understanding of what Raku should and shouldn't be, so you should be able to decide yourself whether these tests can be simply removed.

There was a recent issue with IO .add/.child that also somehow kicked off the “problem-solving repo doesn't work” idea, but I digress. Basically, one dev spent a lot of time deciding that a feature should behave in a certain way, and much much later another dev went ahead and tried to implement it in that way, despite people saying that today they don't see it as a good idea. A lot of wasted energy and effort for outdated wrong decisions.

My opinion is that you should be able to remove tests for outdated ideas without needing @jnthn's approval for every single one, and there isn't even a need to create a ticket (a short commit description will suffice).

AlexDaniel commented 4 years ago

So, thinking that I might be wrong I started looking through these, and here's my opinion:

  1. remove, no longer needed.
  2. remove. Features should address pain points or implement things that are often required. Class hierarchy already allows to generalize signatures in some cases, in other cases you can just call another sub from within. Very rare problem, it'd cost more to implement and maintain it than to work around it once in a lifetime.
  3. remove. Yeah, we no longer allow that, and it'd need to be something other than use v5 to make sense today…
  4. huh? Remove.
  5. remove. No, it doesn't look nice, but maybe something like this is needed. If somebody else reinvents that again properly then we can think, but remove it for now.
  6. remove.
  7. remove.
  8. remove.
  9. remove. This rings some bells to me. I thought I already removed these? Weird. If you search for a ticket somewhere maybe you'll find it, there was definitely a discussion on #raku-dev. Either way, remove.
  10. remove, yeah doesn't seem to mean anything nowadays
  11. …

OK, I got bored. I really didn't need to look through these. Obviously, you didn't put them into this ticket for no good reason. I'd say just feel free to remove all these tests and anything else you stumble upon in the future. Thank you for working on cleaning things up.

niner commented 4 years ago

3. S01-perl-5-integration/modify_inside_p5.t and

S01-perl-5-integration/modify_inside_p5_p6.t

{
  use v5;
  $x .= 'ing';
 {
    use v6;
    $y ~= 'book';
  }
}

I actually plan on implementing this (sharing of lexicals) at some point, though it's gonna be spelled { use v5-inline; ... } and raku { ... }

Our current design prohibits language version switching inside a compunit. I don't see why language switching should be different.

Those are entirely different topics.

AlexDaniel commented 4 years ago

though it's gonna be spelled

Speaking of which, these no longer need to be in roast. I think the goal was to make sure that Perl integration is tested before the release, but we should just make sure that Inline::Perl5 (or whatever it will be called given the v7 situation) and other critical modules are tested by Blin (or whatever we use in the future). It will simplify the release guide as well as give us more freedom to declare other modules critical. You can move tests mentioned in this ticket to the module if you need them.

vrurg commented 4 years ago

@AlexDaniel you're apparently in slash-and-burn mood lately. :)

@nine Are you going to do it as a module or make it into the core? In either case, the syntax is going to be different.

vrurg commented 4 years ago

For the note, so far the following three are to be removed:

12. S05-capture/external-aliasing.t 3. S01-perl-5-integration/modify_inside_p5.t and S01-perl-5-integration/modify_inside_p5_p6.t

AlexDaniel commented 4 years ago

@AlexDaniel you're apparently in slash-and-burn mood lately. :)

Well, we can't just keep adding things without ever cleaning up.