Fix of * .. non-number Range scenarios #5377
Conversation
constructions, therefore it makes sense to infer the same rules for it as for Inf itself. Since modifying the type constraint changed the order of the resolution of 10 .. ^20 (might be a bug in itself), I rather made a coercion to Real in the function body.
| @@ -57,10 +57,12 @@ my class Range is Cool does Iterable does Positional { | |||
| nqp::create(self)!SET-SELF(-Inf,Inf,$excludes-min,$excludes-max,1); | |||
| } | |||
| multi method new(Whatever \min, \max, :$excludes-min, :$excludes-max) { | |||
| nqp::create(self)!SET-SELF(-Inf,max,$excludes-min,$excludes-max,1); | |||
| use fatal; | |||
There was a problem hiding this comment.
Hmmm.. but then why the "use fatal". I seem to be missing something?
There was a problem hiding this comment.
Because otherwise max.Real as a Failure will simply be passed on, and a monstrous Range (that starts/ends with a Failure) will be created.
There was a problem hiding this comment.
Aha, ok, then maybe SET-SELF would need an $max.throw if nqp::istype($max,Failure) ?
There was a problem hiding this comment.
Probably it should check for both $min and $max; I'm not sure of the performance implications. Right now, the other cases only throw because they tried to retrieve the failed value before doing the function call.
|
@lizmat well, it would be good to get this over with. I didn't want to add the check to !SET-SELF because that's common path and I felt that could be heavier on the performance. If I have this wrong, or it's insignificant here, sure I can just change to that. Either is better than leaving it as it is. |
|
Question: did you actually spectest these changes? I see pretty massive test failures with Turns out spectests say that Therefore closing this PR. |
That's quite bad given that they literally create ranges that are forbidden otherwise. I'm gonna open an issue for it. |
Resolves #5340
The whatever-star is always inferred to mean +/- Inf in Range
constructions, therefore it makes sense to infer the same rules for it as for Inf itself. Since modifying the type constraint changed the order of the resolution of 10 .. ^20 (might be a bug in itself), I rather made a coercion to Real in the function body.