Opened 7 months ago

Closed 4 months ago

#1399 closed defect (fixed)

Scrutinizer produces incorrect procedure types after merge

Reported by: LemonBoy Owned by: evhan
Priority: major Milestone: 4.13.0
Component: scrutinizer Version: 4.12.0
Keywords: Cc:
Estimated difficulty: medium


Suppose we get this expression after scrutinizing some code:

(or (procedure (|#!rest|) noreturn)
    (procedure (* |#!rest|) . *))

At the moment the result of the union is (procedure (#!rest)) which is clearly wrong because it has arity N >= 0 while the second procedure in the or expression has arity N >= 1. Moreover the return type is lost, I'd expect the noreturn to be dropped and * to be used instead.

The result of the miscalculation of the resulting type may or may not be dangerous.

Change History (6)

comment:1 Changed 5 months ago by sjamaan

I'm not so sure that the arity is "clearly wrong", because it would be equally incorrect to give a warning when calling the procedure with zero arguments; the procedure might accept zero arguments (if the first alternative is used).

Perhaps the best we can do is not to simplify/merge the procedure, but just keep the or? I don't know what the impact of that would be though.

comment:2 Changed 5 months ago by megane

My understanding is that it is always safe to go for more general type. In this case procedure or even just * would be ok too. And N >= 0 is more general than N >= 1.

Notice that (procedure (#!rest)) does not specify return type. This is different from *.

Indeed, it would be interesting to know if there was any downsides if in this case the union was left as it is. Maybe it would make the scrutinizer slower..

comment:3 Changed 5 months ago by evhan

  • Owner set to evhan
  • Status changed from new to assigned

comment:4 Changed 5 months ago by evhan

On what version are you seeing this? The current behaviour in both master and 4.12.0 is, as far as I can tell, to simplify (or (procedure a (#!rest *) noreturn) (procedure b (* #!rest *) . *)) into (procedure (#!rest) . *), which is correct.

Is there a specific way to trigger this bug, and if so can you share?

comment:5 Changed 4 months ago by LemonBoy

Sure, I caught this error during the compilation of the sxpath egg, to be precise during the compilation of chicken/context-sxpath-lolevel.scm.
The warnings emitted are reported below:

  Warning: in toplevel procedure `context-sxpath-lolevel#draft:ast-step':
    expected a single result in `let' binding of `axis', but received zero results

  Warning: in toplevel procedure `context-sxpath-lolevel#draft:ast-step':
    expected a single result in `let' binding of `axis', but received zero results

You have to start from those warnings and work your way backwards to the source of the type for axis-lst. If I still remember correctly the nitty gritty details it's the analysis of draft:ast-axis-specifier that gets the type mentioned in the ticket.

Tested right now with CHICKEN Version 4.12.1 (rev 5746f0c1)

comment:6 Changed 4 months ago by sjamaan

  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed with 5ea816d and 62d7991.

Note: See TracTickets for help on using tickets.